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@xxxxxxxxxx> 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@xxxxxxxx> Linux Sound Maintainer; ALSA Project; Red Hat, Inc. _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
