[alsa-devel] [PATCH 0/2] ALSA: pcm: implement the anonymous dup v3

[Jaroslav Kysela]

Dne 31.1.2019 v 21:32 Takashi Iwai napsal(a):
> On Thu, 31 Jan 2019 20:54:33 +0100,
> Zach Riggle 🖖 wrote:
>>
>> The big concerns on our end are:
>>
>> (1) Can the buffer be mremap()ed with a different offset into the buffer? 
>> This was a concern in the past and the reason for the anon_inode stuff at
>> all.  I believe that as long as the *size* of the mapping doesn't change,
>> Linux mm will gladly permit mremap() without informing the driver.
> 
> Could you elaborate which perspective of mremap() can be a big
> problem?  The driver interface does nothing but a standard mmap for
> now.

Yes, basically, the sound buffer (including it's size) is locked in the
kernel space until the last mmaped area is active as it should be. See
substream->mmap_count . The mmap implementation is similar what the
dma-buf file interface does.

>> (2) Can we ensure that permissions can only ever be dropped? (new_perms =
>> old_perms & requested_perms) . It's probably useful to throw an error code if
>> new permissions are requested.
> 
> The permission is bound with each fd, and determined only at creation
> time, and unchangeable.

Exactly. The caller who creates the duplicated file descriptor should
specify the permissions which are locked for the lifetime of the
restricted file descriptor.

> Also, the patches Jaroslav posted can become even simpler / safer;
> i.e. we don't need to introduce the perms bits as a start.  IMO, we
> can begin with the minimum, mmap-only file ops.  The API should be
> defined properly from the beginning (e.g. passing perms argument,
> etc), of course.  Then, if any request comes up, we may extend later.

I agree. We can have just two modes for the beginning:

a) full one (useful for testing)
b) buffer only (allow just sound data mmap)

The question, if we should use different names (line anon_inode:snd-pcm
and anon_inode:snd-pcm-buffer) for the anonymous inodes remains. I
believe it might be good to distinguish this to allow the proper SELinux
audit.

>> (3) It looks like the code for snd_pcm_anonymous_dup in the patchset takes a 
>> perm argument from user-space and discards it.  Is this intended?
> 
> My understanding is that it's the design to be simple.  But we don't
> need to stick with it, if you can suggest any better interface.

Yes, I preferred the simplicity (so only one integer argument in and out).

>> (4) I'm not familiar with the lifecycle of all of the objects, and introducing
>> a custom dup routine might cause unexpected problems (e.g. use-after-free,
>> double-free).  I'm not well-versed-enough in how the driver-specific stuff is
>> handled underneath e.g. snd_card_file_add / snd_card_file_remove to be sure
>> about it.
> 
> That's the advantage of anon-dup implementation; the PCM stream
> handling with O_APPEND has been heavily used by alsa-lib dmix PCM
> implementations.  So it already survives over decades.

Yes, there is simple reference counter which contains the count of the
file decriptors assigned to the pcm substream -
substream->ref_count . See snd_pcm_release_substream() for more details.

>> On Thu, Jan 31, 2019 at 1:36 PM Phil Burk <philburk at google.com> wrote:
>>>     permission. What flag allows close? What else is permitted under that
>>     flag? Or is close permission  just a generic "FD" permission unrelated to
>>     ALSA?

All file descriptors must have defined the 'release' callback in the
kernel to free the resources when the close syscall is executed.

					Jaroslav

-- 
Jaroslav Kysela <perex at perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.