| Message ID | 165002363635.1457422.5930635235733982079.stgit@localhost (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: Apply flags2 only when userspace set the FUSE_INIT_EXT flag | expand |
On Fri, 15 Apr 2022 at 13:54, Bernd Schubert <bschubert@ddn.com> wrote: > > This is just a safety precaution to avoid checking flags > on memory that was initialized on the user space side. > libfuse zeroes struct fuse_init_out outarg, but this is not > guranteed to be done in all implementations. Better is to > act on flags and to only apply flags2 when FUSE_INIT_EXT > is set. > > There is a risk with this change, though - it might break existing > user space libraries, which are already using flags2 without > setting FUSE_INIT_EXT. > > The corresponding libfuse patch is here > https://github.com/libfuse/libfuse/pull/662 > > > Signed-off-by: Bernd Schubert <bschubert@ddn.com> Agreed, this is a good change. Applied. Just one comment: please consider adding "Fixes:" and "Cc: <stable@....>" tags next time. I added them now. Thanks, Miklos
On 4/21/22 17:36, Miklos Szeredi wrote: > Agreed, this is a good change. Applied. > > Just one comment: please consider adding "Fixes:" and "Cc: > <stable@....>" tags next time. I added them now. Thank you! And sorry, sure, I will do next time.
On Thu, Apr 21, 2022 at 05:36:02PM +0200, Miklos Szeredi wrote: > On Fri, 15 Apr 2022 at 13:54, Bernd Schubert <bschubert@ddn.com> wrote: > > > > This is just a safety precaution to avoid checking flags > > on memory that was initialized on the user space side. > > libfuse zeroes struct fuse_init_out outarg, but this is not > > guranteed to be done in all implementations. Better is to > > act on flags and to only apply flags2 when FUSE_INIT_EXT > > is set. > > > > There is a risk with this change, though - it might break existing > > user space libraries, which are already using flags2 without > > setting FUSE_INIT_EXT. > > > > The corresponding libfuse patch is here > > https://github.com/libfuse/libfuse/pull/662 > > > > > > Signed-off-by: Bernd Schubert <bschubert@ddn.com> > > Agreed, this is a good change. Applied. > > Just one comment: please consider adding "Fixes:" and "Cc: > <stable@....>" tags next time. I added them now. I am afraid that this probably will break both C and rust version of virtiofsd. I had a quick look and I can't seem to find these implementations setting INIT_EXT flag in reply to init. I am travelling. Will check it more closely when I return next week. If virtiofsd implementations don't set INIT_EXT, I would rather prefer to not do this change and avoid breaking it. Thanks Vivek
On 4/24/22 6:49 PM, Bernd Schubert wrote: > I'm also traveling, but I had checked a bit the links you had given and even created github issue for the rust-fuse because it uses conflicting flags - seems to rely on non-upstream kernel. FYI at least the C version virtiofsd (git@github.com:qemu/qemu.git master) doesn't set FUSE_INIT_EXT on the reply to FUSE_INIT. I didn't check the Rust version, since I'm not familiar with Rust so far... I guess Vivek was referring to [1] when he mentioned the rust version of virtiofsd. This is the rust version developed by the RedHat. As for the "rust-fuse" Bernd Schubert mentioned, actually it's [2] developed by the Alibaba folks. We tried to make this fuse daemon support the per-inode DAX feature when the feature is still in the progress of upstreaming kernel. Later when the feature finally gets merged to mainline kernel, the position of the FUSE_HAS_INODE_DAX flag bit is a little different with the initial implementation. Sadly we forget to fix this, and the fuse daemon keeps using the flag bit different from the mainline version. Sorry for that. Thanks for pointing it out and we are going to fix it. Thanks. [1] https://gitlab.com/virtio-fs/virtiofsd [2] https://github.com/cloud-hypervisor/fuse-backend-rs > > Get Outlook for Android<https://aka.ms/AAb9ysg> > ________________________________ > From: Vivek Goyal <vgoyal@redhat.com> > Sent: Sunday, April 24, 2022 10:29:25 AM > To: Miklos Szeredi <miklos@szeredi.hu> > Cc: Bernd Schubert <bschubert@ddn.com>; linux-fsdevel@vger.kernel.org <linux-fsdevel@vger.kernel.org>; Jeffle Xu <jefflexu@linux.alibaba.com>; Dharmendra Singh <dsingh@ddn.com>; Dr. David Alan Gilbert <dgilbert@redhat.com>; German Maglione <gmaglione@redhat.com> > Subject: Re: [PATCH] fuse: Apply flags2 only when userspace set the FUSE_INIT_EXT flag > > On Thu, Apr 21, 2022 at 05:36:02PM +0200, Miklos Szeredi wrote: >> On Fri, 15 Apr 2022 at 13:54, Bernd Schubert <bschubert@ddn.com> wrote: >>> >>> This is just a safety precaution to avoid checking flags >>> on memory that was initialized on the user space side. >>> libfuse zeroes struct fuse_init_out outarg, but this is not >>> guranteed to be done in all implementations. Better is to >>> act on flags and to only apply flags2 when FUSE_INIT_EXT >>> is set. >>> >>> There is a risk with this change, though - it might break existing >>> user space libraries, which are already using flags2 without >>> setting FUSE_INIT_EXT. >>> >>> The corresponding libfuse patch is here >>> https://github.com/libfuse/libfuse/pull/662 >>> >>> >>> Signed-off-by: Bernd Schubert <bschubert@ddn.com> >> >> Agreed, this is a good change. Applied. >> >> Just one comment: please consider adding "Fixes:" and "Cc: >> <stable@....>" tags next time. I added them now. > > I am afraid that this probably will break both C and rust version of > virtiofsd. I had a quick look and I can't seem to find these > implementations setting INIT_EXT flag in reply to init. > > I am travelling. Will check it more closely when I return next week. > If virtiofsd implementations don't set INIT_EXT, I would rather prefer > to not do this change and avoid breaking it. > > Thanks > Vivek > >
On Sun, 24 Apr 2022 at 10:29, Vivek Goyal <vgoyal@redhat.com> wrote: > > On Thu, Apr 21, 2022 at 05:36:02PM +0200, Miklos Szeredi wrote: > > On Fri, 15 Apr 2022 at 13:54, Bernd Schubert <bschubert@ddn.com> wrote: > > > > > > This is just a safety precaution to avoid checking flags > > > on memory that was initialized on the user space side. > > > libfuse zeroes struct fuse_init_out outarg, but this is not > > > guranteed to be done in all implementations. Better is to > > > act on flags and to only apply flags2 when FUSE_INIT_EXT > > > is set. > > > > > > There is a risk with this change, though - it might break existing > > > user space libraries, which are already using flags2 without > > > setting FUSE_INIT_EXT. > > > > > > The corresponding libfuse patch is here > > > https://github.com/libfuse/libfuse/pull/662 > > > > > > > > > Signed-off-by: Bernd Schubert <bschubert@ddn.com> > > > > Agreed, this is a good change. Applied. > > > > Just one comment: please consider adding "Fixes:" and "Cc: > > <stable@....>" tags next time. I added them now. > > I am afraid that this probably will break both C and rust version of > virtiofsd. I had a quick look and I can't seem to find these > implementations setting INIT_EXT flag in reply to init. > > I am travelling. Will check it more closely when I return next week. > If virtiofsd implementations don't set INIT_EXT, I would rather prefer > to not do this change and avoid breaking it. Okay, let's postpone this kernel patch until libfuse and virtiofsd implementations are updated. Thanks, Miklos
On Mon, Apr 25, 2022 at 10:09:48AM +0200, Miklos Szeredi wrote: > On Sun, 24 Apr 2022 at 10:29, Vivek Goyal <vgoyal@redhat.com> wrote: > > > > On Thu, Apr 21, 2022 at 05:36:02PM +0200, Miklos Szeredi wrote: > > > On Fri, 15 Apr 2022 at 13:54, Bernd Schubert <bschubert@ddn.com> wrote: > > > > > > > > This is just a safety precaution to avoid checking flags > > > > on memory that was initialized on the user space side. > > > > libfuse zeroes struct fuse_init_out outarg, but this is not > > > > guranteed to be done in all implementations. Better is to > > > > act on flags and to only apply flags2 when FUSE_INIT_EXT > > > > is set. > > > > > > > > There is a risk with this change, though - it might break existing > > > > user space libraries, which are already using flags2 without > > > > setting FUSE_INIT_EXT. > > > > > > > > The corresponding libfuse patch is here > > > > https://github.com/libfuse/libfuse/pull/662 > > > > > > > > > > > > Signed-off-by: Bernd Schubert <bschubert@ddn.com> > > > > > > Agreed, this is a good change. Applied. > > > > > > Just one comment: please consider adding "Fixes:" and "Cc: > > > <stable@....>" tags next time. I added them now. > > > > I am afraid that this probably will break both C and rust version of > > virtiofsd. I had a quick look and I can't seem to find these > > implementations setting INIT_EXT flag in reply to init. > > > > I am travelling. Will check it more closely when I return next week. > > If virtiofsd implementations don't set INIT_EXT, I would rather prefer > > to not do this change and avoid breaking it. > > Okay, let's postpone this kernel patch until libfuse and virtiofsd > implementations are updated. Ok. I will work on fixing virtiofsd implementation. Even if we fix it, then older versions will still be broken with newer kernels. I am wondering, which clients are not setting flags2 to zero. And if they are not setting it to zero, it sounds like a bug to me in fuse servers instead and should probably be fixed there without breaking things for existing users. Agree that it probably is a nice change if we had introduced this in the beginning itself. Its like extra saftey net. But now if we add it, it will break things which is not nice. So at this point of time, it probably is better to fix fuse servers instead and set ->flags2 to zero, IMHO. Thanks Vivek
On Tue, 26 Apr 2022 at 15:01, Vivek Goyal <vgoyal@redhat.com> wrote: > Agree that it probably is a nice change if we had introduced this in the > beginning itself. Its like extra saftey net. But now if we add it, it > will break things which is not nice. So at this point of time, it probably > is better to fix fuse servers instead and set ->flags2 to zero, IMHO. I think the question is whether the "unfixed" virtiofsd implementations made it into any sort of release or not. If not, then I think it's fine to break unreleased versions, since they are ephemeral anyway. Thanks, Miklos
On Tue, Apr 26, 2022 at 03:13:50PM +0200, Miklos Szeredi wrote: > On Tue, 26 Apr 2022 at 15:01, Vivek Goyal <vgoyal@redhat.com> wrote: > > > Agree that it probably is a nice change if we had introduced this in the > > beginning itself. Its like extra saftey net. But now if we add it, it > > will break things which is not nice. So at this point of time, it probably > > is better to fix fuse servers instead and set ->flags2 to zero, IMHO. > > I think the question is whether the "unfixed" virtiofsd > implementations made it into any sort of release or not. Existing unfixed versions are already released in various releses. C version of virtiofsd is already being used in RHEL8 release and some fedora releases. And rust version of virtiofsd is supposed to be in RHEL9 beta. Hence if we change it now, it is possible older virtiofsd (unfixed one) is running on host and trying to boot a newer guest kernel and that leads to breaking things. Thanks Vivek > > If not, then I think it's fine to break unreleased versions, since > they are ephemeral anyway. > > Thanks, > Miklos >
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c index 9ee36aa73251..8115a06d5fbb 100644 --- a/fs/fuse/inode.c +++ b/fs/fuse/inode.c @@ -1115,7 +1115,10 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args, process_init_limits(fc, arg); if (arg->minor >= 6) { - u64 flags = arg->flags | (u64) arg->flags2 << 32; + u64 flags = arg->flags; + + if (flags & FUSE_INIT_EXT) + flags |= (u64) arg->flags2 << 32; ra_pages = arg->max_readahead / PAGE_SIZE; if (flags & FUSE_ASYNC_READ)
This is just a safety precaution to avoid checking flags on memory that was initialized on the user space side. libfuse zeroes struct fuse_init_out outarg, but this is not guranteed to be done in all implementations. Better is to act on flags and to only apply flags2 when FUSE_INIT_EXT is set. There is a risk with this change, though - it might break existing user space libraries, which are already using flags2 without setting FUSE_INIT_EXT. The corresponding libfuse patch is here https://github.com/libfuse/libfuse/pull/662 Signed-off-by: Bernd Schubert <bschubert@ddn.com> --- fs/fuse/inode.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)