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

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

[Georgios Androutsopoulos]

Several Rust file descriptor APIs rely on `current->files` being
available. However, `exit_files()` clears it while execution may still
continue on the same task.

This affects `LocalFile::fget()` and the
`FileDescriptorReservation` operations that call
`get_unused_fd_flags()`, `fd_install()`, and `put_unused_fd()`.
`FileDescriptorReservation` cannot cross task boundaries, but remaining
on the same task does not guarantee that `current->files` is still
available when these operations are performed.

Guard the affected operations against a missing `current->files`.
`LocalFile::fget()` returns `EBADF` and
`FileDescriptorReservation::get_unused_fd_flags()` returns `EMFILE`.
For `fd_install()`, warn and abandon the reservation when the fd table
is already gone. In the drop path, skip `put_unused_fd()` after the fd
table has been torn down.

This prevents NULL dereferences through these safe Rust APIs after
`exit_files()`.

Fixes: 851849824bb5 ("rust: file: add Rust abstraction for `struct file`")
Fixes: 5da9857b127e ("rust: file: add `FileDescriptorReservation`")
Closes: https://github.com/Rust-for-Linux/linux/issues/1256
Signed-off-by: Georgios Androutsopoulos <georgeandrout13@gmail.com>
---
Changes in v2:
- Use `warn_on!()` for `FileDescriptorReservation::fd_install()` and
  remove the warning from the drop path, following feedback from Gary Guo.
- Link to v1: https://lore.kernel.org/rust-for-linux/20260920200154.1194983-1-georgeandrout13@gmail.com/
---
 rust/kernel/fs/file.rs | 64 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 55 insertions(+), 9 deletions(-)

diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs
index 23ee689bd240..8b5b5ec4cd04 100644
--- a/rust/kernel/fs/file.rs
+++ b/rust/kernel/fs/file.rs
@@ -260,7 +260,17 @@ impl LocalFile {
     /// [`assume_no_fdget_pos`]: LocalFile::assume_no_fdget_pos
     #[inline]
     pub fn fget(fd: u32) -> Result<ARef<LocalFile>, BadFdError> {
-        // SAFETY: FFI call, there are no requirements on `fd`.
+        let current = crate::current!();
+
+        // SAFETY: `current` points to the currently executing task, so it is
+        // valid to read its `files` pointer. The pointer may be null during
+        // task teardown.
+        if unsafe { (*current.as_ptr()).files.is_null() } {
+            return Err(BadFdError);
+        }
+
+        // SAFETY: There are no requirements on `fd`. We checked above that the
+        // current task still has a file descriptor table, which `fget` accesses.
         let ptr = ptr::NonNull::new(unsafe { bindings::fget(fd) }).ok_or(BadFdError)?;
 
         // SAFETY: `bindings::fget` created a refcount, and we pass ownership of it to the `ARef`.
@@ -403,7 +413,18 @@ impl FileDescriptorReservation {
     /// Creates a new file descriptor reservation.
     #[inline]
     pub fn get_unused_fd_flags(flags: u32) -> Result<Self> {
-        // SAFETY: FFI call, there are no safety requirements on `flags`.
+        let current = crate::current!();
+
+        // SAFETY: `current` points to the currently executing task, so it is
+        // valid to read its `files` pointer. The pointer may be null during
+        // task teardown.
+        if unsafe { (*current.as_ptr()).files.is_null() } {
+            return Err(EMFILE);
+        }
+
+        // SAFETY: There are no safety requirements on `flags`. We checked above
+        // that the current task still has a file descriptor table, which
+        // `get_unused_fd_flags` accesses.
         let fd: i32 = unsafe { bindings::get_unused_fd_flags(flags) };
         to_result(fd)?;
 
@@ -421,13 +442,26 @@ pub fn reserved_fd(&self) -> u32 {
 
     /// Commits the reservation.
     ///
-    /// The previously reserved file descriptor is bound to `file`. This method consumes the
-    /// [`FileDescriptorReservation`], so it will not be usable after this call.
+    /// The previously reserved file descriptor is bound to `file`. If the current task no longer
+    /// has a file descriptor table, the reservation is abandoned instead. This method consumes the
+    /// [`FileDescriptorReservation`] in either case.
     #[inline]
     pub fn fd_install(self, file: ARef<File>) {
-        // SAFETY: `self.fd` was previously returned by `get_unused_fd_flags`. We have not yet used
-        // the fd, so it is still valid, and `current` still refers to the same task, as this type
-        // cannot be moved across task boundaries.
+        let current = crate::current!();
+
+        // SAFETY: `current` points to the currently executing task, so it is
+        // valid to read its `files` pointer. The pointer may be null during
+        // task teardown.
+        if crate::warn_on!(unsafe { (*current.as_ptr()).files.is_null() }) {
+            // `put_unused_fd` also requires `current->files` to be valid, so do not run
+            // the reservation's destructor after the current task has lost its fd table.
+            core::mem::forget(self);
+            return;
+        }
+
+        // SAFETY: `self.fd` was previously returned by `get_unused_fd_flags` and has not yet been
+        // used. This type cannot be moved across task boundaries, so `current` still refers to the
+        // same task, and we checked above that it still has an fd table.
         //
         // Furthermore, the file pointer is guaranteed to own a refcount by its type invariants,
         // and we take ownership of that refcount by not running the destructor below.
@@ -446,9 +480,21 @@ pub fn fd_install(self, file: ARef<File>) {
 impl Drop for FileDescriptorReservation {
     #[inline]
     fn drop(&mut self) {
+        let current = crate::current!();
+
+        // SAFETY: `current` points to the currently executing task, so it is
+        // valid to read its `files` pointer. The pointer may be null during
+        // task teardown.
+        if unsafe { (*current.as_ptr()).files.is_null() } {
+            // `put_unused_fd` uses `current->files`, so it cannot be called
+            // after the current task has torn down its fd table.
+            return;
+        }
+
         // SAFETY: By the type invariants of this type, `self.fd` was previously returned by
-        // `get_unused_fd_flags`. We have not yet used the fd, so it is still valid, and `current`
-        // still refers to the same task, as this type cannot be moved across task boundaries.
+        // `get_unused_fd_flags` and has not yet been used. This type cannot be moved across task
+        // boundaries, so `current` still refers to the same task, and we checked above that it
+        // still has an fd table.
         unsafe { bindings::put_unused_fd(self.fd) };
     }
 }
-- 
2.47.3

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

[Alice Ryhl]

On Tue, Sep 22, 2026 at 10:23:39PM -0400, Georgios Androutsopoulos wrote:
> Several Rust file descriptor APIs rely on `current->files` being
> available. However, `exit_files()` clears it while execution may still
> continue on the same task.
> 
> This affects `LocalFile::fget()` and the
> `FileDescriptorReservation` operations that call
> `get_unused_fd_flags()`, `fd_install()`, and `put_unused_fd()`.
> `FileDescriptorReservation` cannot cross task boundaries, but remaining
> on the same task does not guarantee that `current->files` is still
> available when these operations are performed.
> 
> Guard the affected operations against a missing `current->files`.
> `LocalFile::fget()` returns `EBADF` and
> `FileDescriptorReservation::get_unused_fd_flags()` returns `EMFILE`.
> For `fd_install()`, warn and abandon the reservation when the fd table
> is already gone. In the drop path, skip `put_unused_fd()` after the fd
> table has been torn down.
> 
> This prevents NULL dereferences through these safe Rust APIs after
> `exit_files()`.
> 
> Fixes: 851849824bb5 ("rust: file: add Rust abstraction for `struct file`")
> Fixes: 5da9857b127e ("rust: file: add `FileDescriptorReservation`")
> Closes: https://github.com/Rust-for-Linux/linux/issues/1256
> Signed-off-by: Georgios Androutsopoulos <georgeandrout13@gmail.com>

This looks like it should ideally be on the C side instead.

Alice

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

[Georgios Androutsopoulos]

On Thu, Sep 24, 2026 at 4:39 AM Alice Ryhl <aliceryhl@google.com> wrote:
> This looks like it should ideally be on the C side instead.

I could move the `NULL` checks into the C helpers in `fs/file.c` that
currently dereference `current->files`. Since this would move the fix
into the VFS code, would it be okay to send this as v3 of this patch?

Best,
George

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

[Pedro Falcato]

On Thu, Sep 24, 2026 at 12:06:18PM -0400, Georgios Androutsopoulos wrote:
> On Thu, Sep 24, 2026 at 4:39 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > This looks like it should ideally be on the C side instead.
> 
> I could move the `NULL` checks into the C helpers in `fs/file.c` that
> currently dereference `current->files`. Since this would move the fix
> into the VFS code, would it be okay to send this as v3 of this patch?

FWIW, I don't think this makes sense. The VFS abstractions should ideally
be taught that using any of these functions from ->release() simply isn't
safe.

Adding random branches to rust or C code just to avoid UB sounds like
delaying the inevitable; maybe your kernel doesn't crash (right away?),
but the code is still incorrect and probably cannot correctly handle FD
installation randomly failing, or fdget on a Known Good(tm) fd failing.

-- 
Pedro

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

[Christian Brauner]

On Thu, Sep 24, 2026 at 08:39:37AM +0000, Alice Ryhl wrote:
> On Tue, Sep 22, 2026 at 10:23:39PM -0400, Georgios Androutsopoulos wrote:
> > Several Rust file descriptor APIs rely on `current->files` being
> > available. However, `exit_files()` clears it while execution may still
> > continue on the same task.
> > 
> > This affects `LocalFile::fget()` and the
> > `FileDescriptorReservation` operations that call
> > `get_unused_fd_flags()`, `fd_install()`, and `put_unused_fd()`.
> > `FileDescriptorReservation` cannot cross task boundaries, but remaining
> > on the same task does not guarantee that `current->files` is still
> > available when these operations are performed.
> > 
> > Guard the affected operations against a missing `current->files`.
> > `LocalFile::fget()` returns `EBADF` and
> > `FileDescriptorReservation::get_unused_fd_flags()` returns `EMFILE`.
> > For `fd_install()`, warn and abandon the reservation when the fd table
> > is already gone. In the drop path, skip `put_unused_fd()` after the fd
> > table has been torn down.
> > 
> > This prevents NULL dereferences through these safe Rust APIs after
> > `exit_files()`.
> > 
> > Fixes: 851849824bb5 ("rust: file: add Rust abstraction for `struct file`")
> > Fixes: 5da9857b127e ("rust: file: add `FileDescriptorReservation`")
> > Closes: https://github.com/Rust-for-Linux/linux/issues/1256
> > Signed-off-by: Georgios Androutsopoulos <georgeandrout13@gmail.com>
> 
> This looks like it should ideally be on the C side instead.

Where is this godforsaken broken code, that tries to fd_install() after
exit_files(). It is _a bug in the program_ that is not something the
apis need to work around.

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

[kernel test robot]

Hi Georgios,

kernel test robot noticed the following build warnings:

[auto build test WARNING on brauner-vfs/vfs.all]
[also build test WARNING on linus/master v7.3-rc5 next-20260925]
[cannot apply to linux-review/Georgios-Androutsopoulos/rust-file-handle-fd-table-teardown-in-file-descriptor-APIs/20260920-160154]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Georgios-Androutsopoulos/rust-file-handle-fd-table-teardown-in-file-descriptor-APIs/20260922-222339
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
patch link:    https://lore.kernel.org/r/20260923022339.3340694-1-georgeandrout13%40gmail.com
patch subject: [PATCH v2] rust: file: handle fd table teardown in file descriptor APIs
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20260928/202609281234.H1AixLDP-lkp@intel.com/config)
compiler: clang version 22.1.8 (https://github.com/llvm/llvm-project ca7933e47d3a3451d81e72ac174dcb5aa28b59d1)
rustc: rustc 1.96.0 (ac68faa20 2026-05-25)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260928/202609281234.H1AixLDP-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202609281234.H1AixLDP-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> warning: statement has unnecessary safety comment
   --> rust/kernel/fs/file.rs:455:9
   |
   455 | /         if crate::warn_on!(unsafe { (*current.as_ptr()).files.is_null() }) {
   456 | |             // `put_unused_fd` also requires `current->files` to be valid, so do not run
   457 | |             // the reservation's destructor after the current task has lost its fd table.
   458 | |             core::mem::forget(self);
   459 | |             return;
   460 | |         }
   | |_________^
   |
   help: consider removing the safety comment
   --> rust/kernel/fs/file.rs:452:12
   |
   452 |         // SAFETY: `current` points to the currently executing task, so it is
   |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.96.0/index.html#unnecessary_safety_comment
   = note: requested on the command line with `-W clippy::unnecessary-safety-comment`

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Al Viro]

On Fri, Sep 25, 2026 at 06:00:42PM +0200, Christian Brauner wrote:
> On Thu, Sep 24, 2026 at 08:39:37AM +0000, Alice Ryhl wrote:

> > This looks like it should ideally be on the C side instead.
> 
> Where is this godforsaken broken code, that tries to fd_install() after
> exit_files(). It is _a bug in the program_ that is not something the
> apis need to work around.

	More to the point, papering over that at runtime is wrong, and not
just for modifying descriptor tables - fdget() is just as wrong in anything
that can be called from tail of do_exit().

	It's exactly the same as with "what if it gets called from an
rcu callback?" - it's a bug, that's what.  Don't use these primitives
in such context.

	In particular, ->release() mentioned upthread should not be allowed
to access _anything_ hanging off current, not just descriptor table.  Note
that the last reference to an opened file might be sitting in an SCM_RIGHTS
datagram pruned by AF_UNIX garbage collector; as far as the method is concerned,
it might be called from random thread.

	If it tries to access (let alone modify) the current descriptor table,
you have no memory safety whatsoever and checking if current->files happens
to be NULL is nowhere near enough to resolve that.

	I don't know how to express that gracefully in terms of typechecking -
sure, we could pass an empty token to each syscall, have fdget() et.al.
require that as an argument and propagate the damn thing to all such callsites,
but that would cause an insane amount of churn - if nothing else, ->ioctl()
signature would have to be changed and there's a _lot_ of instances out there.
And then there's the joy of dealing with ->sendmsg() and ->recvmsg(),
thanks to SCM_RIGHTS datagrams, again (reading descriptor table on sendmsg()
side, inserting into it on recvmsg()), especially when you consider the
fact that ->sendmsg() and ->recvmsg() *are* callable from contexts where
one shouldn't be allowed to access descriptor tables.  None of such
call chains is going to trigger descriptor table access (e.g. knbd is
not going to try and send SCM_RIGHTS datagrams, etc.), so it should be
safe, but having compiler prove that without inflicting overhead on
the code paths where it really wouldn't be welcome is not going to be trivial.


Al, finally back to the state when reading from screen is tolerable for
reasonably long time - dry eyes were _really_ not fun to deal with...

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

[Alice Ryhl]

On Tue, Sep 29, 2026 at 05:48:43AM +0100, Al Viro wrote:
> On Fri, Sep 25, 2026 at 06:00:42PM +0200, Christian Brauner wrote:
> > On Thu, Sep 24, 2026 at 08:39:37AM +0000, Alice Ryhl wrote:
> 
> > > This looks like it should ideally be on the C side instead.
> > 
> > Where is this godforsaken broken code, that tries to fd_install() after
> > exit_files(). It is _a bug in the program_ that is not something the
> > apis need to work around.
> 
> 	More to the point, papering over that at runtime is wrong, and not
> just for modifying descriptor tables - fdget() is just as wrong in anything
> that can be called from tail of do_exit().
> 
> 	It's exactly the same as with "what if it gets called from an
> rcu callback?" - it's a bug, that's what.  Don't use these primitives
> in such context.
> 
> 	In particular, ->release() mentioned upthread should not be allowed
> to access _anything_ hanging off current, not just descriptor table.  Note
> that the last reference to an opened file might be sitting in an SCM_RIGHTS
> datagram pruned by AF_UNIX garbage collector; as far as the method is concerned,
> it might be called from random thread.
> 
> 	If it tries to access (let alone modify) the current descriptor table,
> you have no memory safety whatsoever and checking if current->files happens
> to be NULL is nowhere near enough to resolve that.
> 
> 	I don't know how to express that gracefully in terms of typechecking -
> sure, we could pass an empty token to each syscall, have fdget() et.al.
> require that as an argument and propagate the damn thing to all such callsites,
> but that would cause an insane amount of churn - if nothing else, ->ioctl()
> signature would have to be changed and there's a _lot_ of instances out there.
> And then there's the joy of dealing with ->sendmsg() and ->recvmsg(),
> thanks to SCM_RIGHTS datagrams, again (reading descriptor table on sendmsg()
> side, inserting into it on recvmsg()), especially when you consider the
> fact that ->sendmsg() and ->recvmsg() *are* callable from contexts where
> one shouldn't be allowed to access descriptor tables.  None of such
> call chains is going to trigger descriptor table access (e.g. knbd is
> not going to try and send SCM_RIGHTS datagrams, etc.), so it should be
> safe, but having compiler prove that without inflicting overhead on
> the code paths where it really wouldn't be welcome is not going to be trivial.

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.

In this case the correct behavior was just to do nothing.

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.

Alice

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

[Gary Guo]

On Tue Sep 29, 2026 at 5:48 AM BST, Al Viro wrote:
> On Fri, Sep 25, 2026 at 06:00:42PM +0200, Christian Brauner wrote:
>> On Thu, Sep 24, 2026 at 08:39:37AM +0000, Alice Ryhl wrote:
>
>> > This looks like it should ideally be on the C side instead.
>> 
>> Where is this godforsaken broken code, that tries to fd_install() after
>> exit_files(). It is _a bug in the program_ that is not something the
>> apis need to work around.
>
> 	More to the point, papering over that at runtime is wrong, and not
> just for modifying descriptor tables - fdget() is just as wrong in anything
> that can be called from tail of do_exit().
>
> 	It's exactly the same as with "what if it gets called from an
> rcu callback?" - it's a bug, that's what.  Don't use these primitives
> in such context.
>
> 	In particular, ->release() mentioned upthread should not be allowed
> to access _anything_ hanging off current, not just descriptor table.  Note
> that the last reference to an opened file might be sitting in an SCM_RIGHTS
> datagram pruned by AF_UNIX garbage collector; as far as the method is concerned,
> it might be called from random thread.

This part is protected -- we mark FileDescriptionReservation as `!Send` which
prevents it being moved to another task. We also take care to make sure that
the task returned `current!()` is not allowed to be used outside the current
task.

> 	If it tries to access (let alone modify) the current descriptor table,
> you have no memory safety whatsoever and checking if current->files happens
> to be NULL is nowhere near enough to resolve that.

So at least for this part, I think checking for `current->files` would be a
sufficient protection for Rust code. If we don't want to add these checks to the
C implementation, I think adding these checks to Rust wrappers would be ideal
until we figure out how to check things statically.

Calling fget/get_unused_fd_flags is still broken code -- so we could still
`WARN` on them.

>
> 	I don't know how to express that gracefully in terms of typechecking -
> sure, we could pass an empty token to each syscall, have fdget() et.al.
> require that as an argument and propagate the damn thing to all such callsites,
> but that would cause an insane amount of churn - if nothing else, ->ioctl()
> signature would have to be changed and there's a _lot_ of instances out there.
> And then there's the joy of dealing with ->sendmsg() and ->recvmsg(),
> thanks to SCM_RIGHTS datagrams, again (reading descriptor table on sendmsg()
> side, inserting into it on recvmsg()), especially when you consider the
> fact that ->sendmsg() and ->recvmsg() *are* callable from contexts where
> one shouldn't be allowed to access descriptor tables.  None of such
> call chains is going to trigger descriptor table access (e.g. knbd is
> not going to try and send SCM_RIGHTS datagrams, etc.), so it should be
> safe, but having compiler prove that without inflicting overhead on
> the code paths where it really wouldn't be welcome is not going to be trivial.

I think for the simpler case, it is possible to check it statically by declaring
some functions to be only callable from "syscall context" and define ->release
to be not of that context.

Best,
Gary

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

[Al Viro]

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.

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@zeniv.linux.org.uk> 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

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

[Al Viro]

On Tue, Sep 29, 2026 at 01:24:45PM +0100, Gary Guo wrote:

> > 	In particular, ->release() mentioned upthread should not be allowed
> > to access _anything_ hanging off current, not just descriptor table.  Note
> > that the last reference to an opened file might be sitting in an SCM_RIGHTS
> > datagram pruned by AF_UNIX garbage collector; as far as the method is concerned,
> > it might be called from random thread.
> 
> This part is protected -- we mark FileDescriptionReservation as `!Send` which
> prevents it being moved to another task. We also take care to make sure that
> the task returned `current!()` is not allowed to be used outside the current
> task.

Not the point - ->release() can't make any assumptions regarding which thread
it will be run in.  current won't change under it, but there's nothing
useful you could want with it.  If it has non-NULL ->files (or ->mm, or...),
that reference will also remain stable, but any attempt to do anything with
it would be a bug.

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

[Al Viro]

On Tue, Sep 29, 2026 at 03:35:13PM +0200, Alice Ryhl wrote:

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

Er...  But that would *not* see NULL ->files in case of workqueue -
you'd get init_files instead.  vhost_task_create() is the only
thing that creates threads with NULL ->files; it might or might
not make sense to have the same for workqueue worker threads,
but that wouldn't change anything in that bug anyway.

When you want to change the state of a thread component, be it
descriptor table, cwd, umask, etc., you'd better do that in
a thread that *does* share that component with the thread you
want to have affected.

It's not a matter of race with exiting thread - closing a descriptor
via schedule_work() is _always_ wrong.

I really don't get it...

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

[Gary Guo]

On Tue Sep 29, 2026 at 2:51 PM BST, Al Viro wrote:
> On Tue, Sep 29, 2026 at 01:24:45PM +0100, Gary Guo wrote:
>
>> > 	In particular, ->release() mentioned upthread should not be allowed
>> > to access _anything_ hanging off current, not just descriptor table.  Note
>> > that the last reference to an opened file might be sitting in an SCM_RIGHTS
>> > datagram pruned by AF_UNIX garbage collector; as far as the method is concerned,
>> > it might be called from random thread.
>> 
>> This part is protected -- we mark FileDescriptionReservation as `!Send` which
>> prevents it being moved to another task. We also take care to make sure that
>> the task returned `current!()` is not allowed to be used outside the current
>> task.
>
> Not the point - ->release() can't make any assumptions regarding which thread
> it will be run in.  current won't change under it, but there's nothing
> useful you could want with it.  If it has non-NULL ->files (or ->mm, or...),
> that reference will also remain stable, but any attempt to do anything with
> it would be a bug.

What I am saying is that due to FileDescriptorReservation being `!Send`, it
cannot make its way to ->release() from another thread. So
FileDescriptorReservation::drop will guarantee that it is on the same task that
created it.

The reproducer that Georgios posted on GitHub is some cleanup job being added to
task_work, which drops FileDescriptorReservation. And since exit_task_work()
happens after exit_files(), put_unused_fd in that cleanup observe that
current->files is NULL.

So it's not from random thread, it's from the current task. And I find that
particular case of doing per-task cleanup not unrealistic.

For fget/get_unused_fd_flags I already mentioned that I agree it's a bug, but
checking that statically would require codify the concept of syscall context in
a static analysis.

Best,
Gary

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

[Al Viro]

On Tue, Sep 29, 2026 at 05:07:40PM +0100, Gary Guo wrote:

> The reproducer that Georgios posted on GitHub is some cleanup job being added to
> task_work, which drops FileDescriptorReservation. And since exit_task_work()
> happens after exit_files(), put_unused_fd in that cleanup observe that
> current->files is NULL.
>
> So it's not from random thread, it's from the current task. And I find that
> particular case of doing per-task cleanup not unrealistic.

FWIW, descriptor reservation ought to be tied to specific files_struct
instance; note that dup_fd() can be called when there are outstanding
reservations and the copy does *NOT* have those reserved.

What rules would you suggest for such delayed put_unused_fd() wrt e.g.
files_struct unsharing?

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

[Gary Guo]

On Tue Sep 29, 2026 at 6:02 PM BST, Al Viro wrote:
> On Tue, Sep 29, 2026 at 05:07:40PM +0100, Gary Guo wrote:
>
>> The reproducer that Georgios posted on GitHub is some cleanup job being added to
>> task_work, which drops FileDescriptorReservation. And since exit_task_work()
>> happens after exit_files(), put_unused_fd in that cleanup observe that
>> current->files is NULL.
>>
>> So it's not from random thread, it's from the current task. And I find that
>> particular case of doing per-task cleanup not unrealistic.
>
> FWIW, descriptor reservation ought to be tied to specific files_struct
> instance; note that dup_fd() can be called when there are outstanding
> reservations and the copy does *NOT* have those reserved.
>
> What rules would you suggest for such delayed put_unused_fd() wrt e.g.
> files_struct unsharing?

Ah, is this about `unshare(CLONE_FILES)`? For that case indeed our existing
abstraction break down.

So put_unused_fd must be called in the same syscall context as
get_unused_fd_flags.. I think we can address this by give it a lifetime
parameter

    struct FileDescriptorReservation<'a>(..);

and require `current!()` to be passed in:

    impl<'a> FileDescriptorReservation<'a> {
        pub fn get_unused_fd_flags(_current: &'a CurrentTask, flags: u32);
    }

    // User
    FileDescriptorReservation::get_unused_fd_flags(current!(), flags)

I checked binder I think its current use can work with this, although it needs
some changes to pass this `&CurrentTask` token in.

Best,
Gary

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

[Al Viro]

On Tue, Sep 29, 2026 at 07:22:28PM +0100, Gary Guo wrote:
> On Tue Sep 29, 2026 at 6:02 PM BST, Al Viro wrote:
> > On Tue, Sep 29, 2026 at 05:07:40PM +0100, Gary Guo wrote:
> >
> >> The reproducer that Georgios posted on GitHub is some cleanup job being added to
> >> task_work, which drops FileDescriptorReservation. And since exit_task_work()
> >> happens after exit_files(), put_unused_fd in that cleanup observe that
> >> current->files is NULL.
> >>
> >> So it's not from random thread, it's from the current task. And I find that
> >> particular case of doing per-task cleanup not unrealistic.
> >
> > FWIW, descriptor reservation ought to be tied to specific files_struct
> > instance; note that dup_fd() can be called when there are outstanding
> > reservations and the copy does *NOT* have those reserved.
> >
> > What rules would you suggest for such delayed put_unused_fd() wrt e.g.
> > files_struct unsharing?
> 
> Ah, is this about `unshare(CLONE_FILES)`? For that case indeed our existing
> abstraction break down.

FWIW, the current rules are "you must not have any outstanding reservations when
you unshare descriptor table in any manner".  You are adding "... including the
ones that would be discarded by an already-scheduled task_work callback".

It's not just unshare(2) - there are more interesting callchains. For example,
unshare_files() from do_coredump(); this one should be fine in face of
put_unused_fd() in task_work, due to the task_work_run() in get_signal()
being upstream of vfs_coredump() call, but it needs to be considered.
Or begin_new_exec() - that has a lot more callchains leading to it.
It should be safe at the moment, but that needs to be demonstrated, etc.