Re: [PATCH] [REGRESSION] ovl: Handle ENOSYS when fileattr support is missing in lower/upper fs
From: Miklos Szeredi
Date: Fri Apr 21 2023 - 10:27:10 EST
Hi Jonathan,
Can you please try out the attached patch?
Thanks,
Miklos
On Wed, 22 Mar 2023 at 19:42, Jonathan Katz <jkatz@xxxxxxxxxxxx> wrote:
>
> Confirmed bindfs interaction:
>
> Based on your bindfs comment I redid my configuration as follows:
> ORIGINAL (FAILS):
> FS1 - exports "/Data" (nfs)
> FS2 - Mounts "/Data", does a bindfs, does an overlay
>
> TEST (SUCCEEDS):
> FS1 - does a bindfs and exports a series of directories:
> # bindfs -u 5007, -g 5007 /Data /Data-jiajun
> /etc/exports:
> /Data machine.org(ro,sync,no_subtree_check)
> /Data-jiajun machine.org(ro,fsid=12,sync,no_subtree_check)
> FS2 - used to do bindfs to make the lowers, but, now mounts
> "/Data-jiajun" as the lower
> FS2 then does the overlay and samba share.
> It would not let me do the 2nd export if I did not
> include the fsid entry....
>
> WOOT WOOT.
>
>
> Not an ideal solution as I have to make changes to 2 servers in order
> to accomplish my goal :/.
>
>
>
>
>
>
>
>
> On Tue, Mar 14, 2023 at 7:43 PM Jonathan Katz <jkatz@xxxxxxxxxxxx> wrote:
> >
> > On Thu, Mar 9, 2023 at 7:31 AM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> > >
> > > On Tue, 7 Mar 2023 at 18:14, Jonathan Katz <jkatz@xxxxxxxxxxxx> wrote:
> > > >
> > > > On Tue, Mar 7, 2023 at 12:38 AM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Tue, 7 Mar 2023 at 02:12, Jonathan Katz <jkatz@xxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > Hi all,
> > > > > >
> > > > > > In pursuing this issue, I downloaded the kernel source to see if I
> > > > > > could debug it further. In so doing, it looks like Christian's patch
> > > > > > was never committed to the main source tree (sorry if my terminology
> > > > > > is wrong). This is up to and including the 6.3-rc1. I could also
> > > > > > find no mention of the fix in the log.
> > > > > >
> > > > > > I am trying to manually apply this patch now, but, I am wondering if
> > > > > > there was some reason that it was not applied (e.g. it introduces some
> > > > > > instability?)?
> > > > >
> > > > > It's fixing the bug in the wrong place, i.e. it's checking for an
> > > > > -ENOSYS return from vfs_fileattr_get(), but that return value is not
> > > > > valid at that point.
> > > > >
> > > > > The right way to fix this bug is to prevent -ENOSYS from being
> > > > > returned in the first place.
> > > > >
> > > > > Commit 02c0cab8e734 ("fuse: ioctl: translate ENOSYS") fixes one of
> > > > > those bugs, but of course it's possible that I missed something in
> > > > > that fix.
> > > > >
> > > > > Can you please first verify that an upstream kernel (>v6.0) can also
> > > > > reproduce this issue?
> > > >
> > > > Got ya. that makes a lot of sense, thank you.
> > > >
> > > > I have confirmed that I continue to get the error with 6.2 .
> > > > quick summary of the lowerdir:
> > > > server ---- NFS(ro) ---- > client "/nfs"
> > > > client "/nfs" --- bindfs(uidmap) --- > client "/lower"
> > >
> > > Can you please run bindfs in debugging mode (-d) and send the
> > > resulting log after reproducing the issue?
> > >
> > > Thanks,
> > > Miklos
> >
> > OUCH -- MY LAST EMAIL WAS REJECTED FOR BEING TOO BIG
> > I HOPE THAT I AM SUMMARIZING THE RELEVANT INFORMATION HERE:
> >
> >
> > Hi Miklos, thank you.... I am sorry for the delay.
> >
> > The log is somewhat long and was sent in a separate email.
> >
> > I broke up the log into entries to try to match the chronology of actions:
> > * ENTRY 1 nfs mount the external drive
> > * ENTRY 2 perform the bind fs
> > * ENTRY 3 perform the overlay
> > * ENTRY 4 restart smb
> > * ENTRY 5 mount the filesystem on a windows box
> > * ENTRY 6 performing some navigation on the windows file explorer
> > * ENTRY 7 attempt to open a data file with the windows application.
> >
> > The only place that generated a kernel error in dmesg was at ENTRY 7.
> >
> > Because the logs are so big, I tried to parse them, I may have made a
> > mistake or omitted information -- if you think so, as mentioned, the
> > full bindfs logs were sent separately
> >
> >
> > Here is my attempt to parse out the errors associated with this dmesg entry:
> >
> > [ 1925.705908] overlayfs: failed to retrieve lower fileattr (8020
> > MeOHH2O RecoverySample1-20221216-A-JJL-WebinarHilic10C-TOF-TT54-Neg-1632.d/chromatography-data.sqlite,
> > err=-38)
> >
> > --
> > unique: 1550, opcode: GETXATTR (22), nodeid: 71, insize: 73, pid: 3458
> > getxattr /eimstims1/deleteme2/8020 MeOHH2O
> > RecoverySample1-20221216-A-JJL-WebinarHilic10C-TOF-TT54-Neg-1632.d/chromatography-data-pre.sqlite
> > trusted.overlay.metacopy 0
> > unique: 1550, error: -95 (Operation not supported), outsize: 16
> > --
> > unique: 3922, opcode: GETXATTR (22), nodeid: 71, insize: 72, pid: 3458
> > getxattr /eimstims1/deleteme2/8020 MeOHH2O
> > RecoverySample1-20221216-A-JJL-WebinarHilic10C-TOF-TT54-Neg-1632.d/chromatography-data-pre.sqlite
> > system.posix_acl_access 132
> > unique: 3922, error: -95 (Operation not supported), outsize: 16
> > --
> > unique: 3954, opcode: GETXATTR (22), nodeid: 71, insize: 72, pid: 3458
> > getxattr /eimstims1/deleteme2/8020 MeOHH2O
> > RecoverySample1-20221216-A-JJL-WebinarHilic10C-TOF-TT54-Neg-1632.d/chromatography-data-pre.sqlite
> > system.posix_acl_access 132
> > unique: 3954, error: -95 (Operation not supported), outsize: 16
> > --
> > unique: 3960, opcode: GETXATTR (22), nodeid: 71, insize: 72, pid: 3458
> > getxattr /eimstims1/deleteme2/8020 MeOHH2O
> > RecoverySample1-20221216-A-JJL-WebinarHilic10C-TOF-TT54-Neg-1632.d/chromatography-data-pre.sqlite
> > system.posix_acl_access 132
> > unique: 3960, error: -95 (Operation not supported), outsize: 16
> >
> >
> > Thank you again!
> >
> > -Jonathan
From: Miklos Szeredi <mszeredi@redhat.com>
Subject: fuse: ioctl: translate ENOSYS in outarg
Fuse shouldn't return ENOSYS from its ioctl implementation. If userspace
responds with ENOSYS it should be translated to ENOTTY.
There are two ways to return an error from the IOCTL request:
- fuse_out_header.error
- fuse_ioctl_out.result
Commit 02c0cab8e734 ("fuse: ioctl: translate ENOSYS") already fixed this
issue for the first case, but missed the second case. This patch fixes the
second case.
Reported-by: Jonathan Katz <jkatz@eitmlabs.org>
Fixes: 02c0cab8e734 ("fuse: ioctl: translate ENOSYS")
Cc: <stable@vger.kernel.org>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
fs/fuse/ioctl.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
--- a/fs/fuse/ioctl.c
+++ b/fs/fuse/ioctl.c
@@ -9,14 +9,23 @@
#include <linux/compat.h>
#include <linux/fileattr.h>
-static ssize_t fuse_send_ioctl(struct fuse_mount *fm, struct fuse_args *args)
+static ssize_t fuse_send_ioctl(struct fuse_mount *fm, struct fuse_args *args,
+ struct fuse_ioctl_out *outarg)
{
- ssize_t ret = fuse_simple_request(fm, args);
+ ssize_t ret;
+
+ args->out_args[0].size = sizeof(outarg);
+ args->out_args[0].value = &outarg;
+
+ ret = fuse_simple_request(fm, args);
/* Translate ENOSYS, which shouldn't be returned from fs */
if (ret == -ENOSYS)
ret = -ENOTTY;
+ if (ret >= 0 && outarg->result == -ENOSYS)
+ outarg->result = -ENOTTY;
+
return ret;
}
@@ -264,13 +273,11 @@ long fuse_do_ioctl(struct file *file, un
}
ap.args.out_numargs = 2;
- ap.args.out_args[0].size = sizeof(outarg);
- ap.args.out_args[0].value = &outarg;
ap.args.out_args[1].size = out_size;
ap.args.out_pages = true;
ap.args.out_argvar = true;
- transferred = fuse_send_ioctl(fm, &ap.args);
+ transferred = fuse_send_ioctl(fm, &ap.args, &outarg);
err = transferred;
if (transferred < 0)
goto out;
@@ -399,12 +406,10 @@ static int fuse_priv_ioctl(struct inode
args.in_args[1].size = inarg.in_size;
args.in_args[1].value = ptr;
args.out_numargs = 2;
- args.out_args[0].size = sizeof(outarg);
- args.out_args[0].value = &outarg;
args.out_args[1].size = inarg.out_size;
args.out_args[1].value = ptr;
- err = fuse_send_ioctl(fm, &args);
+ err = fuse_send_ioctl(fm, &args, &outarg);
if (!err) {
if (outarg.result < 0)
err = outarg.result;