Re: [PATCH v2] rust: file: handle fd table teardown in file descriptor APIs

[Alice Ryhl]

On Tue, Sep 29, 2026 at 3:28 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Sep 29, 2026 at 08:49:55AM +0000, Alice Ryhl wrote:
>
> > So, I previously wrote some code that could invoke filp_close() to close
> > a given fd ... from a workqueue. This was in the scenario where the
> > process dies and the usual cleanup function gets called deferred from a
> > workqueue instead of from the ioctl like usual.
>
> Huh?
>
> 1) filp_close(file, NULL) doesn't do _anything_ to any descriptor tables;
> the only requirements are that it should happen in _some_ thread context
> (workqueue is fine) and that caller should not be holding any locks that
> might be taken by ->flush() of the file in question (for a workqueue
> callback it's fine as long as the callback itself is not holding any
> of those).
>
> 2) any caller of filp_close(file, files_struct) must obviously guarantee
> that files_struct won't be freed under it; passing current->files from
> workqueue is safe in that respect, but obviously bogus.  Note that
> descriptor table in question will *still* not be accessed; it serves
> only as an opaque tag that identifies POSIX locks (and dnotify_struct
> instances) related to the descriptor table in question.  IF you have
> just manually removed the file in question from descriptor table
> (file_close_fd()), you must call filp_close() passing it the same
> descriptor table while that descriptor table is still guaranteed to
> be alive.
>
> Rationale is memory safety, actually - for POSIX locks descriptor table
> serves as lock owner; the reference is opaque, but we don't want to have
> it outlive freeing and reuse of the object it's pointing to.  So anything
> that removes some file reference from a descriptor table is responsible
> for corresponding filp_close() call done *before* the descriptor table
> is gone.
>
> Note that we only need to take care of the reference we remove from
> descriptor table; files_struct destructor will call filp_close() for
> anything still referenced from it.
>
> Places where file reference is removed from the table:
>         * do_close_on_exec(); filp_close() called in the same loop as
> clearing the descriptor table slot.
>         * do_dup2() in case the new slot had already been in use;
> filp_close() called just before return.
>         * file_close_fd_locked() callers. Three of those call filp_close()
> as soon as they drop ->files_lock (close_fd(), __range_close() and
> io_uring io_close()).  Remaining caller (close_fd_locked()) leaves that to
> _its_ callers (binder_deferred_fd_close() and its equivalent Rust-side).
>
> Again, normally both removal from descriptor table and filp_close() are
> done by the same primitive...
>
> > In this case the correct behavior was just to do nothing.
>
> ... leaking an opened file?  IDGI...
>
> > It was a very easy mistake to make, and if such mistakes lead to null
> > ptr derefs or worse, then I think it's worth doing something to reduce
> > the bad consequences from this kind of mistake.

Sorry I mixed it up ... the code called file_close_fd() and then
filp_close() with current->files. The important call here is the
file_close_fd() one, not the filp_close() one.

Alice