[PATCH] ovl: pass an explicit reference of creators creds to callers

[PATCH] ovl: pass an explicit reference of creators creds to callers

[Amir Goldstein]

ovl_setup_cred_for_create() decrements one refcount of new creds and
ovl_revert_creds() in callers decrements the last refcount.

In preparation to revert_creds_light() back to caller creds, pass an
explicit reference of the creators creds to the callers and drop the
refcount explicitly in the callers after ovl_revert_creds().

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Miklos, Christian,

I was chasing a suspect memleak in revert_creds_light() patches.
This fix is unrelated to memleak but I think it is needed for
correctness anyway.

This applies in the middle of the series after adding the
ovl_revert_creds() helper.

Thanks,
Amir.

 fs/overlayfs/dir.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 09db5eb19242..4b0bb7a91d37 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -553,15 +553,17 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 	goto out_dput;
 }
 
-static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode,
-				     umode_t mode, const struct cred *old_cred)
+static const struct cred *ovl_setup_cred_for_create(struct dentry *dentry,
+						    struct inode *inode,
+						    umode_t mode,
+						    const struct cred *old_cred)
 {
 	int err;
 	struct cred *override_cred;
 
 	override_cred = prepare_creds();
 	if (!override_cred)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	override_cred->fsuid = inode->i_uid;
 	override_cred->fsgid = inode->i_gid;
@@ -569,19 +571,18 @@ static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode,
 					      old_cred, override_cred);
 	if (err) {
 		put_cred(override_cred);
-		return err;
+		return ERR_PTR(err);
 	}
 	put_cred(override_creds(override_cred));
-	put_cred(override_cred);
 
-	return 0;
+	return override_cred;
 }
 
 static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 			      struct ovl_cattr *attr, bool origin)
 {
 	int err;
-	const struct cred *old_cred;
+	const struct cred *old_cred, *new_cred = NULL;
 	struct dentry *parent = dentry->d_parent;
 
 	old_cred = ovl_override_creds(dentry->d_sb);
@@ -610,9 +611,13 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 		 * create a new inode, so just use the ovl mounter's
 		 * fs{u,g}id.
 		 */
-		err = ovl_setup_cred_for_create(dentry, inode, attr->mode, old_cred);
-		if (err)
+		new_cred = ovl_setup_cred_for_create(dentry, inode, attr->mode,
+						     old_cred);
+		err = PTR_ERR(new_cred);
+		if (IS_ERR(new_cred)) {
+			new_cred = NULL;
 			goto out_revert_creds;
+		}
 	}
 
 	if (!ovl_dentry_is_whiteout(dentry))
@@ -622,6 +627,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 
 out_revert_creds:
 	ovl_revert_creds(old_cred);
+	put_cred(new_cred);
 	return err;
 }
 
