Re: [Virtio-fs] [PATCH v7 1/7] virtiofsd: Fix fuse setxattr() API change issue

[Dr. David Alan Gilbert]

* Greg Kurz (groug@kaod.org) wrote:
> On Mon, 28 Jun 2021 15:46:40 +0100
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > With kernel header updates fuse_setxattr_in struct has grown in size.
> > > But this new struct size only takes affect if user has opted in
> > > for fuse feature FUSE_SETXATTR_EXT otherwise fuse continues to
> > > send "fuse_setxattr_in" of older size. Older size is determined
> > > by FUSE_COMPAT_SETXATTR_IN_SIZE.
> > > 
> > > Fix this. If we have not opted in for FUSE_SETXATTR_EXT, then
> > > expect that we will get fuse_setxattr_in of size 
> > > FUSE_COMPAT_SETXATTR_IN_SIZE
> > > and not sizeof(struct fuse_sexattr_in).
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > 
> > Yeh it's a bit of a grim fix, but I think it's the best we can do, and
> > we need to get it in since the headers have already been merged.
> 
> You can also add:
> 
> Fixes: 278f064e4524 ("Update Linux headers to 5.13-rc4")
> 
> because this is basically what happened : this commit exposes the API
> breakage.
> 
> This is kinda problematic : linux kernel headers are updated globally,
> i.e. an header update merged by some other subsystem will unknowingly
> break virtiofsd each time we face a similar situation.
> 
> We could cope with it if the code was adapted to API changes when
> needed, e.g. this patch squashed into 278f064e4524 . It doesn't
> seem that can be achievable without some automation to detect
> buggy situations (e.g. code depends on the size of a structure).
> And even with that, it would still cause the subsystem that
> needs the header update to depend on other subsystems to
> fix the breakage.
> 
> Another possibility could maybe to stop doing global updates and
> let each subsystem handle the kernel headers it needs.
> 
> OR we could avoid breaking the API in the kernel in the first
> place.

That would be my preference!

Dave

> Thoughts ?
> 
> Anyway, the fix is good:
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > (I don't think libfuse has a fix for this in yet itself, but it might
> > survive because it doesn't bother copying the data like we do with
> > mbuf).
> > 
> > 
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > > ---
> > >  tools/virtiofsd/fuse_common.h   | 5 +++++
> > >  tools/virtiofsd/fuse_lowlevel.c | 7 ++++++-
> > >  2 files changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
> > > index fa9671872e..0c2665b977 100644
> > > --- a/tools/virtiofsd/fuse_common.h
> > > +++ b/tools/virtiofsd/fuse_common.h
> > > @@ -372,6 +372,11 @@ struct fuse_file_info {
> > >   */
> > >  #define FUSE_CAP_HANDLE_KILLPRIV_V2 (1 << 28)
> > >  
> > > +/**
> > > + * Indicates that file server supports extended struct fuse_setxattr_in
> > > + */
> > > +#define FUSE_CAP_SETXATTR_EXT (1 << 29)
> > > +
> > >  /**
> > >   * Ioctl flags
> > >   *
> > > diff --git a/tools/virtiofsd/fuse_lowlevel.c 
> > > b/tools/virtiofsd/fuse_lowlevel.c
> > > index 7fe2cef1eb..c2b6ff1686 100644
> > > --- a/tools/virtiofsd/fuse_lowlevel.c
> > > +++ b/tools/virtiofsd/fuse_lowlevel.c
> > > @@ -1419,8 +1419,13 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t 
> > > nodeid,
> > >      struct fuse_setxattr_in *arg;
> > >      const char *name;
> > >      const char *value;
> > > +    bool setxattr_ext = req->se->conn.want & FUSE_CAP_SETXATTR_EXT;
> > >  
> > > -    arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> > > +    if (setxattr_ext) {
> > > +        arg = fuse_mbuf_iter_advance(iter, sizeof(*arg));
> > > +    } else {
> > > +        arg = fuse_mbuf_iter_advance(iter, FUSE_COMPAT_SETXATTR_IN_SIZE);
> > > +    }
> > >      name = fuse_mbuf_iter_advance_str(iter);
> > >      if (!arg || !name) {
> > >          fuse_reply_err(req, EINVAL);
> > > -- 
> > > 2.25.4
> > > 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK