Re: [PATCH] cachefiles: Fix volume coherency attribute

[Jeff Layton]

On Tue, 2022-03-08 at 21:52 +0000, David Howells wrote:
> A network filesystem may set coherency data on a volume cookie, and if
> given, cachefiles will store this in an xattr on the directory in the cache
> corresponding to the volume.
> 
> The function that sets the xattr just stores the contents of the volume
> coherency buffer directly into the xattr, with nothing added; the checking
> function, on the other hand, has a cut'n'paste error whereby it tries to
> interpret the xattr contents as would be the xattr on an ordinary file
> (using the cachefiles_xattr struct).  This results in a failure to match
> the coherency data because the buffer ends up being shifted by 18 bytes.
> 
> Fix this by defining a structure specifically for the volume xattr and
> making both the setting and checking functions use it.
> 
> Since the volume coherency doesn't work if used, take the opportunity to
> insert a reserved field for future use, set it to 0 and check that it is 0.
> Log mismatch through the appropriate tracepoint.
> 
> Note that this only affects cifs; 9p, afs, ceph and nfs don't use the
> volume coherency data at the moment.
> 
> Fixes: 32e150037dce ("fscache, cachefiles: Store the volume coherency data")
> Reported-by: Rohith Surabattula <rohiths.msft@xxxxxxxxx>
> Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
> cc: Steve French <smfrench@xxxxxxxxx>
> cc: Jeff Layton <jlayton@xxxxxxxxxx>
> cc: linux-cifs@xxxxxxxxxxxxxxx
> cc: linux-cachefs@xxxxxxxxxx
> ---
> 
>  fs/cachefiles/xattr.c             |   23 ++++++++++++++++++++---
>  include/trace/events/cachefiles.h |    2 ++
>  2 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/cachefiles/xattr.c b/fs/cachefiles/xattr.c
> index 83f41bd0c3a9..35465109d9c4 100644
> --- a/fs/cachefiles/xattr.c
> +++ b/fs/cachefiles/xattr.c
> @@ -28,6 +28,11 @@ struct cachefiles_xattr {
>  static const char cachefiles_xattr_cache[] =
>  	XATTR_USER_PREFIX "CacheFiles.cache";
>  
> +struct cachefiles_vol_xattr {
> +	__be32	reserved;	/* Reserved, should be 0 */
> +	__u8	data[];		/* netfs volume coherency data */
> +} __packed;
> +
>  /*
>   * set the state xattr on a cache file
>   */
> @@ -185,6 +190,7 @@ void cachefiles_prepare_to_write(struct fscache_cookie *cookie)
>   */
>  bool cachefiles_set_volume_xattr(struct cachefiles_volume *volume)
>  {
> +	struct cachefiles_vol_xattr *buf;
>  	unsigned int len = volume->vcookie->coherency_len;
>  	const void *p = volume->vcookie->coherency;
>  	struct dentry *dentry = volume->dentry;
> @@ -192,10 +198,17 @@ bool cachefiles_set_volume_xattr(struct cachefiles_volume *volume)
>  
>  	_enter("%x,#%d", volume->vcookie->debug_id, len);
>  
> +	len += sizeof(*buf);
> +	buf = kmalloc(len, GFP_KERNEL);
> +	if (!buf)
> +		return false;
> +	buf->reserved = cpu_to_be32(0);
> +	memcpy(buf->data, p, len);
> +
>  	ret = cachefiles_inject_write_error();
>  	if (ret == 0)
>  		ret = vfs_setxattr(&init_user_ns, dentry, cachefiles_xattr_cache,
> -				   p, len, 0);
> +				   buf, len, 0);
>  	if (ret < 0) {
>  		trace_cachefiles_vfs_error(NULL, d_inode(dentry), ret,
>  					   cachefiles_trace_setxattr_error);
> @@ -209,6 +222,7 @@ bool cachefiles_set_volume_xattr(struct cachefiles_volume *volume)
>  					       cachefiles_coherency_vol_set_ok);
>  	}
>  
> +	kfree(buf);
>  	_leave(" = %d", ret);
>  	return ret == 0;
>  }
> @@ -218,7 +232,7 @@ bool cachefiles_set_volume_xattr(struct cachefiles_volume *volume)
>   */
>  int cachefiles_check_volume_xattr(struct cachefiles_volume *volume)
>  {
> -	struct cachefiles_xattr *buf;
> +	struct cachefiles_vol_xattr *buf;
>  	struct dentry *dentry = volume->dentry;
>  	unsigned int len = volume->vcookie->coherency_len;
>  	const void *p = volume->vcookie->coherency;
> @@ -228,6 +242,7 @@ int cachefiles_check_volume_xattr(struct cachefiles_volume *volume)
>  
>  	_enter("");
>  
> +	len += sizeof(*buf);
>  	buf = kmalloc(len, GFP_KERNEL);
>  	if (!buf)
>  		return -ENOMEM;
> @@ -245,7 +260,9 @@ int cachefiles_check_volume_xattr(struct cachefiles_volume *volume)
>  					"Failed to read xattr with error %zd", xlen);
>  		}
>  		why = cachefiles_coherency_vol_check_xattr;
> -	} else if (memcmp(buf->data, p, len) != 0) {
> +	} else if (buf->reserved != cpu_to_be32(0)) {
> +		why = cachefiles_coherency_vol_check_resv;
> +	} else if (memcmp(buf->data, p, len - sizeof(*buf)) != 0) {
>  		why = cachefiles_coherency_vol_check_cmp;
>  	} else {
>  		why = cachefiles_coherency_vol_check_ok;
> diff --git a/include/trace/events/cachefiles.h b/include/trace/events/cachefiles.h
> index 002d0ae4f9bc..311c14a20e70 100644
> --- a/include/trace/events/cachefiles.h
> +++ b/include/trace/events/cachefiles.h
> @@ -56,6 +56,7 @@ enum cachefiles_coherency_trace {
>  	cachefiles_coherency_set_ok,
>  	cachefiles_coherency_vol_check_cmp,
>  	cachefiles_coherency_vol_check_ok,
> +	cachefiles_coherency_vol_check_resv,
>  	cachefiles_coherency_vol_check_xattr,
>  	cachefiles_coherency_vol_set_fail,
>  	cachefiles_coherency_vol_set_ok,
> @@ -139,6 +140,7 @@ enum cachefiles_error_trace {
>  	EM(cachefiles_coherency_set_ok,		"SET ok  ")		\
>  	EM(cachefiles_coherency_vol_check_cmp,	"VOL BAD cmp ")		\
>  	EM(cachefiles_coherency_vol_check_ok,	"VOL OK      ")		\
> +	EM(cachefiles_coherency_vol_check_resv,	"VOL BAD resv")	\
>  	EM(cachefiles_coherency_vol_check_xattr,"VOL BAD xatt")		\
>  	EM(cachefiles_coherency_vol_set_fail,	"VOL SET fail")		\
>  	E_(cachefiles_coherency_vol_set_ok,	"VOL SET ok  ")
> 
> 

Looks good.

Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>