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@google.com wrote:
permission. What flag allows close? What else is permitted under thatflag? 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