* [PATCH] ovl: fix NULL pointer dereference @ 2022-01-12 18:33 Christoph Fritz 2022-01-12 18:46 ` Christoph Fritz 2022-01-14 15:59 ` Miklos Szeredi 0 siblings, 2 replies; 9+ messages in thread From: Christoph Fritz @ 2022-01-12 18:33 UTC (permalink / raw) To: Miklos Szeredi; +Cc: Kevin Locke, linux-unionfs This patch is fixing a NULL pointer dereference to get a recently introduced warning message working. Fixes: 5b0a414d06c3 ("ovl: fix filattr copy-up failure") Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com> --- fs/overlayfs/copy_up.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index b193d08a3dc3..347b06479663 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -145,7 +145,7 @@ static int ovl_copy_fileattr(struct inode *inode, struct path *old, if (err == -ENOTTY || err == -EINVAL) return 0; pr_warn("failed to retrieve lower fileattr (%pd2, err=%i)\n", - old, err); + old->dentry, err); return err; } @@ -168,7 +168,7 @@ static int ovl_copy_fileattr(struct inode *inode, struct path *old, err = ovl_real_fileattr_get(new, &newfa); if (err) { pr_warn("failed to retrieve upper fileattr (%pd2, err=%i)\n", - new, err); + new->dentry, err); return err; } -- 2.30.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] ovl: fix NULL pointer dereference 2022-01-12 18:33 [PATCH] ovl: fix NULL pointer dereference Christoph Fritz @ 2022-01-12 18:46 ` Christoph Fritz 2022-01-12 20:28 ` Christoph Fritz 2022-01-12 20:58 ` Kevin Locke 2022-01-14 15:59 ` Miklos Szeredi 1 sibling, 2 replies; 9+ messages in thread From: Christoph Fritz @ 2022-01-12 18:46 UTC (permalink / raw) To: Miklos Szeredi; +Cc: Kevin Locke, linux-unionfs Hello Miklos On Wed, 2022-01-12 at 19:33 +0100, Christoph Fritz wrote: > This patch is fixing a NULL pointer dereference to get a recently > introduced warning message working. With that patch applied, a lot of these are popping up now: [ 7.132514] overlayfs: failed to retrieve upper fileattr (index/#26, err=-25) [ 7.141520] overlayfs: failed to retrieve upper fileattr (index/#27, err=-25) [ 8.699070] overlayfs: failed to retrieve upper fileattr (index/#7, err=-25) [ 8.715804] overlayfs: failed to retrieve upper fileattr (index/#8, err=-25) [ 8.723218] overlayfs: failed to retrieve upper fileattr (index/#9, err=-25) [ 8.829887] overlayfs: failed to retrieve upper fileattr (index/#43, err=-25) [ 9.387676] overlayfs: failed to retrieve upper fileattr (index/#a, err=-25) [ 9.667531] overlayfs: failed to retrieve upper fileattr (index/#b, err=-25) [ 9.874005] overlayfs: failed to retrieve upper fileattr (index/#c, err=-25) [ 9.934664] overlayfs: failed to retrieve upper fileattr (index/#58, err=-25) [ 9.942036] overlayfs: failed to retrieve upper fileattr (index/#59, err=-25) [ 9.949406] overlayfs: failed to retrieve upper fileattr (index/#60, err=-25) [ 9.956738] overlayfs: failed to retrieve upper fileattr (index/#61, err=-25) [ 10.311610] overlayfs: failed to retrieve upper fileattr (index/#d, err=-25) [ 10.712019] overlayfs: failed to retrieve upper fileattr (index/#e, err=-25) [ 31.901577] overlayfs: failed to retrieve upper fileattr (index/#64, err=-25) These have been -ENOIOCTLCMD errors but got (falsely?) converted to -ENOTTY by the recently introduced commit 5b0a414d06c3 ("ovl: fix filattr copy-up failure"): + if (err == -ENOIOCTLCMD) + err = -ENOTTY; Any ideas? Thanks -- Christoph ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ovl: fix NULL pointer dereference 2022-01-12 18:46 ` Christoph Fritz @ 2022-01-12 20:28 ` Christoph Fritz 2022-01-14 16:01 ` Miklos Szeredi 2022-01-12 20:58 ` Kevin Locke 1 sibling, 1 reply; 9+ messages in thread From: Christoph Fritz @ 2022-01-12 20:28 UTC (permalink / raw) To: Miklos Szeredi; +Cc: Kevin Locke, linux-unionfs > > [ 9.956738] overlayfs: failed to retrieve upper fileattr > (index/#61, err=-25) > [ 10.311610] overlayfs: failed to retrieve upper fileattr > (index/#d, err=-25) > [ 10.712019] overlayfs: failed to retrieve upper fileattr > (index/#e, err=-25) > [ 31.901577] overlayfs: failed to retrieve upper fileattr > (index/#64, err=-25) > > These have been -ENOIOCTLCMD errors but got (falsely?) converted to > -ENOTTY by the recently introduced commit 5b0a414d06c3 ("ovl: fix > filattr copy-up failure"): > > + if (err == -ENOIOCTLCMD) > + err = -ENOTTY; > > Any ideas? > Doing the same "quirk" for upper fileattr seems to fix the issues, but I have no clue about any other implications: diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index 347b06479663..1e69bc000dd8 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -167,6 +167,8 @@ static int ovl_copy_fileattr(struct inode *inode, struct path *old, err = ovl_real_fileattr_get(new, &newfa); if (err) { + if (err == -ENOTTY || err == -EINVAL) + return 0; pr_warn("failed to retrieve upper fileattr (%pd2, err=%i)\n", new->dentry, err); return err; ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] ovl: fix NULL pointer dereference 2022-01-12 20:28 ` Christoph Fritz @ 2022-01-14 16:01 ` Miklos Szeredi 0 siblings, 0 replies; 9+ messages in thread From: Miklos Szeredi @ 2022-01-14 16:01 UTC (permalink / raw) To: chf.fritz; +Cc: Kevin Locke, overlayfs [-- Attachment #1: Type: text/plain, Size: 1741 bytes --] On Wed, 12 Jan 2022 at 21:28, Christoph Fritz <chf.fritz@googlemail.com> wrote: > > > > > [ 9.956738] overlayfs: failed to retrieve upper fileattr > > (index/#61, err=-25) > > [ 10.311610] overlayfs: failed to retrieve upper fileattr > > (index/#d, err=-25) > > [ 10.712019] overlayfs: failed to retrieve upper fileattr > > (index/#e, err=-25) > > [ 31.901577] overlayfs: failed to retrieve upper fileattr > > (index/#64, err=-25) > > > > These have been -ENOIOCTLCMD errors but got (falsely?) converted to > > -ENOTTY by the recently introduced commit 5b0a414d06c3 ("ovl: fix > > filattr copy-up failure"): > > > > + if (err == -ENOIOCTLCMD) > > + err = -ENOTTY; > > > > Any ideas? > > > > Doing the same "quirk" for upper fileattr seems to fix the issues, but > I have no clue about any other implications: > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c > index 347b06479663..1e69bc000dd8 100644 > --- a/fs/overlayfs/copy_up.c > +++ b/fs/overlayfs/copy_up.c > @@ -167,6 +167,8 @@ static int ovl_copy_fileattr(struct inode *inode, struct path *old, > > err = ovl_real_fileattr_get(new, &newfa); > if (err) { > + if (err == -ENOTTY || err == -EINVAL) > + return 0; > pr_warn("failed to retrieve upper fileattr (%pd2, err=%i)\n", > new->dentry, err); > return err; > Can you please test the attached patch? It still prints one warning message to inform the user about this situation, but otherwise it should revert to the old behavior, like your suggested patch. Both patches pushed to: git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git#overlayfs-next Thanks, Miklos [-- Attachment #2: ovl-dont-fail-copy-up-if-no-fileattr-support-on-upper.patch --] [-- Type: text/x-patch, Size: 1569 bytes --] From: Miklos Szeredi <mszeredi@redhat.com> Subject: ovl: don't fail copy up if no fileattr support on upper Christoph Fritz is reporting that failure to copy up fileattr when upper doesn't support fileattr or xattr results in a regression. Return success in these failure cases; this reverts overlayfs to the old behavior. Add a pr_warn_once() in these cases to still let the user know about the copy up failures. Reported-by: Christoph Fritz <chf.fritz@googlemail.com> Fixes: 72db82115d2b ("ovl: copy up sync/noatime fileattr flags") Cc: <stable@vger.kernel.org> # v5.15 Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> --- fs/overlayfs/copy_up.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -157,7 +157,9 @@ static int ovl_copy_fileattr(struct inod */ if (oldfa.flags & OVL_PROT_FS_FLAGS_MASK) { err = ovl_set_protattr(inode, new->dentry, &oldfa); - if (err) + if (err == -EPERM) + pr_warn_once("copying fileattr: no xattr on upper\n"); + else if (err) return err; } @@ -167,6 +169,14 @@ static int ovl_copy_fileattr(struct inod err = ovl_real_fileattr_get(new, &newfa); if (err) { + /* + * Returning an error if upper doesn't support fileattr will + * result in a regression, so revert to the old behavior. + */ + if (err == -ENOTTY || err == -EINVAL) { + pr_warn_once("copying fileattr: no support on upper\n"); + return 0; + } pr_warn("failed to retrieve upper fileattr (%pd2, err=%i)\n", new->dentry, err); return err; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ovl: fix NULL pointer dereference 2022-01-12 18:46 ` Christoph Fritz 2022-01-12 20:28 ` Christoph Fritz @ 2022-01-12 20:58 ` Kevin Locke 2022-01-12 21:06 ` Christoph Fritz 1 sibling, 1 reply; 9+ messages in thread From: Kevin Locke @ 2022-01-12 20:58 UTC (permalink / raw) To: Christoph Fritz; +Cc: Miklos Szeredi, linux-unionfs Hi Christoph, On Wed, 2022-01-12 at 19:46 +0100, Christoph Fritz wrote: > On Wed, 2022-01-12 at 19:33 +0100, Christoph Fritz wrote: >> This patch is fixing a NULL pointer dereference to get a recently >> introduced warning message working. Good catch! > With that patch applied, a lot of these are popping up now: > > [ 7.132514] overlayfs: failed to retrieve upper fileattr (index/#26, err=-25) > > [...] > > These have been -ENOIOCTLCMD errors but got (falsely?) converted to > -ENOTTY by the recently introduced commit 5b0a414d06c3 ("ovl: fix filattr copy-up failure"): Which filesystem are you using for upper (and lower) in your mount? Presumably the upper doesn't support file attributes, if it returns -ENOIOCTLCMD? If so, I guess the question would be what behavior overlayfs should have when attributes can't be copied from lower to upper, which is actually a question Miklos raised when that commit was being worked on![^1] Perhaps your use case can help inform a good answer. Cheers, Kevin [^1]: https://lore.kernel.org/linux-unionfs/CAJfpegsRo3e-9B64W37YrmvDcjo0QB9t+coAW3mO6TSqdROz2w@mail.gmail.com/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ovl: fix NULL pointer dereference 2022-01-12 20:58 ` Kevin Locke @ 2022-01-12 21:06 ` Christoph Fritz 2022-01-12 22:25 ` Kevin Locke 0 siblings, 1 reply; 9+ messages in thread From: Christoph Fritz @ 2022-01-12 21:06 UTC (permalink / raw) To: Kevin Locke; +Cc: Miklos Szeredi, linux-unionfs Hi Kevin > > These have been -ENOIOCTLCMD errors but got (falsely?) converted to > > -ENOTTY by the recently introduced commit 5b0a414d06c3 ("ovl: fix > > filattr copy-up failure"): > > Which filesystem are you using for upper (and lower) in your mount? It's tmpfs. > Presumably the upper doesn't support file attributes, if it returns > -ENOIOCTLCMD? Tmpfs does support file attributes. Thanks -- Christoph ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ovl: fix NULL pointer dereference 2022-01-12 21:06 ` Christoph Fritz @ 2022-01-12 22:25 ` Kevin Locke 2022-01-13 23:21 ` Christoph Fritz 0 siblings, 1 reply; 9+ messages in thread From: Kevin Locke @ 2022-01-12 22:25 UTC (permalink / raw) To: Christoph Fritz; +Cc: Miklos Szeredi, linux-unionfs On Wed, 2022-01-12 at 22:06 +0100, Christoph Fritz wrote: >>> These have been -ENOIOCTLCMD errors but got (falsely?) converted to >>> -ENOTTY by the recently introduced commit 5b0a414d06c3 ("ovl: fix >>> filattr copy-up failure"): >> >> Which filesystem are you using for upper (and lower) in your mount? > > > It's tmpfs. > >> Presumably the upper doesn't support file attributes, if it returns >> -ENOIOCTLCMD? > > > Tmpfs does support file attributes. Although tmpfs can support extended attributes (attr(1)/xattr(7)) with CONFIG_TMPFS_XATTR, I'm not aware of support for traditional attributes (chattr(1)). I'm also not able to reproduce the error message you mentioned with extended attributes. With your patch[^1] applied to 5.16, I ran the following: mkdir lower upwork overlay mount -t tmpfs - lower mount -t tmpfs - upwork mkdir upwork/upper upwork/work touch lower/file.txt setfacl -m 'u:0:rwx' lower/file.txt mount -t overlay -o "lowerdir=$PWD/lower,upperdir=$PWD/upwork/upper,workdir=$PWD/upwork/work" - overlay mv overlay/file.txt overlay/file2.txt getfattr -d -m '.*' overlay/file2.txt This copied file.txt from lower to upper with the system.posix_acl_access extended attribute and did not print any messages from overlayfs to the kernel log. Could you provide a minimal, reproducible example for the log messages you mentioned observing? Thanks, Kevin [^1]: https://lore.kernel.org/linux-unionfs/10d8ed194b934c298713ad7f0958329b46573dd1.camel@googlemail.com/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ovl: fix NULL pointer dereference 2022-01-12 22:25 ` Kevin Locke @ 2022-01-13 23:21 ` Christoph Fritz 0 siblings, 0 replies; 9+ messages in thread From: Christoph Fritz @ 2022-01-13 23:21 UTC (permalink / raw) To: Kevin Locke; +Cc: Miklos Szeredi, linux-unionfs > Could you provide a minimal, reproducible example for the log messages > you mentioned observing? Overlay gets configured in an initramfs, after switching root and while booting these new error messages happen. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ovl: fix NULL pointer dereference 2022-01-12 18:33 [PATCH] ovl: fix NULL pointer dereference Christoph Fritz 2022-01-12 18:46 ` Christoph Fritz @ 2022-01-14 15:59 ` Miklos Szeredi 1 sibling, 0 replies; 9+ messages in thread From: Miklos Szeredi @ 2022-01-14 15:59 UTC (permalink / raw) To: chf.fritz; +Cc: Kevin Locke, overlayfs On Wed, 12 Jan 2022 at 19:33, Christoph Fritz <chf.fritz@googlemail.com> wrote: > > This patch is fixing a NULL pointer dereference to get a recently > introduced warning message working. Thanks, applied. Miklos ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-01-14 16:01 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-12 18:33 [PATCH] ovl: fix NULL pointer dereference Christoph Fritz 2022-01-12 18:46 ` Christoph Fritz 2022-01-12 20:28 ` Christoph Fritz 2022-01-14 16:01 ` Miklos Szeredi 2022-01-12 20:58 ` Kevin Locke 2022-01-12 21:06 ` Christoph Fritz 2022-01-12 22:25 ` Kevin Locke 2022-01-13 23:21 ` Christoph Fritz 2022-01-14 15:59 ` Miklos Szeredi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).