Amir Goldstein <amir73il@xxxxxxxxx> writes: > On Thu, Nov 14, 2024 at 9:56 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: >> >> On Wed, Nov 13, 2024 at 8:30 PM Vinicius Costa Gomes >> <vinicius.gomes@xxxxxxxxx> wrote: >> > >> > Amir Goldstein <amir73il@xxxxxxxxx> writes: >> > >> > > On Thu, Nov 7, 2024 at 1:57 AM Vinicius Costa Gomes >> > > <vinicius.gomes@xxxxxxxxx> wrote: >> > >> > [...] >> > >> > > >> > > Vinicius, >> > > >> > > While testing fanotify with LTP tests (some are using overlayfs), >> > > kmemleak consistently reports the problems below. >> > > >> > > Can you see the bug, because I don't see it. >> > > Maybe it is a false positive... >> > >> > Hm, if the leak wasn't there before and we didn't touch anything related to >> > prepare_creds(), I think that points to the leak being real. >> > >> > But I see your point, still not seeing it. >> > >> > This code should be equivalent to the code we have now (just boot >> > tested): >> > >> > ---- >> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c >> > index 136a2c7fb9e5..7ebc2fd3097a 100644 >> > --- a/fs/overlayfs/dir.c >> > +++ b/fs/overlayfs/dir.c >> > @@ -576,8 +576,7 @@ static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode, >> > * We must be called with creator creds already, otherwise we risk >> > * leaking creds. >> > */ >> > - WARN_ON_ONCE(override_creds(override_cred) != ovl_creds(dentry->d_sb)); >> > - put_cred(override_cred); >> > + WARN_ON_ONCE(override_creds_light(override_cred) != ovl_creds(dentry->d_sb)); >> > >> > return 0; >> > } >> > ---- >> > >> > Does it change anything? (I wouldn't think so, just to try something) >> >> No, but I think this does: >> > > Vinicius, > > Sorry, your fix is correct. I did not apply it properly when I tested. > > I edited the comment as follows and applied on top of the patch > that I just sent [1]: > I just noticed there's a typo in the first sentence of the commit message, the function name that we are using revert_creds_light() is ovl_revert_creds(). Could you fix that while you are at it? Glad that the fix works: Acked-by: Vinicius Costa Gomes <vinicius.gomes@xxxxxxxxx> > > - put_cred(override_creds(override_cred)); > + > + /* > + * Caller is going to match this with revert_creds_light() and drop > + * reference on the returned creds. > + * We must be called with creator creds already, otherwise we risk > + * leaking creds. > + */ > + old_cred = override_creds_light(override_cred); > + WARN_ON_ONCE(old_cred != ovl_creds(dentry->d_sb)); > > return override_cred; > > Thanks, > Amir. > > [1] https://lore.kernel.org/linux-unionfs/20241114100536.628162-1-amir73il@xxxxxxxxx/ -- Vinicius
