* Re: [PATCH 3/5] mm: export zap_page_range_single and list_lru_add/del
From: Lorenzo Stoakes @ 2026-02-05 12:18 UTC (permalink / raw)
To: Alice Ryhl
Cc: Greg Kroah-Hartman, Carlos Llamas, Alexander Viro,
Christian Brauner, Jan Kara, Paul Moore, James Morris,
Serge E. Hallyn, Andrew Morton, Dave Chinner, Qi Zheng,
Roman Gushchin, Muchun Song, David Hildenbrand, Liam R. Howlett,
Vlastimil Babka, Mike Rapoport, Suren Baghdasaryan, Michal Hocko,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
kernel-team, linux-fsdevel, linux-kernel, linux-security-module,
linux-mm, rust-for-linux
In-Reply-To: <aYSH5KG36fVQFePL@google.com>
On Thu, Feb 05, 2026 at 12:07:00PM +0000, Alice Ryhl wrote:
> On Thu, Feb 05, 2026 at 11:29:04AM +0000, Lorenzo Stoakes wrote:
> > We either need a wrapper that eliminates this parameter (but then we're adding a
> > wrapper to this behaviour that is literally for one driver that is _temporarily_
> > being modularised which is weak justifiction), or use of a function that invokes
> > it that is currently exported.
>
> I have not talked with distros about it, but quite a few of them enable
> Binder because one or two applications want to use Binder to emulate
> Android. I imagine that even if Android itself goes back to built-in,
> distros would want it as a module so that you don't have to load it for
> every user, rather than for the few users that want to use waydroid or
> similar.
>
> A few examples:
> https://gitlab.archlinux.org/archlinux/packaging/packages/linux/-/blob/5711a17344ec7cfd90443374a30d5cd3e9a9439e/config#L10993
> https://salsa.debian.org/kernel-team/linux/-/blob/debian/latest/debian/config/arm64/config?ref_type=heads#L106
> https://gitlab.com/cki-project/kernel-ark/-/blob/os-build/redhat/configs/fedora/generic/x86/CONFIG_ANDROID_BINDER_IPC?ref_type=heads
I mean you should update the cover letter to make this clear and drop the whole
reference to things being temporary, this is a lot more strident than the cover
letter is.
In any case, that has nothing to do with whether or not we export internal
implementation details to a module.
Something being in-tree compiled gets to use actually far too many internal
interfaces that really should not have been exposed, we've been far too leniant
about that, and that's something I want to address (mm has mm/*.h internal-only
headers, not sure how we'll deal with that with rust though).
Sadly even with in-tree, every interface you make available leads to driver
abuse. So something compiled in-tree using X, Y or Z interface doesn't mean that
it's correct or even wise, and modularising forces you to rethink that.
folio_mkclean() is a great example, we were about to be able to make that
mm-internal then 2 more filesystems started using it oops :)
>
> Alice
Cheers, Lorenzo
^ permalink raw reply
* Re: [PATCH 3/5] mm: export zap_page_range_single and list_lru_add/del
From: Alice Ryhl @ 2026-02-05 12:19 UTC (permalink / raw)
To: David Hildenbrand (arm)
Cc: Lorenzo Stoakes, Greg Kroah-Hartman, Carlos Llamas,
Alexander Viro, Christian Brauner, Jan Kara, Paul Moore,
James Morris, Serge E. Hallyn, Andrew Morton, Dave Chinner,
Qi Zheng, Roman Gushchin, Muchun Song, Liam R. Howlett,
Vlastimil Babka, Mike Rapoport, Suren Baghdasaryan, Michal Hocko,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
kernel-team, linux-fsdevel, linux-kernel, linux-security-module,
linux-mm, rust-for-linux
In-Reply-To: <8856c839-1a94-4e4d-9ded-d3b1627cd2cc@kernel.org>
On Thu, Feb 05, 2026 at 01:13:57PM +0100, David Hildenbrand (arm) wrote:
> On 2/5/26 13:10, Lorenzo Stoakes wrote:
> > On Thu, Feb 05, 2026 at 11:58:00AM +0000, Alice Ryhl wrote:
> > > On Thu, Feb 05, 2026 at 12:43:03PM +0100, David Hildenbrand (arm) wrote:
> > > >
> > > > I don't expect anybody to set zap_details, but yeah, it could be abused.
> > > > It could be abused right now from anywhere else in the kernel
> > > > where we don't build as a module :)
> > > >
> > > > Apparently we export a similar function in rust where we just removed the last parameter.
> > >
> > > To clarify, said Rust function gets inlined into Rust Binder, so Rust
> > > Binder calls the zap_page_range_single() symbol directly.
> >
> > Presumably only for things compiled into the kernel right?
>
> Could Rust just use zap_vma_ptes() or does it want to zap things in VMAs
> that are not VM_PFNMAP?
The VMA is VM_MIXEDMAP, not VM_PFNMAP.
Alice
^ permalink raw reply
* Re: [PATCH 03/13] libfs: change simple_done_creating() to use end_creating()
From: Jeff Layton @ 2026-02-05 12:19 UTC (permalink / raw)
To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-4-neilb@ownmail.net>
On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> simple_done_creating() and end_creating() are identical.
> So change the former to use the latter. This further centralises
> unlocking of directories.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> fs/libfs.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index f1860dff86f2..db18b53fc189 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -2318,7 +2318,6 @@ EXPORT_SYMBOL(simple_start_creating);
> /* parent must have been held exclusive since simple_start_creating() */
> void simple_done_creating(struct dentry *child)
> {
> - inode_unlock(child->d_parent->d_inode);
> - dput(child);
> + end_creating(child);
> }
> EXPORT_SYMBOL(simple_done_creating);
nit: seems like it would be better to turn this into a static inline
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply
* Re: [PATCH 3/5] mm: export zap_page_range_single and list_lru_add/del
From: Miguel Ojeda @ 2026-02-05 12:24 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: David Hildenbrand (arm), Alice Ryhl, Greg Kroah-Hartman,
Carlos Llamas, Alexander Viro, Christian Brauner, Jan Kara,
Paul Moore, James Morris, Serge E. Hallyn, Andrew Morton,
Dave Chinner, Qi Zheng, Roman Gushchin, Muchun Song,
Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, kernel-team, linux-fsdevel,
linux-kernel, linux-security-module, linux-mm, rust-for-linux,
Christoph Hellwig
In-Reply-To: <ab63390c-9e75-4a45-9bf4-4ceb112ef07f@lucifer.local>
On Thu, Feb 5, 2026 at 12:58 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> What??
>
> Alice - can you confirm rust isn't exporting stuff that isn't explicitly marked
> EXPORT_SYMBOL*() for use by other rust modules?
>
> It's important we keep this in sync, otherwise rust is overriding kernel policy.
Currently, Rust GPL-exports every mangled symbol from the `kernel`
crate. To call something you would need to be a Rust caller (not C --
that is not supported at all, even if technically you could hack
something up) and the Rust API would then need to give you access to
it (i.e. you need to be able to pass the Rust language rules, e.g.
being public etc.).
In this case if we are talking about the `VmaRef` type, someone that
can get a reference to a value of that type could then call the
`zap_page_range_single` method. That in turns would try to call the C
one, but that one is not exported, right? So it should be fine.
In the future, for Rust, we may specify whether a particular crate
exports or not (and perhaps even allow to export non-GPL, but
originally it was decided to only export GPL stuff to be on the safe
side; and perhaps in certain namespaces etc.).
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH 3/5] mm: export zap_page_range_single and list_lru_add/del
From: Lorenzo Stoakes @ 2026-02-05 12:24 UTC (permalink / raw)
To: Alice Ryhl
Cc: David Hildenbrand (arm), Greg Kroah-Hartman, Carlos Llamas,
Alexander Viro, Christian Brauner, Jan Kara, Paul Moore,
James Morris, Serge E. Hallyn, Andrew Morton, Dave Chinner,
Qi Zheng, Roman Gushchin, Muchun Song, Liam R. Howlett,
Vlastimil Babka, Mike Rapoport, Suren Baghdasaryan, Michal Hocko,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
kernel-team, linux-fsdevel, linux-kernel, linux-security-module,
linux-mm, rust-for-linux
In-Reply-To: <aYSKyr7StGpGKNqW@google.com>
On Thu, Feb 05, 2026 at 12:19:22PM +0000, Alice Ryhl wrote:
> On Thu, Feb 05, 2026 at 01:13:57PM +0100, David Hildenbrand (arm) wrote:
> > On 2/5/26 13:10, Lorenzo Stoakes wrote:
> > > On Thu, Feb 05, 2026 at 11:58:00AM +0000, Alice Ryhl wrote:
> > > > On Thu, Feb 05, 2026 at 12:43:03PM +0100, David Hildenbrand (arm) wrote:
> > > > >
> > > > > I don't expect anybody to set zap_details, but yeah, it could be abused.
> > > > > It could be abused right now from anywhere else in the kernel
> > > > > where we don't build as a module :)
> > > > >
> > > > > Apparently we export a similar function in rust where we just removed the last parameter.
> > > >
> > > > To clarify, said Rust function gets inlined into Rust Binder, so Rust
> > > > Binder calls the zap_page_range_single() symbol directly.
> > >
> > > Presumably only for things compiled into the kernel right?
> >
> > Could Rust just use zap_vma_ptes() or does it want to zap things in VMAs
> > that are not VM_PFNMAP?
>
> The VMA is VM_MIXEDMAP, not VM_PFNMAP.
OK this smells like David's cleanup could extend it to allow for
VM_MIXEDMAP :) then we solve the export problem.
The two of these cause endless issues, it's really a mess...
>
> Alice
Cheers, Lorenzo
^ permalink raw reply
* Re: [PATCH 10/13] ovl: change ovl_create_real() to get a new lock when re-opening created file.
From: Amir Goldstein @ 2026-02-05 12:26 UTC (permalink / raw)
To: NeilBrown
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, John Johansen,
Paul Moore, James Morris, Serge E. Hallyn, Stephen Smalley,
linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-11-neilb@ownmail.net>
On Wed, Feb 4, 2026 at 6:09 AM NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
>
> When ovl_create_real() is used to create a file on the upper filesystem
> it needs to return the resulting dentry - positive and hashed.
> It is usually the case the that dentry passed to the create function
> (e.g. vfs_create()) will be suitable but this is not guaranteed. The
> filesystem may unhash that dentry forcing a repeat lookup next time the
> name is wanted.
>
> So ovl_create_real() must be (and is) aware of this and prepared to
> perform that lookup to get a hash positive dentry.
>
> This is currently done under that same directory lock that provided
> exclusion for the create. Proposed changes to locking will make this
> not possible - as the name, rather than the directory, will be locked.
> The new APIs provided for lookup and locking do not and cannot support
> this pattern.
>
> The lock isn't needed. ovl_create_real() can drop the lock and then get
> a new lock for the lookup - then check that the lookup returned the
> correct inode. In a well-behaved configuration where the upper
> filesystem is not being modified by a third party, this will always work
> reliably, and if there are separate modification it will fail cleanly.
>
> So change ovl_create_real() to drop the lock and call
> ovl_start_creating_upper() to find the correct dentry. Note that
> start_creating doesn't fail if the name already exists.
>
> This removes the only remaining use of ovl_lookup_upper, so it is
> removed.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> fs/overlayfs/dir.c | 24 ++++++++++++++++++------
> fs/overlayfs/overlayfs.h | 7 -------
> 2 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index ff3dbd1ca61f..ec08904d084d 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -219,21 +219,33 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> err = -EIO;
> } else if (d_unhashed(newdentry)) {
> struct dentry *d;
> + struct name_snapshot name;
> /*
> * Some filesystems (i.e. casefolded) may return an unhashed
> - * negative dentry from the ovl_lookup_upper() call before
> + * negative dentry from the ovl_start_creating_upper() call before
> * ovl_create_real().
According to the new locking rules, if the hashed dentry itself is
the synchronization object, is it going to be allowed to
filesystem to unhash the dentry while the dentry still in the
"creating" scope? It is hard for me to wrap my head around this.
Or do we need this here because some filesystems (casefold in
particular) are not going to support parallel creations?
> * In that case, lookup again after making the newdentry
> * positive, so ovl_create_upper() always returns a hashed
> * positive dentry.
> + * As we have to drop the lock before the lookup a race
> + * could result in a lookup failure. In that case we return
> + * an error.
> */
> - d = ovl_lookup_upper(ofs, newdentry->d_name.name, parent,
> - newdentry->d_name.len);
> - dput(newdentry);
> - if (IS_ERR_OR_NULL(d))
> + take_dentry_name_snapshot(&name, newdentry);
> + end_creating_keep(newdentry);
> + d = ovl_start_creating_upper(ofs, parent, &name.name);
> + release_dentry_name_snapshot(&name);
OK. not saying no to this (yet) but I have to admit that it is pretty
ugly that the callers of ovl_create_real() want to create a specific
stable name, which is could be passed in as const char *name
and yet we end up doing this weird dance here just to keep the name
from newdentry.
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH 3/5] mm: export zap_page_range_single and list_lru_add/del
From: Lorenzo Stoakes @ 2026-02-05 12:28 UTC (permalink / raw)
To: Miguel Ojeda
Cc: David Hildenbrand (arm), Alice Ryhl, Greg Kroah-Hartman,
Carlos Llamas, Alexander Viro, Christian Brauner, Jan Kara,
Paul Moore, James Morris, Serge E. Hallyn, Andrew Morton,
Dave Chinner, Qi Zheng, Roman Gushchin, Muchun Song,
Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, kernel-team, linux-fsdevel,
linux-kernel, linux-security-module, linux-mm, rust-for-linux,
Christoph Hellwig
In-Reply-To: <CANiq72=ybFtqsh18zkC3e1iyR-RoffcL_ZDr-fU7SjzJiFERHw@mail.gmail.com>
On Thu, Feb 05, 2026 at 01:24:09PM +0100, Miguel Ojeda wrote:
> On Thu, Feb 5, 2026 at 12:58 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > What??
> >
> > Alice - can you confirm rust isn't exporting stuff that isn't explicitly marked
> > EXPORT_SYMBOL*() for use by other rust modules?
> >
> > It's important we keep this in sync, otherwise rust is overriding kernel policy.
>
> Currently, Rust GPL-exports every mangled symbol from the `kernel`
> crate. To call something you would need to be a Rust caller (not C --
> that is not supported at all, even if technically you could hack
> something up) and the Rust API would then need to give you access to
> it (i.e. you need to be able to pass the Rust language rules, e.g.
> being public etc.).
>
> In this case if we are talking about the `VmaRef` type, someone that
> can get a reference to a value of that type could then call the
> `zap_page_range_single` method. That in turns would try to call the C
> one, but that one is not exported, right? So it should be fine.
>
OK cool. Thanks for the explanation.
> In the future, for Rust, we may specify whether a particular crate
> exports or not (and perhaps even allow to export non-GPL, but
> originally it was decided to only export GPL stuff to be on the safe
> side; and perhaps in certain namespaces etc.).
We'd definitely need to keep this in sync with C exports, maybe we can find
a nice way of doing/checking this.
Generally we're _very_ conservative about exports, and actually it'd be
nice if maybe rust only provided GPL ones. The GPL is a good thing :) But I
guess we can figure that out later.
>
> Cheers,
> Miguel
Cheers, Lorenzo
^ permalink raw reply
* Re: [PATCH 3/5] mm: export zap_page_range_single and list_lru_add/del
From: David Hildenbrand (Arm) @ 2026-02-05 12:30 UTC (permalink / raw)
To: Lorenzo Stoakes, Alice Ryhl
Cc: Greg Kroah-Hartman, Carlos Llamas, Alexander Viro,
Christian Brauner, Jan Kara, Paul Moore, James Morris,
Serge E. Hallyn, Andrew Morton, Dave Chinner, Qi Zheng,
Roman Gushchin, Muchun Song, Liam R. Howlett, Vlastimil Babka,
Mike Rapoport, Suren Baghdasaryan, Michal Hocko, Miguel Ojeda,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Danilo Krummrich, kernel-team,
linux-fsdevel, linux-kernel, linux-security-module, linux-mm,
rust-for-linux
In-Reply-To: <4c399644-97d3-40a3-a596-e4c93b713bdd@lucifer.local>
On 2/5/26 13:24, Lorenzo Stoakes wrote:
> On Thu, Feb 05, 2026 at 12:19:22PM +0000, Alice Ryhl wrote:
>> On Thu, Feb 05, 2026 at 01:13:57PM +0100, David Hildenbrand (arm) wrote:
>>>
>>> Could Rust just use zap_vma_ptes() or does it want to zap things in VMAs
>>> that are not VM_PFNMAP?
>>
>> The VMA is VM_MIXEDMAP, not VM_PFNMAP.
>
> OK this smells like David's cleanup could extend it to allow for
> VM_MIXEDMAP :) then we solve the export problem.
My thinking ... and while at it, gonna remove these functions to make
them a bit more ... consistent in naming.
--
Cheers,
David
^ permalink raw reply
* Re: [PATCH 01/13] fs/proc: Don't lock root inode when creating "self" and "thread-self"
From: Jeff Layton @ 2026-02-05 12:33 UTC (permalink / raw)
To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-2-neilb@ownmail.net>
On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> proc_setup_self() and proc_setup_thread_self() are only called from
> proc_fill_super() which is before the filesystem is "live". So there is
> no need to lock the root directory when adding "self" and "thread-self".
> This is clear from simple_fill_super() which provides similar
> functionality for other filesystems and does not lock anything.
>
> The locking is not harmful, except that it may be confusing to a reader.
> As part of a an effort to centralise all locking for directories for
> name-based operations (prior to changing some locking rules), it is
> simplest to remove the locking here.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> fs/proc/self.c | 3 ---
> fs/proc/thread_self.c | 3 ---
> 2 files changed, 6 deletions(-)
>
> diff --git a/fs/proc/self.c b/fs/proc/self.c
> index 62d2c0cfe35c..56adf1c68f7a 100644
> --- a/fs/proc/self.c
> +++ b/fs/proc/self.c
> @@ -35,11 +35,9 @@ unsigned self_inum __ro_after_init;
>
> int proc_setup_self(struct super_block *s)
> {
> - struct inode *root_inode = d_inode(s->s_root);
> struct dentry *self;
> int ret = -ENOMEM;
>
> - inode_lock(root_inode);
> self = d_alloc_name(s->s_root, "self");
> if (self) {
> struct inode *inode = new_inode(s);
> @@ -55,7 +53,6 @@ int proc_setup_self(struct super_block *s)
> }
> dput(self);
> }
> - inode_unlock(root_inode);
>
> if (ret)
> pr_err("proc_fill_super: can't allocate /proc/self\n");
> diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c
> index d6113dbe58e0..61ac62c3fd9f 100644
> --- a/fs/proc/thread_self.c
> +++ b/fs/proc/thread_self.c
> @@ -35,11 +35,9 @@ unsigned thread_self_inum __ro_after_init;
>
> int proc_setup_thread_self(struct super_block *s)
> {
> - struct inode *root_inode = d_inode(s->s_root);
> struct dentry *thread_self;
> int ret = -ENOMEM;
>
> - inode_lock(root_inode);
> thread_self = d_alloc_name(s->s_root, "thread-self");
> if (thread_self) {
> struct inode *inode = new_inode(s);
> @@ -55,7 +53,6 @@ int proc_setup_thread_self(struct super_block *s)
> }
> dput(thread_self);
> }
> - inode_unlock(root_inode);
>
> if (ret)
> pr_err("proc_fill_super: can't allocate /proc/thread-self\n");
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply
* Re: [PATCH 02/13] VFS: move the start_dirop() kerndoc comment to before start_dirop()
From: Jeff Layton @ 2026-02-05 12:33 UTC (permalink / raw)
To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-3-neilb@ownmail.net>
On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> This kerneldoc comment was always meant for start_dirop(), not for
> __start_dirop() which is a static function and doesn't need
> documentation.
>
> It was in the wrong place and was then incorrectly renamed (instead of
> moved) and useless "documentation" was added for "@state" was provided.
>
> This patch reverts the name, removes the mention of @state, and moves
> the comment to where it belongs.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> fs/namei.c | 27 +++++++++++++--------------
> 1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index b28ecb699f32..40af78ddfb1b 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2841,20 +2841,6 @@ static int filename_parentat(int dfd, struct filename *name,
> return __filename_parentat(dfd, name, flags, parent, last, type, NULL);
> }
>
> -/**
> - * __start_dirop - begin a create or remove dirop, performing locking and lookup
> - * @parent: the dentry of the parent in which the operation will occur
> - * @name: a qstr holding the name within that parent
> - * @lookup_flags: intent and other lookup flags.
> - * @state: task state bitmask
> - *
> - * The lookup is performed and necessary locks are taken so that, on success,
> - * the returned dentry can be operated on safely.
> - * The qstr must already have the hash value calculated.
> - *
> - * Returns: a locked dentry, or an error.
> - *
> - */
> static struct dentry *__start_dirop(struct dentry *parent, struct qstr *name,
> unsigned int lookup_flags,
> unsigned int state)
> @@ -2876,6 +2862,19 @@ static struct dentry *__start_dirop(struct dentry *parent, struct qstr *name,
> return dentry;
> }
>
> +/**
> + * start_dirop - begin a create or remove dirop, performing locking and lookup
> + * @parent: the dentry of the parent in which the operation will occur
> + * @name: a qstr holding the name within that parent
> + * @lookup_flags: intent and other lookup flags.
> + *
> + * The lookup is performed and necessary locks are taken so that, on success,
> + * the returned dentry can be operated on safely.
> + * The qstr must already have the hash value calculated.
> + *
> + * Returns: a locked dentry, or an error.
> + *
> + */
> struct dentry *start_dirop(struct dentry *parent, struct qstr *name,
> unsigned int lookup_flags)
> {
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply
* Re: [PATCH 05/13] selinux: Use simple_start_creating() / simple_done_creating()
From: Jeff Layton @ 2026-02-05 12:36 UTC (permalink / raw)
To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-6-neilb@ownmail.net>
On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> Instead of explicitly locking the parent and performing a lookup in
> selinux, use simple_start_creating(), and then use
> simple_done_creating() to unlock.
>
> This extends the region that the directory is locked for, and also
> performs a lookup.
> The lock extension is of no real consequence.
> The lookup uses simple_lookup() and so always succeeds. Thus when
> d_make_persistent() is called the dentry will already be hashed.
> d_make_persistent() handles this case.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> security/selinux/selinuxfs.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 896acad1f5f7..97e02cd5a9dc 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1930,15 +1930,16 @@ static const struct inode_operations swapover_dir_inode_operations = {
> static struct dentry *sel_make_swapover_dir(struct super_block *sb,
> unsigned long *ino)
> {
> - struct dentry *dentry = d_alloc_name(sb->s_root, ".swapover");
> + struct dentry *dentry;
> struct inode *inode;
>
> - if (!dentry)
> + inode = sel_make_inode(sb, S_IFDIR);
> + if (!inode)
> return ERR_PTR(-ENOMEM);
>
> - inode = sel_make_inode(sb, S_IFDIR);
> - if (!inode) {
> - dput(dentry);
> + dentry = simple_start_creating(sb->s_root, ".swapover");
> + if (!dentry) {
> + iput(inode);
> return ERR_PTR(-ENOMEM);
> }
>
> @@ -1946,11 +1947,9 @@ static struct dentry *sel_make_swapover_dir(struct super_block *sb,
> inode->i_ino = ++(*ino);
> /* directory inodes start off with i_nlink == 2 (for "." entry) */
> inc_nlink(inode);
> - inode_lock(sb->s_root->d_inode);
> d_make_persistent(dentry, inode);
> inc_nlink(sb->s_root->d_inode);
> - inode_unlock(sb->s_root->d_inode);
> - dput(dentry);
> + simple_done_creating(dentry);
> return dentry; // borrowed
> }
>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply
* Re: [PATCH 06/13] nfsd: switch purge_old() to use start_removing_noperm()
From: Jeff Layton @ 2026-02-05 12:36 UTC (permalink / raw)
To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-7-neilb@ownmail.net>
On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> Rather than explicit locking, use the start_removing_noperm() and
> end_removing() wrappers.
> This was not done with other start_removing changes due to conflicting
> in-flight patches.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> fs/nfsd/nfs4recover.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index 441dfbfe2d2b..52fbe723a3c8 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -351,16 +351,14 @@ purge_old(struct dentry *parent, char *cname, struct nfsd_net *nn)
> if (nfs4_has_reclaimed_state(name, nn))
> goto out_free;
>
> - inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
> - child = lookup_one(&nop_mnt_idmap, &QSTR(cname), parent);
> + child = start_removing_noperm(parent, &QSTR(cname));
> if (!IS_ERR(child)) {
> status = vfs_rmdir(&nop_mnt_idmap, d_inode(parent), child, NULL);
> if (status)
> printk("failed to remove client recovery directory %pd\n",
> child);
> - dput(child);
> }
> - inode_unlock(d_inode(parent));
> + end_removing(child);
>
> out_free:
> kfree(name.data);
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply
* Re: [PATCH 07/13] VFS: make lookup_one_qstr_excl() static.
From: Jeff Layton @ 2026-02-05 12:37 UTC (permalink / raw)
To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-8-neilb@ownmail.net>
On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> lookup_one_qstr_excl() is no longer used outside of namei.c, so
> make it static.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> Documentation/filesystems/porting.rst | 7 +++++++
> fs/namei.c | 5 ++---
> include/linux/namei.h | 3 ---
> 3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
> index ed3ac56e3c76..ed86c95d9d01 100644
> --- a/Documentation/filesystems/porting.rst
> +++ b/Documentation/filesystems/porting.rst
> @@ -1340,3 +1340,10 @@ The ->setlease() file_operation must now be explicitly set in order to provide
> support for leases. When set to NULL, the kernel will now return -EINVAL to
> attempts to set a lease. Filesystems that wish to use the kernel-internal lease
> implementation should set it to generic_setlease().
> +
> +---
> +
> +**mandatory**
> +
> +lookup_one_qstr_excl() is no longer exported - use start_creating() or
> +similar.
> diff --git a/fs/namei.c b/fs/namei.c
> index 40af78ddfb1b..307b4d0866b8 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1730,8 +1730,8 @@ static struct dentry *lookup_dcache(const struct qstr *name,
> * Will return -ENOENT if name isn't found and LOOKUP_CREATE wasn't passed.
> * Will return -EEXIST if name is found and LOOKUP_EXCL was passed.
> */
> -struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> - struct dentry *base, unsigned int flags)
> +static struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> + struct dentry *base, unsigned int flags)
> {
> struct dentry *dentry;
> struct dentry *old;
> @@ -1768,7 +1768,6 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> }
> return dentry;
> }
> -EXPORT_SYMBOL(lookup_one_qstr_excl);
>
> /**
> * lookup_fast - do fast lockless (but racy) lookup of a dentry
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 58600cf234bc..c7a7288cdd25 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -54,9 +54,6 @@ extern int path_pts(struct path *path);
>
> extern int user_path_at(int, const char __user *, unsigned, struct path *);
>
> -struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> - struct dentry *base,
> - unsigned int flags);
> extern int kern_path(const char *, unsigned, struct path *);
> struct dentry *kern_path_parent(const char *name, struct path *parent);
>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply
* Re: [PATCH 08/13] ovl: Simplify ovl_lookup_real_one()
From: Jeff Layton @ 2026-02-05 12:38 UTC (permalink / raw)
To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-9-neilb@ownmail.net>
On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> The primary purpose of this patch is to remove the locking from
> ovl_lookup_real_one() as part of centralising all locking of directories
> for name operations.
>
> The locking here isn't needed. By performing consistency tests after
> the lookup we can be sure that the result of the lookup was valid at
> least for a moment, which is all the original code promised.
>
> lookup_noperm_unlocked() is used for the lookup and it will take the
> lock if needed only where it is needed.
>
> Also:
> - don't take a reference to real->d_parent. The parent is
> only use for a pointer comparison, and no reference is needed for
> that.
> - Several "if" statements have a "goto" followed by "else" - the
> else isn't needed: the following statement can directly follow
> the "if" as a new statement
> - Use a consistent pattern of setting "err" before performing a test
> and possibly going to "fail".
> - remove the "out" label (now that we don't need to dput(parent) or
> unlock) and simply return from fail:.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> fs/overlayfs/export.c | 61 ++++++++++++++++++-------------------------
> 1 file changed, 26 insertions(+), 35 deletions(-)
>
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 83f80fdb1567..dcd28ffc4705 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -359,59 +359,50 @@ static struct dentry *ovl_lookup_real_one(struct dentry *connected,
> struct dentry *real,
> const struct ovl_layer *layer)
> {
> - struct inode *dir = d_inode(connected);
> - struct dentry *this, *parent = NULL;
> + struct dentry *this;
> struct name_snapshot name;
> int err;
>
> /*
> - * Lookup child overlay dentry by real name. The dir mutex protects us
> - * from racing with overlay rename. If the overlay dentry that is above
> - * real has already been moved to a parent that is not under the
> - * connected overlay dir, we return -ECHILD and restart the lookup of
> - * connected real path from the top.
> + * @connected is a directory in the overlay and @real is an object
> + * on @layer which is expected to be a child of @connected.
> + * The goal is to return a dentry from the overlay which corresponds
> + * to @real. This is done by looking up the name from @real in
> + * @connected and checking that the result meets expectations.
> + *
> + * Return %-ECHILD if the parent of @real no-longer corresponds to
> + * @connected, and %-ESTALE if the dentry found by lookup doesn't
> + * correspond to @real.
> */
> - inode_lock_nested(dir, I_MUTEX_PARENT);
> - err = -ECHILD;
> - parent = dget_parent(real);
> - if (ovl_dentry_real_at(connected, layer->idx) != parent)
> - goto fail;
>
> - /*
> - * We also need to take a snapshot of real dentry name to protect us
> - * from racing with underlying layer rename. In this case, we don't
> - * care about returning ESTALE, only from dereferencing a free name
> - * pointer because we hold no lock on the real dentry.
> - */
> take_dentry_name_snapshot(&name, real);
> - /*
> - * No idmap handling here: it's an internal lookup.
> - */
> - this = lookup_noperm(&name.name, connected);
> + this = lookup_noperm_unlocked(&name.name, connected);
> release_dentry_name_snapshot(&name);
> +
> + err = -ECHILD;
> + if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent)
> + goto fail;
> +
> err = PTR_ERR(this);
> - if (IS_ERR(this)) {
> + if (IS_ERR(this))
> goto fail;
> - } else if (!this || !this->d_inode) {
> - dput(this);
> - err = -ENOENT;
> +
> + err = -ENOENT;
> + if (!this || !this->d_inode)
> goto fail;
> - } else if (ovl_dentry_real_at(this, layer->idx) != real) {
> - dput(this);
> - err = -ESTALE;
> +
> + err = -ESTALE;
> + if (ovl_dentry_real_at(this, layer->idx) != real)
> goto fail;
> - }
>
> -out:
> - dput(parent);
> - inode_unlock(dir);
> return this;
>
> fail:
> pr_warn_ratelimited("failed to lookup one by real (%pd2, layer=%d, connected=%pd2, err=%i)\n",
> real, layer->idx, connected, err);
> - this = ERR_PTR(err);
> - goto out;
> + if (!IS_ERR(this))
> + dput(this);
> + return ERR_PTR(err);
> }
>
> static struct dentry *ovl_lookup_real(struct super_block *sb,
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply
* Re: [PATCH 09/13] cachefiles: change cachefiles_bury_object to use start_renaming_dentry()
From: Jeff Layton @ 2026-02-05 12:38 UTC (permalink / raw)
To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-10-neilb@ownmail.net>
On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> Rather then using lock_rename() and lookup_one() etc we can use
> the new start_renaming_dentry(). This is part of centralising dir
> locking and lookup so that locking rules can be changed.
>
> Some error check are removed as not necessary. Checks for rep being a
> non-dir or IS_DEADDIR and the check that ->graveyard is still a
> directory only provide slightly more informative errors and have been
> dropped.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> fs/cachefiles/namei.c | 76 ++++++++-----------------------------------
> 1 file changed, 14 insertions(+), 62 deletions(-)
>
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index e5ec90dccc27..3af42ec78411 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -270,7 +270,8 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
> struct dentry *rep,
> enum fscache_why_object_killed why)
> {
> - struct dentry *grave, *trap;
> + struct dentry *grave;
> + struct renamedata rd = {};
> struct path path, path_to_graveyard;
> char nbuffer[8 + 8 + 1];
> int ret;
> @@ -302,77 +303,36 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
> (uint32_t) ktime_get_real_seconds(),
> (uint32_t) atomic_inc_return(&cache->gravecounter));
>
> - /* do the multiway lock magic */
> - trap = lock_rename(cache->graveyard, dir);
> - if (IS_ERR(trap))
> - return PTR_ERR(trap);
> -
> - /* do some checks before getting the grave dentry */
> - if (rep->d_parent != dir || IS_DEADDIR(d_inode(rep))) {
> - /* the entry was probably culled when we dropped the parent dir
> - * lock */
> - unlock_rename(cache->graveyard, dir);
> - _leave(" = 0 [culled?]");
> - return 0;
> - }
> -
> - if (!d_can_lookup(cache->graveyard)) {
> - unlock_rename(cache->graveyard, dir);
> - cachefiles_io_error(cache, "Graveyard no longer a directory");
> - return -EIO;
> - }
> -
> - if (trap == rep) {
> - unlock_rename(cache->graveyard, dir);
> - cachefiles_io_error(cache, "May not make directory loop");
> + rd.mnt_idmap = &nop_mnt_idmap;
> + rd.old_parent = dir;
> + rd.new_parent = cache->graveyard;
> + rd.flags = 0;
> + ret = start_renaming_dentry(&rd, 0, rep, &QSTR(nbuffer));
> + if (ret) {
> + cachefiles_io_error(cache, "Cannot lock/lookup in graveyard");
> return -EIO;
> }
>
> if (d_mountpoint(rep)) {
> - unlock_rename(cache->graveyard, dir);
> + end_renaming(&rd);
> cachefiles_io_error(cache, "Mountpoint in cache");
> return -EIO;
> }
>
> - grave = lookup_one(&nop_mnt_idmap, &QSTR(nbuffer), cache->graveyard);
> - if (IS_ERR(grave)) {
> - unlock_rename(cache->graveyard, dir);
> - trace_cachefiles_vfs_error(object, d_inode(cache->graveyard),
> - PTR_ERR(grave),
> - cachefiles_trace_lookup_error);
> -
> - if (PTR_ERR(grave) == -ENOMEM) {
> - _leave(" = -ENOMEM");
> - return -ENOMEM;
> - }
> -
> - cachefiles_io_error(cache, "Lookup error %ld", PTR_ERR(grave));
> - return -EIO;
> - }
> -
> + grave = rd.new_dentry;
> if (d_is_positive(grave)) {
> - unlock_rename(cache->graveyard, dir);
> - dput(grave);
> + end_renaming(&rd);
> grave = NULL;
> cond_resched();
> goto try_again;
> }
>
> if (d_mountpoint(grave)) {
> - unlock_rename(cache->graveyard, dir);
> - dput(grave);
> + end_renaming(&rd);
> cachefiles_io_error(cache, "Mountpoint in graveyard");
> return -EIO;
> }
>
> - /* target should not be an ancestor of source */
> - if (trap == grave) {
> - unlock_rename(cache->graveyard, dir);
> - dput(grave);
> - cachefiles_io_error(cache, "May not make directory loop");
> - return -EIO;
> - }
> -
> /* attempt the rename */
> path.mnt = cache->mnt;
> path.dentry = dir;
> @@ -382,13 +342,6 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
> if (ret < 0) {
> cachefiles_io_error(cache, "Rename security error %d", ret);
> } else {
> - struct renamedata rd = {
> - .mnt_idmap = &nop_mnt_idmap,
> - .old_parent = dir,
> - .old_dentry = rep,
> - .new_parent = cache->graveyard,
> - .new_dentry = grave,
> - };
> trace_cachefiles_rename(object, d_inode(rep)->i_ino, why);
> ret = cachefiles_inject_read_error();
> if (ret == 0)
> @@ -402,8 +355,7 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
> }
>
> __cachefiles_unmark_inode_in_use(object, d_inode(rep));
> - unlock_rename(cache->graveyard, dir);
> - dput(grave);
> + end_renaming(&rd);
> _leave(" = 0");
> return 0;
> }
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply
* Re: [PATCH 11/13] ovl: use is_subdir() for testing if one thing is a subdir of another
From: Jeff Layton @ 2026-02-05 12:38 UTC (permalink / raw)
To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-12-neilb@ownmail.net>
On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> Rather than using lock_rename(), use the more obvious is_subdir() for
> ensuring that neither upper nor workdir contain the other.
> Also be explicit in the comment that the two directories cannot be the
> same.
>
> As this is a point-it-time sanity check and does not provide any
> on-going guarantees, the removal of locking does not introduce any
> interesting races.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> fs/overlayfs/super.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index ba9146f22a2c..2fd3e0aee50e 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -451,18 +451,13 @@ static int ovl_lower_dir(const char *name, const struct path *path,
> return 0;
> }
>
> -/* Workdir should not be subdir of upperdir and vice versa */
> +/*
> + * Workdir should not be subdir of upperdir and vice versa, and
> + * they should not be the same.
> + */
> static bool ovl_workdir_ok(struct dentry *workdir, struct dentry *upperdir)
> {
> - bool ok = false;
> -
> - if (workdir != upperdir) {
> - struct dentry *trap = lock_rename(workdir, upperdir);
> - if (!IS_ERR(trap))
> - unlock_rename(workdir, upperdir);
> - ok = (trap == NULL);
> - }
> - return ok;
> + return !is_subdir(workdir, upperdir) && !is_subdir(upperdir, workdir);
> }
>
> static int ovl_setup_trap(struct super_block *sb, struct dentry *dir,
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply
* Re: [PATCH 12/13] ovl: remove ovl_lock_rename_workdir()
From: Jeff Layton @ 2026-02-05 12:40 UTC (permalink / raw)
To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-13-neilb@ownmail.net>
On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> This function is unused.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> fs/overlayfs/overlayfs.h | 2 --
> fs/overlayfs/util.c | 25 -------------------------
> 2 files changed, 27 deletions(-)
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 4fb4750a83e4..3eedc2684c23 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -569,8 +569,6 @@ bool ovl_is_inuse(struct dentry *dentry);
> bool ovl_need_index(struct dentry *dentry);
> int ovl_nlink_start(struct dentry *dentry);
> void ovl_nlink_end(struct dentry *dentry);
> -int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
> - struct dentry *upperdir, struct dentry *upper);
> int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path,
> struct ovl_metacopy *data);
> int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d,
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 94986d11a166..810c8752b4f7 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1213,31 +1213,6 @@ void ovl_nlink_end(struct dentry *dentry)
> ovl_inode_unlock(inode);
> }
>
> -int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
> - struct dentry *upperdir, struct dentry *upper)
> -{
> - struct dentry *trap;
> -
> - /* Workdir should not be subdir of upperdir and vice versa */
> - trap = lock_rename(workdir, upperdir);
> - if (IS_ERR(trap))
> - goto err;
> - if (trap)
> - goto err_unlock;
> - if (work && (work->d_parent != workdir || d_unhashed(work)))
> - goto err_unlock;
> - if (upper && (upper->d_parent != upperdir || d_unhashed(upper)))
> - goto err_unlock;
> -
> - return 0;
> -
> -err_unlock:
> - unlock_rename(workdir, upperdir);
> -err:
> - pr_err("failed to lock workdir+upperdir\n");
> - return -EIO;
> -}
> -
> /*
> * err < 0, 0 if no metacopy xattr, metacopy data size if xattr found.
> * an empty xattr returns OVL_METACOPY_MIN_SIZE to distinguish from no xattr value.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply
* Re: [PATCH 13/13] VFS: unexport lock_rename(), lock_rename_child(), unlock_rename()
From: Jeff Layton @ 2026-02-05 12:41 UTC (permalink / raw)
To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-14-neilb@ownmail.net>
On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> These three function are now only used in namei.c, so they don't need to
> be exported.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> Documentation/filesystems/porting.rst | 7 +++++++
> fs/namei.c | 9 +++------
> include/linux/namei.h | 3 ---
> 3 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
> index ed86c95d9d01..5f7008172f14 100644
> --- a/Documentation/filesystems/porting.rst
> +++ b/Documentation/filesystems/porting.rst
> @@ -1347,3 +1347,10 @@ implementation should set it to generic_setlease().
>
> lookup_one_qstr_excl() is no longer exported - use start_creating() or
> similar.
> +---
> +
> +** mandatory**
> +
> +lock_rename(), lock_rename_child(), unlock_rename() are no
> +longer available. Use start_renaming() or similar.
> +
> diff --git a/fs/namei.c b/fs/namei.c
> index 307b4d0866b8..0bc82bf90adc 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3713,7 +3713,7 @@ static struct dentry *lock_two_directories(struct dentry *p1, struct dentry *p2)
> /*
> * p1 and p2 should be directories on the same fs.
> */
> -struct dentry *lock_rename(struct dentry *p1, struct dentry *p2)
> +static struct dentry *lock_rename(struct dentry *p1, struct dentry *p2)
> {
> if (p1 == p2) {
> inode_lock_nested(p1->d_inode, I_MUTEX_PARENT);
> @@ -3723,12 +3723,11 @@ struct dentry *lock_rename(struct dentry *p1, struct dentry *p2)
> mutex_lock(&p1->d_sb->s_vfs_rename_mutex);
> return lock_two_directories(p1, p2);
> }
> -EXPORT_SYMBOL(lock_rename);
>
> /*
> * c1 and p2 should be on the same fs.
> */
> -struct dentry *lock_rename_child(struct dentry *c1, struct dentry *p2)
> +static struct dentry *lock_rename_child(struct dentry *c1, struct dentry *p2)
> {
> if (READ_ONCE(c1->d_parent) == p2) {
> /*
> @@ -3765,9 +3764,8 @@ struct dentry *lock_rename_child(struct dentry *c1, struct dentry *p2)
> mutex_unlock(&c1->d_sb->s_vfs_rename_mutex);
> return NULL;
> }
> -EXPORT_SYMBOL(lock_rename_child);
>
> -void unlock_rename(struct dentry *p1, struct dentry *p2)
> +static void unlock_rename(struct dentry *p1, struct dentry *p2)
> {
> inode_unlock(p1->d_inode);
> if (p1 != p2) {
> @@ -3775,7 +3773,6 @@ void unlock_rename(struct dentry *p1, struct dentry *p2)
> mutex_unlock(&p1->d_sb->s_vfs_rename_mutex);
> }
> }
> -EXPORT_SYMBOL(unlock_rename);
>
> /**
> * __start_renaming - lookup and lock names for rename
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index c7a7288cdd25..2ad6dd9987b9 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -165,9 +165,6 @@ extern int follow_down_one(struct path *);
> extern int follow_down(struct path *path, unsigned int flags);
> extern int follow_up(struct path *);
>
> -extern struct dentry *lock_rename(struct dentry *, struct dentry *);
> -extern struct dentry *lock_rename_child(struct dentry *, struct dentry *);
> -extern void unlock_rename(struct dentry *, struct dentry *);
> int start_renaming(struct renamedata *rd, int lookup_flags,
> struct qstr *old_last, struct qstr *new_last);
> int start_renaming_dentry(struct renamedata *rd, int lookup_flags,
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply
* Re: [PATCH 04/13] Apparmor: Use simple_start_creating() / simple_done_creating()
From: Jeff Layton @ 2026-02-05 12:58 UTC (permalink / raw)
To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-5-neilb@ownmail.net>
On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> Instead of explicitly locking the parent and performing a look up in
> apparmor, use simple_start_creating(), and then simple_done_creating()
> to unlock and drop the dentry.
>
> This removes the need to check for an existing entry (as
> simple_start_creating() acts like an exclusive create and can return
> -EEXIST), simplifies error paths, and keeps dir locking code
> centralised.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> security/apparmor/apparmorfs.c | 38 ++++++++--------------------------
> 1 file changed, 9 insertions(+), 29 deletions(-)
>
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 907bd2667e28..7f78c36e6e50 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -282,32 +282,19 @@ static struct dentry *aafs_create(const char *name, umode_t mode,
>
> dir = d_inode(parent);
>
> - inode_lock(dir);
> - dentry = lookup_noperm(&QSTR(name), parent);
> + dentry = simple_start_creating(parent, name);
> if (IS_ERR(dentry)) {
> error = PTR_ERR(dentry);
> - goto fail_lock;
> - }
> -
> - if (d_really_is_positive(dentry)) {
> - error = -EEXIST;
> - goto fail_dentry;
> + goto fail;
> }
>
> error = __aafs_setup_d_inode(dir, dentry, mode, data, link, fops, iops);
> + simple_done_creating(dentry);
> if (error)
> - goto fail_dentry;
> - inode_unlock(dir);
> -
> - return dentry;
> -
> -fail_dentry:
> - dput(dentry);
> -
> -fail_lock:
> - inode_unlock(dir);
> + goto fail;
> + return 0;
As KTR points out, this should be "return NULL;"
> +fail:
> simple_release_fs(&aafs_mnt, &aafs_count);
> -
> return ERR_PTR(error);
> }
>
> @@ -2572,8 +2559,7 @@ static int aa_mk_null_file(struct dentry *parent)
> if (error)
> return error;
>
> - inode_lock(d_inode(parent));
> - dentry = lookup_noperm(&QSTR(NULL_FILE_NAME), parent);
> + dentry = simple_start_creating(parent, NULL_FILE_NAME);
> if (IS_ERR(dentry)) {
> error = PTR_ERR(dentry);
> goto out;
> @@ -2581,7 +2567,7 @@ static int aa_mk_null_file(struct dentry *parent)
> inode = new_inode(parent->d_inode->i_sb);
> if (!inode) {
> error = -ENOMEM;
> - goto out1;
> + goto out;
> }
>
> inode->i_ino = get_next_ino();
> @@ -2593,18 +2579,12 @@ static int aa_mk_null_file(struct dentry *parent)
> aa_null.dentry = dget(dentry);
> aa_null.mnt = mntget(mount);
>
> - error = 0;
> -
> -out1:
> - dput(dentry);
> out:
> - inode_unlock(d_inode(parent));
> + simple_done_creating(dentry);
> simple_release_fs(&mount, &count);
> return error;
> }
>
> -
> -
> static const char *policy_get_link(struct dentry *dentry,
> struct inode *inode,
> struct delayed_call *done)
Assuming you fix the minor problem above.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply
* Re: [PATCH 12/13] ovl: remove ovl_lock_rename_workdir()
From: Jeff Layton @ 2026-02-05 12:58 UTC (permalink / raw)
To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley
Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-13-neilb@ownmail.net>
On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> This function is unused.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
> fs/overlayfs/overlayfs.h | 2 --
> fs/overlayfs/util.c | 25 -------------------------
> 2 files changed, 27 deletions(-)
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 4fb4750a83e4..3eedc2684c23 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -569,8 +569,6 @@ bool ovl_is_inuse(struct dentry *dentry);
> bool ovl_need_index(struct dentry *dentry);
> int ovl_nlink_start(struct dentry *dentry);
> void ovl_nlink_end(struct dentry *dentry);
> -int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
> - struct dentry *upperdir, struct dentry *upper);
> int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path,
> struct ovl_metacopy *data);
> int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d,
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 94986d11a166..810c8752b4f7 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1213,31 +1213,6 @@ void ovl_nlink_end(struct dentry *dentry)
> ovl_inode_unlock(inode);
> }
>
> -int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
> - struct dentry *upperdir, struct dentry *upper)
> -{
> - struct dentry *trap;
> -
> - /* Workdir should not be subdir of upperdir and vice versa */
> - trap = lock_rename(workdir, upperdir);
> - if (IS_ERR(trap))
> - goto err;
> - if (trap)
> - goto err_unlock;
> - if (work && (work->d_parent != workdir || d_unhashed(work)))
> - goto err_unlock;
> - if (upper && (upper->d_parent != upperdir || d_unhashed(upper)))
> - goto err_unlock;
> -
> - return 0;
> -
> -err_unlock:
> - unlock_rename(workdir, upperdir);
> -err:
> - pr_err("failed to lock workdir+upperdir\n");
> - return -EIO;
> -}
> -
> /*
> * err < 0, 0 if no metacopy xattr, metacopy data size if xattr found.
> * an empty xattr returns OVL_METACOPY_MIN_SIZE to distinguish from no xattr value.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply
* Re: [PATCH 08/13] ovl: Simplify ovl_lookup_real_one()
From: Amir Goldstein @ 2026-02-05 13:04 UTC (permalink / raw)
To: Jeff Layton
Cc: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Miklos Szeredi, John Johansen, Paul Moore,
James Morris, Serge E. Hallyn, Stephen Smalley, linux-kernel,
netfs, linux-fsdevel, linux-nfs, linux-unionfs, apparmor,
linux-security-module, selinux
In-Reply-To: <5d273a008fc51a2fded785efbe30e5bd2a89b985.camel@kernel.org>
On Thu, Feb 5, 2026 at 1:38 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> >
> > The primary purpose of this patch is to remove the locking from
> > ovl_lookup_real_one() as part of centralising all locking of directories
> > for name operations.
> >
> > The locking here isn't needed. By performing consistency tests after
> > the lookup we can be sure that the result of the lookup was valid at
> > least for a moment, which is all the original code promised.
> >
> > lookup_noperm_unlocked() is used for the lookup and it will take the
> > lock if needed only where it is needed.
> >
> > Also:
> > - don't take a reference to real->d_parent. The parent is
> > only use for a pointer comparison, and no reference is needed for
> > that.
> > - Several "if" statements have a "goto" followed by "else" - the
> > else isn't needed: the following statement can directly follow
> > the "if" as a new statement
> > - Use a consistent pattern of setting "err" before performing a test
> > and possibly going to "fail".
> > - remove the "out" label (now that we don't need to dput(parent) or
> > unlock) and simply return from fail:.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> > fs/overlayfs/export.c | 61 ++++++++++++++++++-------------------------
> > 1 file changed, 26 insertions(+), 35 deletions(-)
> >
> > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > index 83f80fdb1567..dcd28ffc4705 100644
> > --- a/fs/overlayfs/export.c
> > +++ b/fs/overlayfs/export.c
> > @@ -359,59 +359,50 @@ static struct dentry *ovl_lookup_real_one(struct dentry *connected,
> > struct dentry *real,
> > const struct ovl_layer *layer)
> > {
> > - struct inode *dir = d_inode(connected);
> > - struct dentry *this, *parent = NULL;
> > + struct dentry *this;
> > struct name_snapshot name;
> > int err;
> >
> > /*
> > - * Lookup child overlay dentry by real name. The dir mutex protects us
> > - * from racing with overlay rename. If the overlay dentry that is above
> > - * real has already been moved to a parent that is not under the
> > - * connected overlay dir, we return -ECHILD and restart the lookup of
> > - * connected real path from the top.
> > + * @connected is a directory in the overlay and @real is an object
> > + * on @layer which is expected to be a child of @connected.
> > + * The goal is to return a dentry from the overlay which corresponds
As the header comment already says:
"...return a connected overlay dentry whose real dentry is @real"
The wording "corresponds to @real" reduces clarity IMO.
> > + * to @real. This is done by looking up the name from @real in
> > + * @connected and checking that the result meets expectations.
> > + *
> > + * Return %-ECHILD if the parent of @real no-longer corresponds to
> > + * @connected, and %-ESTALE if the dentry found by lookup doesn't
> > + * correspond to @real.
> > */
I dislike kernel-doc inside code comments.
I think this is actively discouraged and I haven't found a single example
of this style in fs code.
If you want to keep this format, please lift the comment to function
header comment - it is anyway a very generic comment that explains the
function in general.
> > - inode_lock_nested(dir, I_MUTEX_PARENT);
> > - err = -ECHILD;
> > - parent = dget_parent(real);
> > - if (ovl_dentry_real_at(connected, layer->idx) != parent)
> > - goto fail;
> >
> > - /*
> > - * We also need to take a snapshot of real dentry name to protect us
> > - * from racing with underlying layer rename. In this case, we don't
> > - * care about returning ESTALE, only from dereferencing a free name
> > - * pointer because we hold no lock on the real dentry.
> > - */
> > take_dentry_name_snapshot(&name, real);
> > - /*
> > - * No idmap handling here: it's an internal lookup.
> > - */
> > - this = lookup_noperm(&name.name, connected);
> > + this = lookup_noperm_unlocked(&name.name, connected);
> > release_dentry_name_snapshot(&name);
> > +
> > + err = -ECHILD;
> > + if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent)
> > + goto fail;
> > +
> > err = PTR_ERR(this);
> > - if (IS_ERR(this)) {
> > + if (IS_ERR(this))
> > goto fail;
> > - } else if (!this || !this->d_inode) {
> > - dput(this);
> > - err = -ENOENT;
> > +
> > + err = -ENOENT;
> > + if (!this || !this->d_inode)
> > goto fail;
> > - } else if (ovl_dentry_real_at(this, layer->idx) != real) {
> > - dput(this);
> > - err = -ESTALE;
> > +
> > + err = -ESTALE;
> > + if (ovl_dentry_real_at(this, layer->idx) != real)
> > goto fail;
> > - }
> >
> > -out:
> > - dput(parent);
> > - inode_unlock(dir);
> > return this;
> >
> > fail:
> > pr_warn_ratelimited("failed to lookup one by real (%pd2, layer=%d, connected=%pd2, err=%i)\n",
> > real, layer->idx, connected, err);
> > - this = ERR_PTR(err);
> > - goto out;
> > + if (!IS_ERR(this))
> > + dput(this);
> > + return ERR_PTR(err);
> > }
> >
> > static struct dentry *ovl_lookup_real(struct super_block *sb,
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
Otherwise, it looks fine.
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH] man/man7/kernel_lockdown.7: remove Secure Boot untruth
From: Alejandro Colomar @ 2026-02-05 13:10 UTC (permalink / raw)
To: Xiu Jianfeng
Cc: Alyssa Ross, Heinrich Schuchardt, David Howells,
Nicolas Bouchinet, linux-security-module, linux-man
In-Reply-To: <aa62e24c-537e-4141-9507-37cd0af19dfc@huawei.com>
[-- Attachment #1: Type: text/plain, Size: 1688 bytes --]
Hi Xiu,
On 2026-02-05T19:48:02+0800, Xiu Jianfeng wrote:
> On 2/4/2026 3:50 AM, Alyssa Ross wrote:
> > This is true for Fedora, where this page was sourced from, but I don't
> > believe it has ever been true for the mainline kernel, because Linus
> > rejected it.
>
> Yeah, I also found this issue not long ago, but I haven't had time to submit
> a fix patch yet.
>
> >
> > Link: https://bbs.archlinux.org/viewtopic.php?pid=2088704#p2088704
> > Link: https://lore.kernel.org/lkml/CA+55aFzYbpRAdma0PvqE+9ygySuKzNKByqOzzMufBoovXVnfPw@mail.gmail.com/
> > Fixes: bb509e6fc ("kernel_lockdown.7: New page documenting the Kernel Lockdown feature")
> > Signed-off-by: Alyssa Ross <hi@alyssa.is>
>
> I am not sure if appropriate to add my ACK here, if needed, feel free to
> add:
>
> Acked-by: Xiu Jianfeng <xiujianfeng@huawei.com>
It's appropriate. Thanks!
Have a lovely day!
Alex
>
> > ---
> > man/man7/kernel_lockdown.7 | 3 ---
> > 1 file changed, 3 deletions(-)
> >
> > diff --git a/man/man7/kernel_lockdown.7 b/man/man7/kernel_lockdown.7
> > index 5090484ea..5986c8f01 100644
> > --- a/man/man7/kernel_lockdown.7
> > +++ b/man/man7/kernel_lockdown.7
> > @@ -23,9 +23,6 @@ Lockdown: X: Y is restricted, see man kernel_lockdown.7
> > .in
> > .P
> > where X indicates the process name and Y indicates what is restricted.
> > -.P
> > -On an EFI-enabled x86 or arm64 machine, lockdown will be automatically enabled
> > -if the system boots in EFI Secure Boot mode.
> > .\"
> > .SS Coverage
> > When lockdown is in effect, a number of features are disabled or have their
>
>
--
<https://www.alejandro-colomar.es>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 5/5] rust_binder: mark ANDROID_BINDER_IPC_RUST tristate
From: Gary Guo @ 2026-02-05 13:21 UTC (permalink / raw)
To: Alice Ryhl, Greg Kroah-Hartman, Carlos Llamas
Cc: Alexander Viro, Christian Brauner, Jan Kara, Paul Moore,
James Morris, Serge E. Hallyn, Andrew Morton, Dave Chinner,
Qi Zheng, Roman Gushchin, Muchun Song, David Hildenbrand,
Lorenzo Stoakes, Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, kernel-team, linux-fsdevel,
linux-kernel, linux-security-module, linux-mm, rust-for-linux
In-Reply-To: <20260205-binder-tristate-v1-5-dfc947c35d35@google.com>
On Thu Feb 5, 2026 at 10:51 AM GMT, Alice Ryhl wrote:
> Currently Binder only builds as built-in module, but in downstream
> Android branches we update the build system to make Rust Binder
> buildable as a module. The same situation applies to distros, as there
> are many distros that enable Binder for support of apps such as
> waydroid, which would benefit from the ability to build Binder as a
> module.
>
> Note that although the situation in Android may be temporary - once we
> no longer have a C implementation, it makes sense for Rust Binder to be
> built-in. But that will both take a while, and in any case, distros
> enabling Binder will benefit from it being a module even if Android goes
> back to built-in.
>
> This doesn't mark C Binder buildable as a module. That would require
> more intrusive Makefile changes as it's built from multiple objects, and
> I'm not sure there's any way to produce a file called 'binder.ko'
> containing all of those objects linked together without renaming
> 'binder.c', as right now there will be naming conflicts between the
> object built from binder.c, and the object that results from linking
> binder.o,binderfs.o,binder_alloc.o and so on together. (As an aside,
> this issue is why the Rust Binder entry-point is called
> rust_binder_main.rs instead of just rust_binder.rs)
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> drivers/android/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
> index e2e402c9d1759c81591473ad02ab7ad011bc61d0..3c1755e53195b0160d0ed244f078eed96e16272c 100644
> --- a/drivers/android/Kconfig
> +++ b/drivers/android/Kconfig
> @@ -15,7 +15,7 @@ config ANDROID_BINDER_IPC
> between said processes.
>
> config ANDROID_BINDER_IPC_RUST
> - bool "Rust version of Android Binder IPC Driver"
> + tristate "Rust version of Android Binder IPC Driver"
> depends on RUST && MMU && !ANDROID_BINDER_IPC
> help
> This enables the Rust implementation of the Binder driver.
Hi Alice,
AFAIK Rust binder doesn't specifically handle module unloading, so global
statics (e.g. CONTEXTS doesn't get dropped).
If we're going to build Binder as module, we need to ensure that we have the
mechanism in the module macro to prevent unloading of Binder.
Best,
Gary
^ permalink raw reply
* Re: [PATCH v14 1/9] rust: types: Add Ownable/Owned types
From: Gary Guo @ 2026-02-05 13:29 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Greg Kroah-Hartman, Dave Ertman, Ira Weiny,
Leon Romanovsky, Paul Moore, Serge Hallyn, Rafael J. Wysocki,
David Airlie, Simona Vetter, Alexander Viro, Christian Brauner,
Jan Kara, Igor Korotin, Daniel Almeida, Lorenzo Stoakes,
Liam R. Howlett, Viresh Kumar, Nishanth Menon, Stephen Boyd,
Bjorn Helgaas, Krzysztof Wilczyński
Cc: linux-kernel, rust-for-linux, linux-block, linux-security-module,
dri-devel, linux-fsdevel, linux-mm, linux-pm, linux-pci,
Asahi Lina
In-Reply-To: <20260204-unique-ref-v14-1-17cb29ebacbb@kernel.org>
On Wed Feb 4, 2026 at 11:56 AM GMT, Andreas Hindborg wrote:
> From: Asahi Lina <lina+kernel@asahilina.net>
>
> By analogy to `AlwaysRefCounted` and `ARef`, an `Ownable` type is a
> (typically C FFI) type that *may* be owned by Rust, but need not be. Unlike
> `AlwaysRefCounted`, this mechanism expects the reference to be unique
> within Rust, and does not allow cloning.
>
> Conceptually, this is similar to a `KBox<T>`, except that it delegates
> resource management to the `T` instead of using a generic allocator.
>
> This change is a derived work based on work by Asahi Lina
> <lina+kernel@asahilina.net> [1] and Oliver Mangold <oliver.mangold@pm.me>.
>
> Link: https://lore.kernel.org/rust-for-linux/20250202-rust-page-v1-1-e3170d7fe55e@asahilina.net/ [1]
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/lib.rs | 1 +
> rust/kernel/owned.rs | 196 +++++++++++++++++++++++++++++++++++++++++++++++
> rust/kernel/sync/aref.rs | 5 ++
> rust/kernel/types.rs | 11 ++-
> 4 files changed, 212 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index f812cf1200428..96a3fadc3377a 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -119,6 +119,7 @@
> pub mod of;
> #[cfg(CONFIG_PM_OPP)]
> pub mod opp;
> +pub mod owned;
> pub mod page;
> #[cfg(CONFIG_PCI)]
> pub mod pci;
> diff --git a/rust/kernel/owned.rs b/rust/kernel/owned.rs
> new file mode 100644
> index 0000000000000..fe30580331df9
> --- /dev/null
> +++ b/rust/kernel/owned.rs
> <snip>
> +
> + /// Get a pinned mutable reference to the data owned by this `Owned<T>`.
> + pub fn get_pin_mut(&mut self) -> Pin<&mut T> {
> + // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
> + // return a mutable reference to it.
> + let unpinned = unsafe { self.ptr.as_mut() };
> +
> + // SAFETY: We never hand out unpinned mutable references to the data in
> + // `Self`, unless the contained type is `Unpin`.
> + unsafe { Pin::new_unchecked(unpinned) }
> + }
Probably should be name `as_pin_mut` instead.
With name changed and SOB fixed:
Reviewed-by: Gary Guo <gary@garyguo.net>
Best,
Gary
> +}
> +
> +// SAFETY: It is safe to send an [`Owned<T>`] to another thread when the underlying `T` is [`Send`],
> +// because of the ownership invariant. Sending an [`Owned<T>`] is equivalent to sending the `T`.
> +unsafe impl<T: Ownable + Send> Send for Owned<T> {}
> +
> +// SAFETY: It is safe to send [`&Owned<T>`] to another thread when the underlying `T` is [`Sync`],
> +// because of the ownership invariant. Sending an [`&Owned<T>`] is equivalent to sending the `&T`.
> +unsafe impl<T: Ownable + Sync> Sync for Owned<T> {}
> +
^ permalink raw reply
* Re: [PATCH v14 2/9] rust: rename `AlwaysRefCounted` to `RefCounted`.
From: Gary Guo @ 2026-02-05 13:31 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Greg Kroah-Hartman, Dave Ertman, Ira Weiny,
Leon Romanovsky, Paul Moore, Serge Hallyn, Rafael J. Wysocki,
David Airlie, Simona Vetter, Alexander Viro, Christian Brauner,
Jan Kara, Igor Korotin, Daniel Almeida, Lorenzo Stoakes,
Liam R. Howlett, Viresh Kumar, Nishanth Menon, Stephen Boyd,
Bjorn Helgaas, Krzysztof Wilczyński
Cc: linux-kernel, rust-for-linux, linux-block, linux-security-module,
dri-devel, linux-fsdevel, linux-mm, linux-pm, linux-pci,
Oliver Mangold
In-Reply-To: <20260204-unique-ref-v14-2-17cb29ebacbb@kernel.org>
On Wed Feb 4, 2026 at 11:56 AM GMT, Andreas Hindborg wrote:
> From: Oliver Mangold <oliver.mangold@pm.me>
>
> There are types where it may both be reference counted in some cases and
> owned in others. In such cases, obtaining `ARef<T>` from `&T` would be
> unsound as it allows creation of `ARef<T>` copy from `&Owned<T>`.
>
> Therefore, we split `AlwaysRefCounted` into `RefCounted` (which `ARef<T>`
> would require) and a marker trait to indicate that the type is always
> reference counted (and not `Ownable`) so the `&T` -> `ARef<T>` conversion
> is possible.
>
> - Rename `AlwaysRefCounted` to `RefCounted`.
> - Add a new unsafe trait `AlwaysRefCounted`.
> - Implement the new trait `AlwaysRefCounted` for the newly renamed
> `RefCounted` implementations. This leaves functionality of existing
> implementers of `AlwaysRefCounted` intact.
>
> Original patch by Oliver Mangold <oliver.mangold@pm.me> [1].
>
> Link: https://lore.kernel.org/r/20251117-unique-ref-v13-2-b5b243df1250@pm.me [1]
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
I think you also need to update the `AlwaysRefCounted` reference mentioned in
the `Owned` patch too? (Or perhaps this patch should be moved before `Owned`
instead?)
With that fixed:
Reviewed-by: Gary Guo <gary@garyguo.net>
> ---
> rust/kernel/auxiliary.rs | 7 +++++-
> rust/kernel/block/mq/request.rs | 15 +++++++------
> rust/kernel/cred.rs | 13 ++++++++++--
> rust/kernel/device.rs | 10 ++++++---
> rust/kernel/device/property.rs | 7 +++++-
> rust/kernel/drm/device.rs | 10 ++++++---
> rust/kernel/drm/gem/mod.rs | 8 ++++---
> rust/kernel/fs/file.rs | 16 ++++++++++----
> rust/kernel/i2c.rs | 16 +++++++++-----
> rust/kernel/mm.rs | 15 +++++++++----
> rust/kernel/mm/mmput_async.rs | 9 ++++++--
> rust/kernel/opp.rs | 10 ++++++---
> rust/kernel/owned.rs | 2 +-
> rust/kernel/pci.rs | 10 ++++++++-
> rust/kernel/pid_namespace.rs | 12 +++++++++--
> rust/kernel/platform.rs | 7 +++++-
> rust/kernel/sync/aref.rs | 47 ++++++++++++++++++++++++++---------------
> rust/kernel/task.rs | 10 ++++++---
> rust/kernel/types.rs | 3 ++-
> 19 files changed, 164 insertions(+), 63 deletions(-)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox