Re: [PATCH v2 6/8] fuse: add extent map data structure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] fuse: sleep in RCU read-side critical section during backing files cleanup
- [Medium] fuse: missing overflow validation on ext->addr and ext->length
--

commit 791e33c26af3a517c133a125042b17d8f5ff57ec
Author: Miklos Szeredi <mszeredi@redhat.com>

fuse: add extent map data structure

Add support for creating and managing extent maps that map file regions to
dax device regions. Introduce FUSE_NOTIFY_BACKING_MAP for populating extent
maps from userspace.

> diff --git a/fs/fuse/backing.c b/fs/fuse/backing.c
> index c852f04989619..e954391d696d0 100644
> --- a/fs/fuse/backing.c
> +++ b/fs/fuse/backing.c
[ ... ]
> @@ -306,12 +310,36 @@ static void fuse_backing_rht_free(void *p, void *data)
>  
>  void fuse_backing_files_free(struct fuse_conn *fc)
>  {
[ ... ]
> +	/*
> +	 * extents are referencing other backings, put these refs before
> +	 * destroying the backings themselves
> +	 */
> +	rhashtable_walk_enter(&fc->backing_64_ht, &iter);
> +	rhashtable_walk_start(&iter);
> +	while ((fb = rhashtable_walk_next(&iter))) {
> +		if (IS_ERR(fb)) {
> +			if (PTR_ERR(fb) == -EAGAIN)
> +				continue;
> +			break;
> +		}
> +		if (fb->type == FUSE_BACKING_EXTMAP) {
> +			fuse_ext_map_destroy(&fb->extents);

[Severity: High]
Does this code sleep in an RCU read-side critical section?

rhashtable_walk_start() explicitly acquires the RCU read lock.
When this code drops the final reference to the underlying DAXDEV backing,
the call chain is:

fuse_ext_map_destroy()
  fuse_backing_put()
    fuse_backing_free()
      fs_put_dax()
        iput()

Since iput() can sleep, it appears this could trigger 'scheduling while
atomic' panics or lockdep warnings when unmounting a FUSE passthrough
connection.

> +			fb->extents.rb_node = NULL;
> +		}
> +	}
> +	rhashtable_walk_stop(&iter);
> +	rhashtable_walk_exit(&iter);
> +
> +	rhashtable_free_and_destroy(&fc->backing_64_ht, fuse_backing_rht_free, NULL);
>  }

> diff --git a/fs/fuse/ext_map.c b/fs/fuse/ext_map.c
> new file mode 100644
> index 0000000000000..ab01d678d08e5
> --- /dev/null
> +++ b/fs/fuse/ext_map.c
[ ... ]
> +static int fuse_add_extent(struct fuse_conn *fc, struct rb_root *extents,
> +			   struct fuse_extent *ext)
> +{
> +	struct fuse_iext *new_fie __free(kfree) = kzalloc_obj(*new_fie);
> +	struct rb_node *parent = NULL, **link = &extents->rb_node;
> +	loff_t end;
[ ... ]
> +	if (overflows_type(ext->offset, loff_t) ||
> +	    check_add_overflow(ext->offset, ext->length, &end))
> +		return fuse_EIO("offset overflow");
> +
> +	new_fie->start = ext->offset;
> +	new_fie->end = end;
> +	new_fie->backing_offset = ext->addr;

[Severity: Medium]
Is it possible for the physical address calculation to wrap around?

The bounds check above validates ext->offset, but appears to omit checking
ext->addr + ext->length for overflow.

Could a user-space process supplying an ext->addr near U64_MAX cause an
integer overflow on the physical address range calculation, resulting in a
non-contiguous backing mapping?

> +
> +	while (*link) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001150935.655979-1-mszeredi@redhat.com?part=6