[PATCH] ovl: fix regression caused for lookup helpers API changes

[PATCH] ovl: fix regression caused for lookup helpers API changes

[Amir Goldstein]

The lookup helpers API was changed by merge of vfs-6.16-rc1.async.dir to
pass a non-const qstr pointer argument to lookup_one*() helpers.

All of the callers of this API were changed to pass a pointer to temp
copy of qstr, except overlays that was passing a const pointer to
dentry->d_name that was changed to pass a non-const copy instead
when doing a lookup in lower layer which is not the fs of said dentry.

This wrong use of the API caused a regression in fstest overlay/012.

Fix the regression by making a non-const copy of dentry->d_name prior
to calling the lookup API, but the API should be fixed to not allow this
class of bugs.

Cc: NeilBrown <neilb@suse.de>
Fixes: 5741909697a3 ("VFS: improve interface for lookup_one functions")
Fixes: 390e34bc1490 ("VFS: change lookup_one_common and lookup_noperm_common to take a qstr")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Christian,

Please fast track this fix to Linus before 6.16-rc1 and
consider how you would like to fix the lookup_one*() API.

This change is independent of the ovl changes staged on overlayfs-next,
which I am assuming Miklos is going to send to Linus.

Thanks,
Amir.

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index bf722daf19a9..00979555223d 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1371,7 +1371,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 bool ovl_lower_positive(struct dentry *dentry)
 {
 	struct ovl_entry *poe = OVL_E(dentry->d_parent);
-	struct qstr *name = &dentry->d_name;
+	const struct qstr *name = &dentry->d_name;
 	const struct cred *old_cred;
 	unsigned int i;
 	bool positive = false;
@@ -1394,9 +1394,16 @@ bool ovl_lower_positive(struct dentry *dentry)
 		struct dentry *this;
 		struct ovl_path *parentpath = &ovl_lowerstack(poe)[i];
 
+		/*
+		 * We need to make a non-const copy of dentry->d_name,
+		 * becuase lookup_one_positive_unlocked() will hash name
+		 * with parentpath base, which is on another (lwoer fs).
+		 * TODO: the lookup_* API should be changed not to allow this.
+		 */
 		this = lookup_one_positive_unlocked(
 				mnt_idmap(parentpath->layer->mnt),
-				name, parentpath->dentry);
+				&QSTR_LEN(name->name, name->len),
+				parentpath->dentry);
 		if (IS_ERR(this)) {
 			switch (PTR_ERR(this)) {
 			case -ENOENT:
-- 
2.34.1

Re: [PATCH] ovl: fix regression caused for lookup helpers API changes

[Christian Brauner]

On Thu, 05 Jun 2025 12:15:30 +0200, Amir Goldstein wrote:
> The lookup helpers API was changed by merge of vfs-6.16-rc1.async.dir to
> pass a non-const qstr pointer argument to lookup_one*() helpers.
> 
> All of the callers of this API were changed to pass a pointer to temp
> copy of qstr, except overlays that was passing a const pointer to
> dentry->d_name that was changed to pass a non-const copy instead
> when doing a lookup in lower layer which is not the fs of said dentry.
> 
> [...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] ovl: fix regression caused for lookup helpers API changes
      https://git.kernel.org/vfs/vfs/c/339740d5d1cd

Re: [PATCH] ovl: fix regression caused for lookup helpers API changes

[Amir Goldstein]

On Thu, Jun 5, 2025 at 1:00 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Thu, 05 Jun 2025 12:15:30 +0200, Amir Goldstein wrote:
> > The lookup helpers API was changed by merge of vfs-6.16-rc1.async.dir to
> > pass a non-const qstr pointer argument to lookup_one*() helpers.
> >
> > All of the callers of this API were changed to pass a pointer to temp
> > copy of qstr, except overlays that was passing a const pointer to
> > dentry->d_name that was changed to pass a non-const copy instead
> > when doing a lookup in lower layer which is not the fs of said dentry.
> >
> > [...]
>
> Applied to the vfs.fixes branch of the vfs/vfs.git tree.
> Patches in the vfs.fixes branch should appear in linux-next soon.
>

Could you fix the grammatical mistake in my commit title:

s/caused for/caused by/

Thanks,
Amir.

Re: [PATCH] ovl: fix regression caused for lookup helpers API changes

[Christian Brauner]

On Thu, Jun 05, 2025 at 01:14:20PM +0200, Amir Goldstein wrote:
> On Thu, Jun 5, 2025 at 1:00 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Thu, 05 Jun 2025 12:15:30 +0200, Amir Goldstein wrote:
> > > The lookup helpers API was changed by merge of vfs-6.16-rc1.async.dir to
> > > pass a non-const qstr pointer argument to lookup_one*() helpers.
> > >
> > > All of the callers of this API were changed to pass a pointer to temp
> > > copy of qstr, except overlays that was passing a const pointer to
> > > dentry->d_name that was changed to pass a non-const copy instead
> > > when doing a lookup in lower layer which is not the fs of said dentry.
> > >
> > > [...]
> >
> > Applied to the vfs.fixes branch of the vfs/vfs.git tree.
> > Patches in the vfs.fixes branch should appear in linux-next soon.
> >
> 
> Could you fix the grammatical mistake in my commit title:
> 
> s/caused for/caused by/

Already done including the ones in the comments to your fix. ;)

Re: [PATCH] ovl: fix regression caused for lookup helpers API changes

[Amir Goldstein]

On Thu, Jun 5, 2025 at 1:50 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Thu, Jun 05, 2025 at 01:14:20PM +0200, Amir Goldstein wrote:
> > On Thu, Jun 5, 2025 at 1:00 PM Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > On Thu, 05 Jun 2025 12:15:30 +0200, Amir Goldstein wrote:
> > > > The lookup helpers API was changed by merge of vfs-6.16-rc1.async.dir to
> > > > pass a non-const qstr pointer argument to lookup_one*() helpers.
> > > >
> > > > All of the callers of this API were changed to pass a pointer to temp
> > > > copy of qstr, except overlays that was passing a const pointer to
> > > > dentry->d_name that was changed to pass a non-const copy instead
> > > > when doing a lookup in lower layer which is not the fs of said dentry.
> > > >
> > > > [...]
> > >
> > > Applied to the vfs.fixes branch of the vfs/vfs.git tree.
> > > Patches in the vfs.fixes branch should appear in linux-next soon.
> > >
> >
> > Could you fix the grammatical mistake in my commit title:
> >
> > s/caused for/caused by/
>
> Already done including the ones in the comments to your fix. ;)

Great!
Only I don't see that it was pushed to vfs.fixes yet. forgot?
It's quite an ugly regression so I was hoping it would get in before rc1,
but no real harm if it makes it only to rc2.

Thanks,
Amir.