[PATCH 1/3] orangefs: fix request_mask misuse

Orangefs only handles STATX_BASIC_STATS in its getattr implementation, so
mask off all other flags.  Not doing so results in statx(2) forcing a
refresh of cached attributes on any other requested flag (i.e. STATX_BTIME
currently) due to the following test in orangefs_inode_getattr():

  (request_mask & orangefs_inode->getattr_mask) == request_mask

Also clean up gratuitous uses of STATX_ALL.

Signed-off-by: Miklos Szeredi <redacted>
Cc: Mike Marshall <hubcap@omnibond.com>
Cc: Martin Brandenburg <martin@omnibond.com>
---
 fs/orangefs/inode.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
index 31932879b716..bd7f15a831dc 100644
--- a/fs/orangefs/inode.c
+++ b/fs/orangefs/inode.c

@@ -256,7 +256,8 @@ int orangefs_getattr(const struct path *path, struct kstat *stat,
 		     "orangefs_getattr: called on %pd\n",
 		     path->dentry);
 
-	ret = orangefs_inode_getattr(inode, 0, 0, request_mask);
+	ret = orangefs_inode_getattr(inode, 0, 0,
+				     request_mask & STATX_BASIC_STATS);
 	if (ret == 0) {
 		generic_fillattr(inode, stat);
 

@@ -408,7 +409,7 @@ struct inode *orangefs_iget(struct super_block *sb,
 	if (!inode || !(inode->i_state & I_NEW))
 		return inode;
 
-	error = orangefs_inode_getattr(inode, 1, 1, STATX_ALL);
+	error = orangefs_inode_getattr(inode, 1, 1, STATX_BASIC_STATS);
 	if (error) {
 		iget_failed(inode);
 		return ERR_PTR(error);

@@ -453,7 +454,7 @@ struct inode *orangefs_new_inode(struct super_block *sb, struct inode *dir,
 	orangefs_set_inode(inode, ref);
 	inode->i_ino = hash;	/* needed for stat etc */
 
-	error = orangefs_inode_getattr(inode, 1, 1, STATX_ALL);
+	error = orangefs_inode_getattr(inode, 1, 1, STATX_BASIC_STATS);
 	if (error)
 		goto out_iput;
 

-- 
2.14.3

FUSE will want to know if stx_attributes is interesting or not, because
there's a non-zero cost of retreiving it.

This is more of a "want" flag, since stx_attributes_mask already indicates
whether we "got" stx_attributes or not.  So for now we can just deal with
this flag in the generic code.

Signed-off-by: Miklos Szeredi <redacted>
Cc: David Howells <dhowells@redhat.com>
Cc: Michael Kerrisk <redacted>
---
 fs/stat.c                       | 3 +++
 include/uapi/linux/stat.h       | 1 +
 samples/statx/test-statx.c      | 2 +-
 tools/include/uapi/linux/stat.h | 1 +
 4 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/stat.c b/fs/stat.c
index 8d297a279991..6bf86d57e2e3 100644
--- a/fs/stat.c
+++ b/fs/stat.c

@@ -535,6 +535,9 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
 	tmp.stx_size = stat->size;
 	tmp.stx_blocks = stat->blocks;
 	tmp.stx_attributes_mask = stat->attributes_mask;
+	/* Having anything in attributes_mask means attributes are valid. */
+	if (tmp.stx_attributes_mask)
+		tmp.stx_mask |= STATX_ATTRIBUTES;
 	tmp.stx_atime.tv_sec = stat->atime.tv_sec;
 	tmp.stx_atime.tv_nsec = stat->atime.tv_nsec;
 	tmp.stx_btime.tv_sec = stat->btime.tv_sec;

diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
index 370f09d92fa6..aef0aba5dfe7 100644
--- a/include/uapi/linux/stat.h
+++ b/include/uapi/linux/stat.h

@@ -148,6 +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_ATTRIBUTES	0x00001000U	/* Want/got stx_attributes */
 
 #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 e354048dea3c..deef9a68ff68 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_BASIC_STATS | STATX_BTIME;
+	unsigned int mask = STATX_BASIC_STATS | STATX_BTIME | STATX_ATTRIBUTES;
 
 	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 370f09d92fa6..aef0aba5dfe7 100644
--- a/tools/include/uapi/linux/stat.h
+++ b/tools/include/uapi/linux/stat.h

@@ -148,6 +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_ATTRIBUTES	0x00001000U	/* Want/got stx_attributes */
 
 #define STATX__RESERVED		0x80000000U	/* Reserved for future struct statx expansion */
 

-- 
2.14.3

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 <redacted>
Cc: David Howells <dhowells@redhat.com>
Cc: Michael Kerrisk <redacted>
---
 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

On Thu, Oct 18, 2018 at 4:11 PM Miklos Szeredi [off-list ref] 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 [off-list ref] 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

Miklos Szeredi [off-list ref] wrote:

+	/* Having anything in attributes_mask means attributes are valid. */
+	if (tmp.stx_attributes_mask)
+		tmp.stx_mask |= STATX_ATTRIBUTES;

That would be superfluous, since userspace can make this check too.

Note that fsinfo() might inform you better - if and when it arrives.

David

On Thu, Oct 18, 2018 at 03:11:23PM +0200, Miklos Szeredi wrote:

Orangefs only handles STATX_BASIC_STATS in its getattr implementation, so
mask off all other flags.  Not doing so results in statx(2) forcing a
refresh of cached attributes on any other requested flag (i.e. STATX_BTIME
currently) due to the following test in orangefs_inode_getattr():

  (request_mask & orangefs_inode->getattr_mask) == request_mask

Also clean up gratuitous uses of STATX_ALL.

Signed-off-by: Miklos Szeredi <redacted>
Cc: Mike Marshall <hubcap@omnibond.com>
Cc: Martin Brandenburg <martin@omnibond.com>

Reviewed-by: Martin Brandenburg <martin@omnibond.com>

On Oct 18, 2018, at 7:11 AM, Miklos Szeredi [off-list ref] wrote:

FUSE will want to know if stx_attributes is interesting or not, because
there's a non-zero cost of retreiving it.

This is more of a "want" flag, since stx_attributes_mask already indicates
whether we "got" stx_attributes or not.  So for now we can just deal with
this flag in the generic code.

Signed-off-by: Miklos Szeredi <redacted>
Cc: David Howells <dhowells@redhat.com>
Cc: Michael Kerrisk <redacted>

Reviewed-by: Andreas Dilger <redacted>

Cheers, Andreas

On Oct 18, 2018, at 7:11 AM, Miklos Szeredi [off-list ref] 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

Cheers, Andreas