| Message ID | 20181018131125.6303-2-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] orangefs: fix request_mask misuse | expand |
* Miklos Szeredi:
> #define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */
What about this? Isn't it similar to STATX_ALL in the sense that we
don't know yet what it will mean?
On Thu, Oct 18, 2018 at 3:15 PM, Florian Weimer <fw@deneb.enyo.de> wrote: > * Miklos Szeredi: > >> #define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */ > > What about this? Isn't it similar to STATX_ALL in the sense that we > don't know yet what it will mean? Kernel will return -EINVAL if request_mask contains STATX__RESERVED, so it's definitely different from other flag values. Specifying this in the UAPI sort of implies that other flag values will *not* need a struct statx expansion, so it's safe to pass in any random value not containing STATX__RESERVED on any past or future kernel and it will not write beyond the current struct statx boundary. Not sure if that's a useful thing or not, but it's not actively harmful, like the STATX_ALL flag. Thanks, Miklos
On Thu, Oct 18, 2018 at 4:32 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: > On Thu, Oct 18, 2018 at 3:15 PM, Florian Weimer <fw@deneb.enyo.de> wrote: >> * Miklos Szeredi: >> >>> #define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */ >> >> What about this? Isn't it similar to STATX_ALL in the sense that we >> don't know yet what it will mean? > > Kernel will return -EINVAL if request_mask contains STATX__RESERVED, > so it's definitely different from other flag values. > > Specifying this in the UAPI sort of implies that other flag values > will *not* need a struct statx expansion, so it's safe to pass in any > random value not containing STATX__RESERVED on any past or future > kernel and it will not write beyond the current struct statx boundary. > Not sure if that's a useful thing or not, but it's not actively > harmful, like the STATX_ALL flag. In other words, if STATX_ALL was defined as 0x7fffffff, then that would mean the same thing, and I wouldn't complain about it. Thanks, Miklos
On Thu, Oct 18, 2018 at 4:11 PM Miklos Szeredi <mszeredi@redhat.com> wrote: > > Constants of the *_ALL type can be actively harmful due to the fact that > developers will usually fail to consider the possible effects of future > changes to the definition. > > Remove STATX_ALL from the uapi, while no damage has been done yet. > Look. When Linus says "let's see if somebody notices" and referring to ABI it means sooner or later someone will upgrade to newer kernel and complain if something breaks. But what does it mean with UAPI change? How often do people re-build existing programs? I, for one, build master for my testing, but never install uapi headers from master. I just can't wrap my head around the backward compatibiltiy nightmare a change like this could create. How about just leave STATX_ALL in uapi header to rot forever mark it with a "deprecated" comment and #undef it out in include/linux/stat.h, so we can't use it in the kernel anymore. No use experimenting with pain. BTW, man page needs to be fixes as well, because man page promotes STATX_ALL. Thanks, Amir.
Miklos Szeredi <mszeredi@redhat.com> wrote: > Constants of the *_ALL type can be actively harmful due to the fact that > developers will usually fail to consider the possible effects of future > changes to the definition. > > Remove STATX_ALL from the uapi, while no damage has been done yet. You don't know that someone's not using it. It's been there a year and a half, long enough. So, regretfully, I don't think we can remove it at this point. David
* Amir Goldstein: > On Thu, Oct 18, 2018 at 4:11 PM Miklos Szeredi <mszeredi@redhat.com> wrote: >> >> Constants of the *_ALL type can be actively harmful due to the fact that >> developers will usually fail to consider the possible effects of future >> changes to the definition. >> >> Remove STATX_ALL from the uapi, while no damage has been done yet. >> > > Look. When Linus says "let's see if somebody notices" and referring to ABI > it means sooner or later someone will upgrade to newer kernel and complain > if something breaks. > > But what does it mean with UAPI change? How often do people > re-build existing programs? I, for one, build master for my > testing, but never install uapi headers from master. I just can't > wrap my head around the backward compatibiltiy nightmare a change > like this could create. So it appears that people use #ifdef STATX_ALL to check for struct statx availability. So the backwards compatibility impact is that you silently lose features in a consistent manner, which is very hard to spot. 8-( Probably not a good idea, then.
> On Oct 18, 2018, at 7:15 AM, Florian Weimer <fw@deneb.enyo.de> wrote: > > * Miklos Szeredi: > >> #define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */ > > What about this? Isn't it similar to STATX_ALL in the sense that we > don't know yet what it will mean? No, this means that this last bit will be used for increasing the size of the stx_mask field at some point in the future. IMHO, this is present as a reminder to any developer who is adding fields in the future not to use the last flag. Cheers, Andreas
On Oct 18, 2018, at 7:11 AM, Miklos Szeredi <mszeredi@redhat.com> wrote: > > Constants of the *_ALL type can be actively harmful due to the fact that > developers will usually fail to consider the possible effects of future > changes to the definition. > > Remove STATX_ALL from the uapi, while no damage has been done yet. > > We could keep something like this around in the kernel, but there's > actually no point, since all filesystems should be explicitly checking > flags that they support and not rely on the VFS masking unknown ones out: a > flag could be known to the VFS, yet not known to the filesystem (see > orangefs bug fixed in the previous patch). What is actually strange is that the vfs_getattr_nosec() code is setting stat->result_mask = STATX_BASIC_STATS; in the code before any of the filesystem ->getattr methods are called. That means it is up to the filesystems to actively *clear* flags from the result_mask as opposed to only *setting* flags for fields that it is filling in. That seems a bit backward to me? It looks like NFS is _probably_ doing the right thing, though it is still using the userspace-supplied request_mask as a mask for the bits being returned, but the saving grace is that result_mask is STATX_BASIC_STATS set by vfs_getattr_nosec() AFAICS. Looking at OCFS2, CIFS, GFS2, they are doing a full inode revalidation and returning the basic stats without looking at flags, request_mask, or result_mask at all, so I'd expect they could be more efficient (i.e. not revalidating the inode and possibly doing an RPC at all if only some minimal flags are requested, or if AT_STATX_FORCE_SYNC is not set). It looks like overlayfs, nfsd, and fuse at least (as statx callers) are often requesting a small subset of flags (e.g. STATX_INO, STATX_NLINK, STATX_ATIME | STATX_MTIME, etc.) so they could be more efficient if the underlying filesystems did only what was asked. Cheers, Andreas > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > Cc: David Howells <dhowells@redhat.com> > Cc: Michael Kerrisk <mtk.manpages@gmail.com> > --- > fs/stat.c | 1 - > include/uapi/linux/stat.h | 2 +- > samples/statx/test-statx.c | 2 +- > tools/include/uapi/linux/stat.h | 2 +- > 4 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/fs/stat.c b/fs/stat.c > index f8e6fb2c3657..8d297a279991 100644 > --- a/fs/stat.c > +++ b/fs/stat.c > @@ -73,7 +73,6 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat, > > memset(stat, 0, sizeof(*stat)); > stat->result_mask |= STATX_BASIC_STATS; > - request_mask &= STATX_ALL; > query_flags &= KSTAT_QUERY_FLAGS; > if (inode->i_op->getattr) > return inode->i_op->getattr(path, stat, request_mask, > diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h > index 7b35e98d3c58..370f09d92fa6 100644 > --- a/include/uapi/linux/stat.h > +++ b/include/uapi/linux/stat.h > @@ -148,7 +148,7 @@ struct statx { > #define STATX_BLOCKS 0x00000400U /* Want/got stx_blocks */ > #define STATX_BASIC_STATS 0x000007ffU /* The stuff in the normal stat struct */ > #define STATX_BTIME 0x00000800U /* Want/got stx_btime */ > -#define STATX_ALL 0x00000fffU /* All currently supported flags */ > + > #define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */ > > /* > diff --git a/samples/statx/test-statx.c b/samples/statx/test-statx.c > index d4d77b09412c..e354048dea3c 100644 > --- a/samples/statx/test-statx.c > +++ b/samples/statx/test-statx.c > @@ -211,7 +211,7 @@ int main(int argc, char **argv) > struct statx stx; > int ret, raw = 0, atflag = AT_SYMLINK_NOFOLLOW; > > - unsigned int mask = STATX_ALL; > + unsigned int mask = STATX_BASIC_STATS | STATX_BTIME; > > for (argv++; *argv; argv++) { > if (strcmp(*argv, "-F") == 0) { > diff --git a/tools/include/uapi/linux/stat.h b/tools/include/uapi/linux/stat.h > index 7b35e98d3c58..370f09d92fa6 100644 > --- a/tools/include/uapi/linux/stat.h > +++ b/tools/include/uapi/linux/stat.h > @@ -148,7 +148,7 @@ struct statx { > #define STATX_BLOCKS 0x00000400U /* Want/got stx_blocks */ > #define STATX_BASIC_STATS 0x000007ffU /* The stuff in the normal stat struct */ > #define STATX_BTIME 0x00000800U /* Want/got stx_btime */ > -#define STATX_ALL 0x00000fffU /* All currently supported flags */ > + > #define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */ > > /* > -- > 2.14.3 > Cheers, Andreas
diff --git a/fs/stat.c b/fs/stat.c index f8e6fb2c3657..8d297a279991 100644 --- a/fs/stat.c +++ b/fs/stat.c @@ -73,7 +73,6 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat, memset(stat, 0, sizeof(*stat)); stat->result_mask |= STATX_BASIC_STATS; - request_mask &= STATX_ALL; query_flags &= KSTAT_QUERY_FLAGS; if (inode->i_op->getattr) return inode->i_op->getattr(path, stat, request_mask, diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h index 7b35e98d3c58..370f09d92fa6 100644 --- a/include/uapi/linux/stat.h +++ b/include/uapi/linux/stat.h @@ -148,7 +148,7 @@ struct statx { #define STATX_BLOCKS 0x00000400U /* Want/got stx_blocks */ #define STATX_BASIC_STATS 0x000007ffU /* The stuff in the normal stat struct */ #define STATX_BTIME 0x00000800U /* Want/got stx_btime */ -#define STATX_ALL 0x00000fffU /* All currently supported flags */ + #define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */ /* diff --git a/samples/statx/test-statx.c b/samples/statx/test-statx.c index d4d77b09412c..e354048dea3c 100644 --- a/samples/statx/test-statx.c +++ b/samples/statx/test-statx.c @@ -211,7 +211,7 @@ int main(int argc, char **argv) struct statx stx; int ret, raw = 0, atflag = AT_SYMLINK_NOFOLLOW; - unsigned int mask = STATX_ALL; + unsigned int mask = STATX_BASIC_STATS | STATX_BTIME; for (argv++; *argv; argv++) { if (strcmp(*argv, "-F") == 0) { diff --git a/tools/include/uapi/linux/stat.h b/tools/include/uapi/linux/stat.h index 7b35e98d3c58..370f09d92fa6 100644 --- a/tools/include/uapi/linux/stat.h +++ b/tools/include/uapi/linux/stat.h @@ -148,7 +148,7 @@ struct statx { #define STATX_BLOCKS 0x00000400U /* Want/got stx_blocks */ #define STATX_BASIC_STATS 0x000007ffU /* The stuff in the normal stat struct */ #define STATX_BTIME 0x00000800U /* Want/got stx_btime */ -#define STATX_ALL 0x00000fffU /* All currently supported flags */ + #define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */ /*
Constants of the *_ALL type can be actively harmful due to the fact that developers will usually fail to consider the possible effects of future changes to the definition. Remove STATX_ALL from the uapi, while no damage has been done yet. We could keep something like this around in the kernel, but there's actually no point, since all filesystems should be explicitly checking flags that they support and not rely on the VFS masking unknown ones out: a flag could be known to the VFS, yet not known to the filesystem (see orangefs bug fixed in the previous patch). Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> Cc: David Howells <dhowells@redhat.com> Cc: Michael Kerrisk <mtk.manpages@gmail.com> --- fs/stat.c | 1 - include/uapi/linux/stat.h | 2 +- samples/statx/test-statx.c | 2 +- tools/include/uapi/linux/stat.h | 2 +- 4 files changed, 3 insertions(+), 4 deletions(-)