| Message ID | 20210622150852.1507204-2-vgoyal@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtiofsd: Add support to enable/disable posix acls | expand |
* 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. (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 >
On Mon, Jun 28, 2021 at 03:46:40PM +0100, Dr. David Alan Gilbert 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. > (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). [ CC Niklaus Rath] libfuse will need a fix as well once they move to 5.13 kernel headers. As of now they seem to be still working with 5.2 kernel headers. commit 249942f6411042c4af18bd10438da34801cce02b Author: Kirill Smelkov <kirr@nexedi.com> Date: Tue Jul 9 23:54:09 2019 +0300 Sync fuse_kernel.h with Linux 5.2 (#433) Thanks Vivek > > > 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 > > _______________________________________________ > Virtio-fs mailing list > Virtio-fs@redhat.com > https://listman.redhat.com/mailman/listinfo/virtio-fs >
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. 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 > >
* 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 > > > >
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);
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> --- tools/virtiofsd/fuse_common.h | 5 +++++ tools/virtiofsd/fuse_lowlevel.c | 7 ++++++- 2 files changed, 11 insertions(+), 1 deletion(-)