| Message ID | 20241114100536.628162-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ovl: pass an explicit reference of creators creds to callers | expand |
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.
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.
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!
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>
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.
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; }
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(-)