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.m...@gmail.com>
> Signed-off-by: David Howells <dhowe...@redhat.com>
> cc: Steve French <smfre...@gmail.com>
> cc: Jeff Layton <jlay...@kernel.org>
> cc: linux-c...@vger.kernel.org
> cc: linux-cachefs@redhat.com
> ---
>
> 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 <jlay...@kernel.org>
--
Linux-cachefs mailing list
Linux-cachefs@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-cachefs
