Message-ID: <aYSfBJA4hR4shPfI@google.com>
Date: Thu, 5 Feb 2026 13:45:40 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Carlos Llamas <cmllamas@...gle.com>, 
	Alexander Viro <viro@...iv.linux.org.uk>, Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, 
	Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>, 
	"Serge E. Hallyn" <serge@...lyn.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	Dave Chinner <david@...morbit.com>, Qi Zheng <zhengqi.arch@...edance.com>, 
	Roman Gushchin <roman.gushchin@...ux.dev>, Muchun Song <muchun.song@...ux.dev>, 
	David Hildenbrand <david@...nel.org>, "Liam R. Howlett" <Liam.Howlett@...cle.com>, 
	Vlastimil Babka <vbabka@...e.cz>, Mike Rapoport <rppt@...nel.org>, Suren Baghdasaryan <surenb@...gle.com>, 
	Michal Hocko <mhocko@...e.com>, Miguel Ojeda <ojeda@...nel.org>, Boqun Feng <boqun.feng@...il.com>, 
	Gary Guo <gary@...yguo.net>, 
	"Björn Roy Baron" <bjorn3_gh@...tonmail.com>, Benno Lossin <lossin@...nel.org>, 
	Andreas Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>, 
	Danilo Krummrich <dakr@...nel.org>, kernel-team@...roid.com, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org, 
	linux-mm@...ck.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 1/5] export file_close_fd and task_work_add

On Thu, Feb 05, 2026 at 11:53:19AM +0000, Lorenzo Stoakes wrote:
> On Thu, Feb 05, 2026 at 11:42:46AM +0000, Alice Ryhl wrote:
> > On Thu, Feb 05, 2026 at 11:20:33AM +0000, Lorenzo Stoakes wrote:
> > > On Thu, Feb 05, 2026 at 10:51:26AM +0000, Alice Ryhl wrote:
> > > > This exports the functionality needed by Binder to close file
> > > > descriptors.
> > > >
> > > > When you send a fd over Binder, what happens is this:
> > > >
> > > > 1. The sending process turns the fd into a struct file and stores it in
> > > >    the transaction object.
> > > > 2. When the receiving process gets the message, the fd is installed as a
> > > >    fd into the current process.
> > > > 3. When the receiving process is done handling the message, it tells
> > > >    Binder to clean up the transaction. As part of this, fds embedded in
> > > >    the transaction are closed.
> > > >
> > > > Note that it was not always implemented like this. Previously the
> > > > sending process would install the fd directly into the receiving proc in
> > > > step 1, but as discussed previously [1] this is not ideal and has since
> > > > been changed so that fd install happens during receive.
> > > >
> > > > The functions being exported here are for closing the fd in step 3. They
> > > > are required because closing a fd from an ioctl is in general not safe.
> > > > This is to meet the requirements for using fdget(), which is used by the
> > > > ioctl framework code before calling into the driver's implementation of
> > > > the ioctl. Binder works around this with this sequence of operations:
> > > >
> > > > 1. file_close_fd()
> > > > 2. get_file()
> > > > 3. filp_close()
> > > > 4. task_work_add(current, TWA_RESUME)
> > > > 5. <binder returns from ioctl>
> > > > 6. fput()
> > > >
> > > > This ensures that when fput() is called in the task work, the fdget()
> > > > that the ioctl framework code uses has already been fdput(), so if the
> > > > fd being closed happens to be the same fd, then the fd is not closed
> > > > in violation of the fdget() rules.
> > >
> > > I'm not really familiar with this mechanism but you're already talking about
> > > this being a workaround so strikes me the correct thing to do is to find a way
> > > to do this in the kernel sensibly rather than exporting internal implementation
> > > details and doing it in binder.
> >
> > I did previously submit a patch that implemented this logic outside of
> > Binder, but I was advised to move it into Binder.
> 
> Right yeah that's just odd to me, we really do not want to be adding internal
> implementation details to drivers.
> 
> This is based on bitter experience of bugs being caused by drivers abusing every
> interface they get, which is basically exactly what always happens, sadly.
> 
> And out-of-tree is heavily discouraged.
> 
> Also can we use EXPORT_SYMBOL_FOR_MODULES() for anything we do need to export to
> make it explicitly only for binder, perhaps?
> 
> >
> > But I'm happy to submit a patch to extract this logic into some sort of
> > close_fd_safe() method that can be called even if said fd is currently
> > held using fdget().
> 
> Yup, especially given Christian's view on the kernel task export here I think
> that's a more sensible approach.
> 
> But obviously I defer the sensible-ness of this to him as I am but an mm dev :)

Quick sketch of how this would look:

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index adde1e40cccd..6fb7175ff69b 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -64,7 +64,6 @@
 #include <linux/spinlock.h>
 #include <linux/ratelimit.h>
 #include <linux/syscalls.h>
-#include <linux/task_work.h>
 #include <linux/sizes.h>
 #include <linux/ktime.h>
 
