Re: [PATCH V11 3/9] famfs_fuse: Plumb the GET_FMAP message/response

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] famfs_meta_free() fails to free metadata memory and clear the pointer, causing a memory leak, and famfs_meta_set() has an incorrect return type.
- [Low] The struct fuse_famfs_fmap_header is incomplete and undefined, causing a build failure.
- [Low] Lockless read of shared pointer fi->famfs_meta without READ_ONCE().
--

--- Patch [3]: [PATCH V11 3/9] famfs_fuse: Plumb the GET_FMAP message/response ---
commit d837c9b994af8cb48d023d1ff3ded4fc24c04e86
Author: John Groves <[email protected]>

famfs_fuse: Plumb the GET_FMAP message/response
    
On completion of an OPEN in famfs mode, issue a GET_FMAP request to the
server to retrieve the file's file-offset-to-dax map (fmap) and cache it
on the fuse_inode (fi->famfs_meta). Once the map is cached, read, write
and mmap are resolved directly to dax with no further upcalls.

> diff --git a/fs/fuse/famfs.c b/fs/fuse/famfs.c
> new file mode 100644
> index 0000000000000..80e6640ac970b
> --- /dev/null
> +++ b/fs/fuse/famfs.c
[ ... ]
> @@ -0,0 +1,134 @@
> +int fuse_get_fmap(struct fuse_mount *fm, struct inode *inode)
> +{
> +	struct fuse_inode *fi = get_fuse_inode(inode);
> +	u64 nodeid = get_node_id(inode);
> +	size_t bufsize = FMAP_BUFSIZE_INIT;
> +	void *fmap_buf = NULL;
> +	ssize_t fmap_size;
> +	int attempt;
> +	int rc;
> +
> +	/* Don't retrieve if we already have the famfs metadata */
> +	if (fi->famfs_meta)
            ^^^^^^^^^^^^^^

[Severity: Low]
Does this lockless read need a READ_ONCE() wrapper?  Without it, we might risk
compiler optimization issues, especially since this is a shared pointer that
can be concurrently modified by another thread.

> +		return 0;
> +
> +	/*
> +	 * The fmap size is not known in advance. Start with a modest buffer and,
> +	 * if the server reports (via the returned header's fmap_size) that the
> +	 * whole fmap did not fit, reallocate exactly that size and retry once.
> +	 * The server learns our buffer size from the request's
> +	 * fuse_getxattr_in.size (GETXATTR-style size probe).
> +	 */
> +	for (attempt = 0; ; attempt++) {
> +		struct fuse_getxattr_in in = { .size = bufsize };
> +		struct fuse_famfs_fmap_header *fmh;
> +		u32 required;
> +
> +		FUSE_ARGS(args);
[ ... ]
> +		fmap_size = rc;
> +
> +		/* Need at least a header to learn the required size */
> +		if (fmap_size < (ssize_t)sizeof(*fmh)) {
                                          ^^^^^^^^^^^

[Severity: Low]
Will this cause a build failure?  It looks like struct fuse_famfs_fmap_header
is declared as a pointer but never defined anywhere in the source tree, so
its size cannot be evaluated here.

> +			pr_err("%s: short fmap reply %zd\n", __func__, fmap_size);
> +			kvfree(fmap_buf);
> +			return -EIO;
> +		}
> +
> +		fmh = fmap_buf;
> +		required = fmh->fmap_size;
                             ^^^^^^^^^^^^

[Severity: Low]
this isn't a bug, but similar to the above, this dereference of fmh->fmap_size
might fail to compile because the struct is incomplete.

[ ... ]
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 9c354118c931e..5bacc50986203 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
[ ... ]
> @@ -1339,4 +1344,59 @@ extern void fuse_sysctl_unregister(void);
> +/* Set fi->famfs_meta iff the current value is NULL */
> +static inline struct fuse_backing *famfs_meta_set(struct fuse_inode *fi,
> +						  void *meta)
> +{
> +	return cmpxchg(&fi->famfs_meta, NULL, meta);
> +}
> +
> +static inline void famfs_meta_free(struct fuse_inode *fi)
> +{
> +	famfs_meta_set(fi, NULL);
        ^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this code leak the metadata?  When called from fuse_free_inode() on inode
teardown to free the metadata, fi->famfs_meta is non-NULL. But famfs_meta_set()
uses cmpxchg() against NULL, so the exchange will fail and the pointer will
never be cleared, bypassing any actual free operation.

Also, famfs_meta_set() returns a struct fuse_backing *, which looks like a
copy-paste error from passthrough code.

> +}
> +
> +static inline int fuse_file_famfs(struct fuse_inode *fi)
> +{
> +	return (READ_ONCE(fi->famfs_meta) != NULL);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019f7d9fbe81-6cb16662-2522-47ea-a152-fab0ee3d9b35-000000@email.amazonses.com?part=3