Re: [PATCH v6 3/8] rust: file: add Rust abstraction for `struct file`

[Al Viro]

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...