On Tue, May 28, 2024 at 10:29:03PM +0200, Alice Ryhl wrote: > > Incidentally, I'm very tempted to unexport close_fd(); in addition to > > being a source of bugs when called from ioctl on user-supplied descriptor > > it encourages racy crap - just look at e.g. 1819200166ce > > "drm/amdkfd: Export DMABufs from KFD using GEM handles", where we > > call drm_gem_prime_handle_to_fd(), immediately followed by > > dmabuf = dma_buf_get(fd); > > close_fd(fd); > > dup2() from another thread with guessed descriptor number as target and > > you've got a problem... It's not a violation of fdget() use rules > > (it is called from ioctl, but descriptor is guaranteed to be different > > from the one passed to ioctl(2)), but it's still wrong. Would take > > some work, though... > > Wait, what's going on there? It adds the fd and then immediately > removes it again, or? It creates an object and associated struct file, using a primitive that shoves the reference to that new struct file into descriptor table and returns the slot number. Then it looks the file up by the returned descriptor, tries to pick the object out of it and closes the descriptor. If that descriptor table is shared, well... pray the descriptor still refers to the same file by the time you try to look the file up. It's bogus; the song and dance with putting it into descriptor table makes sense for the primary user (ioctl that returns the descriptor number to userland), but here it's just plain wrong. What they need is to cut that sucker in two functions - one that returns dmabuf, with wrapper doing dma_buf_fd() on the result (or allocating a descriptor first, then calling the primitives that gets their dmabuf, then doing fd_install()). This caller should use the new primitive without messing with descriptor table. In general, new descriptors are fit only for one thing - returning them to userland. As soon as file reference is in descriptor table it might get closed right under you - file argument of fd_install() is moved, not borrowed. You might find something on lookup by that descritor, but it's not guaranteed to have anything to do with what you'd just put there. That's why we have anon_inode_getfile(), with anon_inode_getfd() being only a convenience helper, for example...
