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

[Vivek Goyal <vgoyal@redhat.com>]

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