@@ -1962,68 +1961,6 @@ static bool binder_validate_fixup(struct binder_proc *proc,
 	return (fixup_offset >= last_min_offset);
 }
 
-/**
- * struct binder_task_work_cb - for deferred close
- *
- * @twork:                callback_head for task work
- * @file:                 file to close
- *
- * Structure to pass task work to be handled after
- * returning from binder_ioctl() via task_work_add().
- */
-struct binder_task_work_cb {
-	struct callback_head twork;
-	struct file *file;
-};
-
-/**
- * binder_do_fd_close() - close list of file descriptors
- * @twork:	callback head for task work
- *
- * It is not safe to call ksys_close() during the binder_ioctl()
- * function if there is a chance that binder's own file descriptor
- * might be closed. This is to meet the requirements for using
- * fdget() (see comments for __fget_light()). Therefore use
- * task_work_add() to schedule the close operation once we have
- * returned from binder_ioctl(). This function is a callback
- * for that mechanism and does the actual ksys_close() on the
- * given file descriptor.
- */
-static void binder_do_fd_close(struct callback_head *twork)
-{
-	struct binder_task_work_cb *twcb = container_of(twork,
-			struct binder_task_work_cb, twork);
-
-	fput(twcb->file);
-	kfree(twcb);
-}
-
-/**
- * binder_deferred_fd_close() - schedule a close for the given file-descriptor
- * @fd:		file-descriptor to close
- *
- * See comments in binder_do_fd_close(). This function is used to schedule
- * a file-descriptor to be closed after returning from binder_ioctl().
- */
-static void binder_deferred_fd_close(int fd)
-{
-	struct binder_task_work_cb *twcb;
-
-	twcb = kzalloc(sizeof(*twcb), GFP_KERNEL);
-	if (!twcb)
-		return;
-	init_task_work(&twcb->twork, binder_do_fd_close);
-	twcb->file = file_close_fd(fd);
-	if (twcb->file) {
-		// pin it until binder_do_fd_close(); see comments there
-		get_file(twcb->file);
-		filp_close(twcb->file, current->files);
-		task_work_add(current, &twcb->twork, TWA_RESUME);
-	} else {
-		kfree(twcb);
-	}
-}
-
 static void binder_transaction_buffer_release(struct binder_proc *proc,
 					      struct binder_thread *thread,
 					      struct binder_buffer *buffer,
@@ -2183,7 +2120,10 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 						offset, sizeof(fd));
 				WARN_ON(err);
 				if (!err) {
-					binder_deferred_fd_close(fd);
+					/*
+					 * Intentionally ignore EBADF errors here.
+					 */
+					close_fd_safe(fd, GFP_KERNEL | __GFP_NOFAIL);
 					/*
 					 * Need to make sure the thread goes
 					 * back to userspace to complete the
diff --git a/fs/file.c b/fs/file.c
index 0a4f3bdb2dec..58e3825e846c 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -21,6 +21,7 @@
 #include <linux/rcupdate.h>
 #include <linux/close_range.h>
 #include <linux/file_ref.h>
+#include <linux/task_work.h>
 #include <net/sock.h>
 #include <linux/init_task.h>
 
@@ -1525,3 +1526,47 @@ int iterate_fd(struct files_struct *files, unsigned n,
 	return res;
 }
 EXPORT_SYMBOL(iterate_fd);
+
+struct close_fd_safe_task_work {
+	struct callback_head twork;
+	struct file *file;
+};
+
+static void close_fd_safe_callback(struct callback_head *twork)
+{
+	struct close_fd_safe_task_work *twcb = container_of(twork,
+			struct close_fd_safe_task_work, twork);
+
+	fput(twcb->file);
+	kfree(twcb);
+}
+
+/**
+ * close_fd_safe - close the given fd
+ * @fd: file descriptor to close
+ * @flags: gfp flags for allocation of task work
+ *
+ * This closes an fd. Unlike close_fd(), this may be used even if the fd is
+ * currently held with fdget().
+ *
+ * Returns: 0 or an error code
+ */
+int close_fd_safe(unsigned int fd, gfp_t flags)
+{
+	struct close_fd_safe_task_work *twcb;
+
+	twcb = kzalloc(sizeof(*twcb), flags);
+	if (!twcb)
+		return -ENOMEM;
+	init_task_work(&twcb->twork, close_fd_safe_callback);
+	twcb->file = file_close_fd(fd);
+	if (!twcb->file) {
+		kfree(twcb);
+		return -EBADF;
+	}
+
+	get_file(twcb->file);
+	filp_close(twcb->file, current->files);
+	task_work_add(current, &twcb->twork, TWA_RESUME);
+	return 0;
+}
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index c45306a9f007..1c99a56c0cdf 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -111,6 +111,7 @@ int iterate_fd(struct files_struct *, unsigned,
 		const void *);
 
 extern int close_fd(unsigned int fd);
+extern int close_fd_safe(unsigned int fd, gfp_t flags);
 extern struct file *file_close_fd(unsigned int fd);
 
 extern struct kmem_cache *files_cachep;

Powered by blists - more mailing lists