Message-ID: <9a037fdf-1a98-437f-8b80-7fdc53d5b0fa@lucifer.local>
Date: Thu, 5 Feb 2026 11:53:19 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Alice Ryhl <aliceryhl@...gle.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: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 :)

>
> > > Link: https://lore.kernel.org/all/20180730203633.GC12962@bombadil.infradead.org/ [1]
> > > Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> > > ---
> > >  fs/file.c          | 1 +
> > >  kernel/task_work.c | 1 +
> > >  2 files changed, 2 insertions(+)
> > >
> > > diff --git a/fs/file.c b/fs/file.c
> > > index 0a4f3bdb2dec6284a0c7b9687213137f2eecb250..0046d0034bf16270cdea7e30a86866ebea3a5a81 100644
> > > --- a/fs/file.c
> > > +++ b/fs/file.c
> > > @@ -881,6 +881,7 @@ struct file *file_close_fd(unsigned int fd)
> > >
> > >  	return file;
> > >  }
> > > +EXPORT_SYMBOL(file_close_fd);
> >
> > As a matter of policy we generally don't like to export without GPL like this
> > unless there's a _really_ good reason.
> >
> > Christian or Al may have a different viewpoint but generally this should be an
> > EXPORT_SYMBOL_GPL() and also - there has to be a _really_ good reason to export
> > it.
>
> Sorry I should just have done _GPL from the beginning. My mistake.

Thanks!

>
> Alice

Cheers, Lorenzo

Powered by blists - more mailing lists