[PATCH] fuse: Apply flags2 only when userspace set the FUSE_INIT_EXT flag

[PATCH] fuse: Apply flags2 only when userspace set the FUSE_INIT_EXT flag

[Bernd Schubert]

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(-)

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)

Re: [PATCH] fuse: Apply flags2 only when userspace set the FUSE_INIT_EXT flag

[Miklos Szeredi]

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

Re: [PATCH] fuse: Apply flags2 only when userspace set the FUSE_INIT_EXT flag

[Bernd Schubert]

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.

Re: [PATCH] fuse: Apply flags2 only when userspace set the FUSE_INIT_EXT flag

[Vivek Goyal]

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

Re: [PATCH] fuse: Apply flags2 only when userspace set the FUSE_INIT_EXT flag

[JeffleXu]

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
> 
> 

-- 
Thanks,
Jeffle

Re: [PATCH] fuse: Apply flags2 only when userspace set the FUSE_INIT_EXT flag

[Miklos Szeredi]

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

Re: [PATCH] fuse: Apply flags2 only when userspace set the FUSE_INIT_EXT flag

[Vivek Goyal]

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

Re: [PATCH] fuse: Apply flags2 only when userspace set the FUSE_INIT_EXT flag

[Miklos Szeredi]

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

Re: [PATCH] fuse: Apply flags2 only when userspace set the FUSE_INIT_EXT flag

[Vivek Goyal]

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
>