@@ -1306,7 +1312,7 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 static int ovl_create_tmpfile(struct file *file, struct dentry *dentry,
 			      struct inode *inode, umode_t mode)
 {
-	const struct cred *old_cred;
+	const struct cred *old_cred, *new_cred = NULL;
 	struct path realparentpath;
 	struct file *realfile;
 	struct dentry *newdentry;
@@ -1315,9 +1321,12 @@ static int ovl_create_tmpfile(struct file *file, struct dentry *dentry,
 	int err;
 
 	old_cred = ovl_override_creds(dentry->d_sb);
-	err = ovl_setup_cred_for_create(dentry, inode, mode, old_cred);
-	if (err)
+	new_cred = ovl_setup_cred_for_create(dentry, inode, mode, old_cred);
+	err = PTR_ERR(new_cred);
+	if (IS_ERR(new_cred)) {
+		new_cred = NULL;
 		goto out_revert_creds;
+	}
 
 	ovl_path_upper(dentry->d_parent, &realparentpath);
 	realfile = backing_tmpfile_open(&file->f_path, flags, &realparentpath,
@@ -1338,6 +1347,7 @@ static int ovl_create_tmpfile(struct file *file, struct dentry *dentry,
 	}
 out_revert_creds:
 	ovl_revert_creds(old_cred);
+	put_cred(new_cred);
 	return err;
 }
 
-- 
2.34.1

Re: [PATCH] ovl: pass an explicit reference of creators creds to callers

[Christian Brauner]

On Thu, Nov 14, 2024 at 11:05:36AM +0100, Amir Goldstein wrote:
> ovl_setup_cred_for_create() decrements one refcount of new creds and
> ovl_revert_creds() in callers decrements the last refcount.
> 
> In preparation to revert_creds_light() back to caller creds, pass an
> explicit reference of the creators creds to the callers and drop the
> refcount explicitly in the callers after ovl_revert_creds().
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 
> Miklos, Christian,
> 
> I was chasing a suspect memleak in revert_creds_light() patches.
> This fix is unrelated to memleak but I think it is needed for
> correctness anyway.
> 
> This applies in the middle of the series after adding the
> ovl_revert_creds() helper.

I'm going to try and reproduce the kmemleak with your ovl_creds branch
as is and then retry with the series applied as is plus one small fix
you correctly pointed out.

Re: [PATCH] ovl: pass an explicit reference of creators creds to callers

[Amir Goldstein]

On Thu, Nov 14, 2024 at 11:12 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Thu, Nov 14, 2024 at 11:05:36AM +0100, Amir Goldstein wrote:
> > ovl_setup_cred_for_create() decrements one refcount of new creds and
> > ovl_revert_creds() in callers decrements the last refcount.
> >
> > In preparation to revert_creds_light() back to caller creds, pass an
> > explicit reference of the creators creds to the callers and drop the
> > refcount explicitly in the callers after ovl_revert_creds().
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > Miklos, Christian,
> >
> > I was chasing a suspect memleak in revert_creds_light() patches.
> > This fix is unrelated to memleak but I think it is needed for
> > correctness anyway.
> >
> > This applies in the middle of the series after adding the
> > ovl_revert_creds() helper.
>
> I'm going to try and reproduce the kmemleak with your ovl_creds branch
> as is and then retry with the series applied as is plus one small fix
> you correctly pointed out.

Don't bother. The fix that Vinicius sent me was correct.
I still want to use this patch, so would appreciate a review.

Thanks,
Amir.

Re: [PATCH] ovl: pass an explicit reference of creators creds to callers

[Christian Brauner]

On Thu, Nov 14, 2024 at 12:03:16PM +0100, Amir Goldstein wrote:
> On Thu, Nov 14, 2024 at 11:12 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Thu, Nov 14, 2024 at 11:05:36AM +0100, Amir Goldstein wrote:
> > > ovl_setup_cred_for_create() decrements one refcount of new creds and
> > > ovl_revert_creds() in callers decrements the last refcount.
> > >
> > > In preparation to revert_creds_light() back to caller creds, pass an
> > > explicit reference of the creators creds to the callers and drop the
> > > refcount explicitly in the callers after ovl_revert_creds().
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >
> > > Miklos, Christian,
> > >
> > > I was chasing a suspect memleak in revert_creds_light() patches.
> > > This fix is unrelated to memleak but I think it is needed for
> > > correctness anyway.
> > >
> > > This applies in the middle of the series after adding the
> > > ovl_revert_creds() helper.
> >
> > I'm going to try and reproduce the kmemleak with your ovl_creds branch
> > as is and then retry with the series applied as is plus one small fix
> > you correctly pointed out.
> 
> Don't bother. The fix that Vinicius sent me was correct.
> I still want to use this patch, so would appreciate a review.

Ah, thanks for letting me know!

Re: [PATCH] ovl: pass an explicit reference of creators creds to callers

[Christian Brauner]

On Thu, Nov 14, 2024 at 11:05:36AM +0100, Amir Goldstein wrote:
> ovl_setup_cred_for_create() decrements one refcount of new creds and
> ovl_revert_creds() in callers decrements the last refcount.
> 
> In preparation to revert_creds_light() back to caller creds, pass an
> explicit reference of the creators creds to the callers and drop the
> refcount explicitly in the callers after ovl_revert_creds().
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 
> Miklos, Christian,
> 
> I was chasing a suspect memleak in revert_creds_light() patches.
> This fix is unrelated to memleak but I think it is needed for
> correctness anyway.
> 
> This applies in the middle of the series after adding the
> ovl_revert_creds() helper.

Ok, so you're moving the put_cred() on the cred_for_create creds into
the callers. Tbh, that patch alone here is very confusing because with
the last patch in your tree at 07532f7f8450 you're calling

old_cred = override_creds(override_cred);

which made it buggy. But I see in 3756f22061c2 this is fixed and
correctly uses

old_cred = override_creds_light(override_cred);

as expected. And together with that change your patch here makes perfect
sense. I don't want to complain too much but it would've been nice if
that was spelled out in the commit message. Would've spared me 30
minutes of staring at the code until I refreshed your branch. :)

Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH] ovl: pass an explicit reference of creators creds to callers

[Amir Goldstein]

On Thu, Nov 14, 2024 at 12:52 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Thu, Nov 14, 2024 at 11:05:36AM +0100, Amir Goldstein wrote:
> > ovl_setup_cred_for_create() decrements one refcount of new creds and
> > ovl_revert_creds() in callers decrements the last refcount.
> >
> > In preparation to revert_creds_light() back to caller creds, pass an
> > explicit reference of the creators creds to the callers and drop the
> > refcount explicitly in the callers after ovl_revert_creds().
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > Miklos, Christian,
> >
> > I was chasing a suspect memleak in revert_creds_light() patches.
> > This fix is unrelated to memleak but I think it is needed for
> > correctness anyway.
> >
> > This applies in the middle of the series after adding the
> > ovl_revert_creds() helper.
>
> Ok, so you're moving the put_cred() on the cred_for_create creds into
> the callers. Tbh, that patch alone here is very confusing because with
> the last patch in your tree at 07532f7f8450 you're calling
>
> old_cred = override_creds(override_cred);
>
> which made it buggy. But I see in 3756f22061c2 this is fixed and
> correctly uses
>
> old_cred = override_creds_light(override_cred);
>
> as expected. And together with that change your patch here makes perfect
> sense. I don't want to complain too much but it would've been nice if
> that was spelled out in the commit message. Would've spared me 30
> minutes of staring at the code until I refreshed your branch. :)

No, do feel free to complain :)
At the time of writing the commit message I still did not understand the bug.
It just made sense to me that the reference should be passed explicitly,
because I *thought* that ovl_setup_cred_for_create() was dropping the
last reference after the last commit, but that wasn't going to explain a
memleak (quite the opposite).

My point with *this* patch is that it was more clear to code readers
who is responsible to drop the reference and after a while, this
clarity also helped me to see the bug.

>
> Reviewed-by: Christian Brauner <brauner@kernel.org>

Thanks!
Amir.