| Message ID | 20241107005720.901335-5-vinicius.gomes@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | overlayfs: Optimize override/revert creds | expand |
On Thu, Nov 7, 2024 at 1:57 AM Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote: > > Use override_creds_light() in ovl_override_creds() and > revert_creds_light() in ovl_revert_creds_light(). > > The _light() functions do not change the 'usage' of the credentials in > question, as they refer to the credentials associated with the > mounter, which have a longer lifetime. > > In ovl_setup_cred_for_create(), do not need to modify the mounter > credentials (returned by override_creds()) 'usage' counter. Add a > warning to verify that we are indeed working with the mounter > credentials (stored in the superblock). Failure in this assumption > means that creds may leak. > > Suggested-by: Christian Brauner <brauner@kernel.org> > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> > --- > fs/overlayfs/dir.c | 7 ++++++- > fs/overlayfs/util.c | 4 ++-- > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > index 09db5eb19242..136a2c7fb9e5 100644 > --- a/fs/overlayfs/dir.c > +++ b/fs/overlayfs/dir.c > @@ -571,7 +571,12 @@ static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode, > put_cred(override_cred); > return err; > } > - put_cred(override_creds(override_cred)); > + > + /* > + * 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); > > return 0; 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... Christian, Miklos, Can you see a problem? Thanks, Amir. unreferenced object 0xffff888008ad8240 (size 192): comm "fanotify06", pid 1803, jiffies 4294890084 hex dump (first 32 bytes): 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace (crc ee6a93ea): [<00000000ab4340a4>] __create_object+0x22/0x83 [<0000000053dcaf3b>] kmem_cache_alloc_noprof+0x156/0x1e6 [<00000000b4a08c1d>] prepare_creds+0x1d/0xf9 [<00000000c55dfb6c>] ovl_setup_cred_for_create+0x27/0x93 [<00000000f82af4ee>] ovl_create_or_link+0x73/0x1bd [<0000000040a439db>] ovl_create_object+0xda/0x11d [<00000000fbbadf17>] lookup_open.isra.0+0x3a0/0x3ff [<0000000007a2faf0>] open_last_lookups+0x160/0x223 [<00000000e7d8243a>] path_openat+0x136/0x1b5 [<0000000004e51585>] do_filp_open+0x57/0xb8 [<0000000053871b92>] do_sys_openat2+0x6f/0xc0 [<000000004d76b8b7>] do_sys_open+0x3f/0x60 [<000000009b0be238>] do_syscall_64+0x96/0xf8 [<000000006ff466ad>] entry_SYSCALL_64_after_hwframe+0x76/0x7e
Amir Goldstein <amir73il@gmail.com> writes: > On Thu, Nov 7, 2024 at 1:57 AM Vinicius Costa Gomes > <vinicius.gomes@intel.com> 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) > > Christian, Miklos, > > Can you see a problem? > > Thanks, > Amir. > > > unreferenced object 0xffff888008ad8240 (size 192): > comm "fanotify06", pid 1803, jiffies 4294890084 > hex dump (first 32 bytes): > 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace (crc ee6a93ea): > [<00000000ab4340a4>] __create_object+0x22/0x83 > [<0000000053dcaf3b>] kmem_cache_alloc_noprof+0x156/0x1e6 > [<00000000b4a08c1d>] prepare_creds+0x1d/0xf9 > [<00000000c55dfb6c>] ovl_setup_cred_for_create+0x27/0x93 > [<00000000f82af4ee>] ovl_create_or_link+0x73/0x1bd > [<0000000040a439db>] ovl_create_object+0xda/0x11d > [<00000000fbbadf17>] lookup_open.isra.0+0x3a0/0x3ff > [<0000000007a2faf0>] open_last_lookups+0x160/0x223 > [<00000000e7d8243a>] path_openat+0x136/0x1b5 > [<0000000004e51585>] do_filp_open+0x57/0xb8 > [<0000000053871b92>] do_sys_openat2+0x6f/0xc0 > [<000000004d76b8b7>] do_sys_open+0x3f/0x60 > [<000000009b0be238>] do_syscall_64+0x96/0xf8 > [<000000006ff466ad>] entry_SYSCALL_64_after_hwframe+0x76/0x7e Cheers,
On Wed, Nov 13, 2024 at 8:30 PM Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote: > > Amir Goldstein <amir73il@gmail.com> writes: > > > On Thu, Nov 7, 2024 at 1:57 AM Vinicius Costa Gomes > > <vinicius.gomes@intel.com> 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: --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -576,7 +576,8 @@ 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)); + old_cred = override_creds(override_cred); + WARN_ON_ONCE(old_cred != ovl_creds(dentry->d_sb)); put_cred(override_cred); return 0; Compiler optimized out override_creds(override_cred)? :-/ However, this is not enough. Dropping the ref of the new creds is going to drop the refcount to zero, so that is incorrect, we need to return the reference to the new creds explicitly to the callers. I will send a patch. Thanks, Amir.
On Thu, Nov 14, 2024 at 9:56 AM Amir Goldstein <amir73il@gmail.com> wrote: > > On Wed, Nov 13, 2024 at 8:30 PM Vinicius Costa Gomes > <vinicius.gomes@intel.com> wrote: > > > > Amir Goldstein <amir73il@gmail.com> writes: > > > > > On Thu, Nov 7, 2024 at 1:57 AM Vinicius Costa Gomes > > > <vinicius.gomes@intel.com> 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: > > --- a/fs/overlayfs/dir.c > +++ b/fs/overlayfs/dir.c > @@ -576,7 +576,8 @@ 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)); > + old_cred = override_creds(override_cred); > + WARN_ON_ONCE(old_cred != ovl_creds(dentry->d_sb)); > put_cred(override_cred); > > return 0; > > Compiler optimized out override_creds(override_cred)? :-/ Nope, this voodoo did not help either. > > However, this is not enough. > > Dropping the ref of the new creds is going to drop the refcount to zero, > so that is incorrect, we need to return the reference to the new creds > explicitly to the callers. I will send a patch. And neither did this. The suspect memleak is still reported. Any other ideas? Thanks, Amir.
On Thu, Nov 14, 2024 at 9:56 AM Amir Goldstein <amir73il@gmail.com> wrote: > > On Wed, Nov 13, 2024 at 8:30 PM Vinicius Costa Gomes > <vinicius.gomes@intel.com> wrote: > > > > Amir Goldstein <amir73il@gmail.com> writes: > > > > > On Thu, Nov 7, 2024 at 1:57 AM Vinicius Costa Gomes > > > <vinicius.gomes@intel.com> 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]: - 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@gmail.com/
Amir Goldstein <amir73il@gmail.com> writes: > On Thu, Nov 14, 2024 at 9:56 AM Amir Goldstein <amir73il@gmail.com> wrote: >> >> On Wed, Nov 13, 2024 at 8:30 PM Vinicius Costa Gomes >> <vinicius.gomes@intel.com> wrote: >> > >> > Amir Goldstein <amir73il@gmail.com> writes: >> > >> > > On Thu, Nov 7, 2024 at 1:57 AM Vinicius Costa Gomes >> > > <vinicius.gomes@intel.com> 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@intel.com> > > - 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@gmail.com/
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index 09db5eb19242..136a2c7fb9e5 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -571,7 +571,12 @@ static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode, put_cred(override_cred); return err; } - put_cred(override_creds(override_cred)); + + /* + * 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); return 0; diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c index 9408046f4f41..3bb107471fb4 100644 --- a/fs/overlayfs/util.c +++ b/fs/overlayfs/util.c @@ -65,12 +65,12 @@ const struct cred *ovl_override_creds(struct super_block *sb) { struct ovl_fs *ofs = OVL_FS(sb); - return override_creds(ofs->creator_cred); + return override_creds_light(ofs->creator_cred); } void ovl_revert_creds(const struct cred *old_cred) { - revert_creds(old_cred); + revert_creds_light(old_cred); } /*
Use override_creds_light() in ovl_override_creds() and revert_creds_light() in ovl_revert_creds_light(). The _light() functions do not change the 'usage' of the credentials in question, as they refer to the credentials associated with the mounter, which have a longer lifetime. In ovl_setup_cred_for_create(), do not need to modify the mounter credentials (returned by override_creds()) 'usage' counter. Add a warning to verify that we are indeed working with the mounter credentials (stored in the superblock). Failure in this assumption means that creds may leak. Suggested-by: Christian Brauner <brauner@kernel.org> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> --- fs/overlayfs/dir.c | 7 ++++++- fs/overlayfs/util.c | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-)