| Message ID | 07534e31d822b5c08609c72e5a2e8054604765d9.1668530684.git.rgoldwyn@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Lock extents before pages | expand |
On 15.11.22 19:02, Goldwyn Rodrigues wrote: > + if (unlikely(start > end)) > + return 0; I wonder if returning 0 really is the best value here, instead of -EINVAL or similar? Also does unlikely really provide any benefit here?
On 11:09 17/11, Johannes Thumshirn wrote: > On 15.11.22 19:02, Goldwyn Rodrigues wrote: > > + if (unlikely(start > end)) > > + return 0; > > I wonder if returning 0 really is the best value here, > instead of -EINVAL or similar? or ERANGE to be more precise? > > Also does unlikely really provide any benefit here? It is more to tell the compiler that this scenario is unlikely and would move the instructions during optimizing branch prediction.
On 22.11.22 18:17, Goldwyn Rodrigues wrote: > On 11:09 17/11, Johannes Thumshirn wrote: >> On 15.11.22 19:02, Goldwyn Rodrigues wrote: >>> + if (unlikely(start > end)) >>> + return 0; >> >> I wonder if returning 0 really is the best value here, >> instead of -EINVAL or similar? > > or ERANGE to be more precise? Yup.
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 >
On 13:12 23/11, Filipe Manana wrote: > 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? An example is: A user can perform read() with zero length. Since we are now locking extent before pages, this leads to set/clear bit with start>end. > > Is this something you ran into, or is this just to prevent such cases > from happening? This is a preparatory patch and encountered while I was working on the series.
On Tue, Nov 15, 2022 at 12:00:19PM -0600, Goldwyn Rodrigues 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. > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> Echo'ing Johannes' comments as well, I'd rather this be an ASSERT() and we catch it. Thanks, Josef
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; + 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;
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. 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(+)