Re: [PATCH 01/16] btrfs: check for range correctness while locking or setting extent bits

[Filipe Manana <fdmanana@kernel.org>]

On Tue, Nov 15, 2022 at 6:13 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
> Since we will be working at the mercy of userspace, check if the range
> is valid and proceed to lock or set bits only if start < end.

At the mercy of user space, how? Can you be more detailed about what you mean?

Is this something you ran into, or is this just to prevent such cases
from happening?

>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/extent-io-tree.c | 6 ++++++
>  fs/btrfs/ordered-data.c   | 3 +++
>  2 files changed, 9 insertions(+)
>
> diff --git a/fs/btrfs/extent-io-tree.c b/fs/btrfs/extent-io-tree.c
> index 21fa15123af8..80657c820df4 100644
> --- a/fs/btrfs/extent-io-tree.c
> +++ b/fs/btrfs/extent-io-tree.c
> @@ -557,6 +557,9 @@ int __clear_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
>         int wake;
>         int delete = (bits & EXTENT_CLEAR_ALL_BITS);
>
> +       if (unlikely(start > end))
> +               return 0;

Having a start > end indicates a bug somewhere else, which should be
fixed in the caller.

That happened a few times in a distant past, one example:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ccccf3d67294714af2d72a6fd6fd7d73b01c9329

And that leads to nasty side effects much later on (inode eviction),
as described in that changelog.

If anything, we should assert here, and if assertions are disabled,
trigger a warning and return an error, not silently ignoring it.
Something like:

ASSERT(start < end);
if (WARN_ON(start >= end))
     return -WHAT_EVER_ERRNO;

Thanks.

> +
>         btrfs_debug_check_extent_io_range(tree, start, end);
>         trace_btrfs_clear_extent_bit(tree, start, end - start + 1, bits);
>
> @@ -979,6 +982,9 @@ static int __set_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
>         u64 last_end;
>         u32 exclusive_bits = (bits & EXTENT_LOCKED);
>
> +       if (unlikely(start > end))
> +               return 0;
> +
>         btrfs_debug_check_extent_io_range(tree, start, end);
>         trace_btrfs_set_extent_bit(tree, start, end - start + 1, bits);
>
> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> index 4bed0839b640..0a5512ed9a21 100644
> --- a/fs/btrfs/ordered-data.c
> +++ b/fs/btrfs/ordered-data.c
> @@ -1043,6 +1043,9 @@ void btrfs_lock_and_flush_ordered_range(struct btrfs_inode *inode, u64 start,
>         struct extent_state *cache = NULL;
>         struct extent_state **cachedp = &cache;
>
> +       if (unlikely(start > end))
> +               return;
> +
>         if (cached_state)
>                 cachedp = cached_state;
>
> --
> 2.35.3
>