[PATCH 0/5] Speedups and check_setget_bounds reporting updates

[PATCH 0/5] Speedups and check_setget_bounds reporting updates

[David Sterba]

This is a followup to [1] disabling asserts by default. There's some
performance hit involved because of check_setget_bounds that's called
for any extent buffer item read/write. Trading correctness for speed is
should be considered but in general correctness should be favored.

I've optimized the helper so it should have negligible impact (one
inlined branch instead of function call and 2 branches), I haven't
measured that yet though.

How the checks and reporting works:

- any function generated by BTRFS_SETGET_FUNCS calls check_setget_bounds
  and verifies if the reguested bytes are within the extent buffer

- if the checks fail nothing is read or written to the buffer
  - set a bit in fs_info::fs_state so we know something happened
  - store information about the failed range into fs_info for later

- as the setget helpers aren't checked for errors, there are a few
  points where the transaction is ended/aborted once the bit is set

I've tested that in fstests with inserted failure [2] after each 100K
operations, this allows tests to proceed but fail eventually. This makes
i up to btrfs/020 and hits assert in warn_about_uncommitted_trans.

Possible improvements:

- there's a similar check/report for read_extent_buffer so that could
  also set the bit and fail fast

- can be used as base for the shutdown logic

[1] https://lore.kernel.org/linux-btrfs/20200724164147.39925-1-josef@toxicpanda.com

[2]
--- a/fs/btrfs/struct-funcs.c
+++ b/fs/btrfs/struct-funcs.c
@@ -33,7 +33,15 @@ static void report_setget_bounds(const struct extent_buffer *eb,
 static inline bool check_setget_bounds(const struct extent_buffer *eb,
                                       const void *ptr, unsigned off, int size)
 {
-       const unsigned long member_offset = (unsigned long)ptr + off;
+       unsigned long member_offset = (unsigned long)ptr + off;
+       static unsigned long failure = 1;
+       const unsigned long fail_after = 102400;
+
+       if (test_bit(BTRFS_FS_OPEN, &eb->fs_info->flags) &&
+           (failure++ % fail_after) == 0) {
+               printk(KERN_ERR "SETGET: trigger setget failure at %lu\n", failure);
+               member_offset = 128 * 1024;
+       }
---

David Sterba (5):
  btrfs: remove redundant check in up check_setget_bounds
  btrfs: factor out reporting when check_setget_bounds fails
  btrfs: store details about first setget bounds check failure
  btrfs: fail transaction when a setget bounds check failure is detected
  btrfs: move check_setget_bounds out of ASSERT

 fs/btrfs/ctree.h        | 16 +++++++++++--
 fs/btrfs/struct-funcs.c | 51 ++++++++++++++++++++++++++++-------------
 fs/btrfs/transaction.c  | 11 +++++++++
 3 files changed, 60 insertions(+), 18 deletions(-)

-- 
2.34.1

[PATCH 1/5] btrfs: remove redundant check in up check_setget_bounds

[David Sterba]

There are two separate checks in the bounds checker, the first one being
a special case of the second. As this function is performance critical
due to checking access to any eb member, reducing the size can slightly
improve performance.

On a release build on x86_64 the helper is completely inlined so the
function call overhead is also gone.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/struct-funcs.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/struct-funcs.c b/fs/btrfs/struct-funcs.c
index f429256f56db..12455b2b41de 100644
--- a/fs/btrfs/struct-funcs.c
+++ b/fs/btrfs/struct-funcs.c
@@ -12,15 +12,10 @@ static bool check_setget_bounds(const struct extent_buffer *eb,
 {
 	const unsigned long member_offset = (unsigned long)ptr + off;
 
-	if (member_offset > eb->len) {
+	if (unlikely(member_offset + size > eb->len)) {
 		btrfs_warn(eb->fs_info,
-	"bad eb member start: ptr 0x%lx start %llu member offset %lu size %d",
-			(unsigned long)ptr, eb->start, member_offset, size);
-		return false;
-	}
-	if (member_offset + size > eb->len) {
-		btrfs_warn(eb->fs_info,
-	"bad eb member end: ptr 0x%lx start %llu member offset %lu size %d",
+		"bad eb member %s: ptr 0x%lx start %llu member offset %lu size %d",
+			(member_offset > eb->len ? "start" : "end"),
 			(unsigned long)ptr, eb->start, member_offset, size);
 		return false;
 	}
-- 
2.34.1

[PATCH 2/5] btrfs: factor out reporting when check_setget_bounds fails

[David Sterba]

The eb bounds checks are supposed to almost never fail so the reporting
part can be moved away to a separate function and will be used to store
information about the failed checks. Add the inline keyword as a hint,
but according to the generated assembly it is already inlined.

The message level is set from warning to error as this should be
noticeable in the logs.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/struct-funcs.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/struct-funcs.c b/fs/btrfs/struct-funcs.c
index 12455b2b41de..c97c69e29d64 100644
--- a/fs/btrfs/struct-funcs.c
+++ b/fs/btrfs/struct-funcs.c
@@ -7,16 +7,24 @@
 
 #include "ctree.h"
 
-static bool check_setget_bounds(const struct extent_buffer *eb,
-				const void *ptr, unsigned off, int size)
+static void report_setget_bounds(const struct extent_buffer *eb,
+				 const void *ptr, unsigned off, int size)
 {
 	const unsigned long member_offset = (unsigned long)ptr + off;
 
-	if (unlikely(member_offset + size > eb->len)) {
-		btrfs_warn(eb->fs_info,
+	btrfs_err_rl(eb->fs_info,
 		"bad eb member %s: ptr 0x%lx start %llu member offset %lu size %d",
-			(member_offset > eb->len ? "start" : "end"),
-			(unsigned long)ptr, eb->start, member_offset, size);
+		(member_offset > eb->len ? "start" : "end"),
+		(unsigned long)ptr, eb->start, member_offset, size);
+}
+
+static inline bool check_setget_bounds(const struct extent_buffer *eb,
+				       const void *ptr, unsigned off, int size)
+{
+	const unsigned long member_offset = (unsigned long)ptr + off;
+
+	if (unlikely(member_offset + size > eb->len)) {
+		report_setget_bounds(eb, ptr, off, size);
 		return false;
 	}
 
-- 
2.34.1

[PATCH 3/5] btrfs: store details about first setget bounds check failure

[David Sterba]

The way the setget helpers are used makes it hard to return an error
code at the call site, as this would require to clutter a lot of code
with potential failures that are expected to be rare.

Instead, do a delayed reporting, tracked by a filesystem-wide bit that
synchronizes potential races in the reporting function that records only
the first event. To give a bit more insight into the scale, count the
total number of events.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.h        | 16 ++++++++++++++--
 fs/btrfs/struct-funcs.c | 12 ++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 9f7a950b8a69..5d12a80d09f5 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -127,6 +127,13 @@ static inline unsigned long btrfs_chunk_item_size(int num_stripes)
 enum {
 	/* Global indicator of serious filesystem errors */
 	BTRFS_FS_STATE_ERROR,
+	/* Track if a transaction abort has been reported on this filesystem */
+	BTRFS_FS_STATE_TRANS_ABORTED,
+	/*
+	 * There was a failed bounds check in check_setget_bounds, set this on
+	 * first event.
+	 */
+	BTRFS_FS_SETGET_COMPLAINS,
 	/*
 	 * Filesystem is being remounted, allow to skip some operations, like
 	 * defrag
@@ -134,8 +141,6 @@ enum {
 	BTRFS_FS_STATE_REMOUNTING,
 	/* Filesystem in RO mode */
 	BTRFS_FS_STATE_RO,
-	/* Track if a transaction abort has been reported on this filesystem */
-	BTRFS_FS_STATE_TRANS_ABORTED,
 	/*
 	 * Bio operations should be blocked on this filesystem because a source
 	 * or target device is being destroyed as part of a device replace
@@ -1060,6 +1065,13 @@ struct btrfs_fs_info {
 	spinlock_t zone_active_bgs_lock;
 	struct list_head zone_active_bgs;
 
+	/* Store details about the first bounds check failure in report_setget_bounds */
+	u64 setget_eb_start;
+	const void *setget_ptr;
+	unsigned setget_off;
+	int setget_size;
+	atomic_t setget_failures;
+
 #ifdef CONFIG_BTRFS_FS_REF_VERIFY
 	spinlock_t ref_verify_lock;
 	struct rb_root block_tree;
diff --git a/fs/btrfs/struct-funcs.c b/fs/btrfs/struct-funcs.c
index c97c69e29d64..28b9e62cdc86 100644
--- a/fs/btrfs/struct-funcs.c
+++ b/fs/btrfs/struct-funcs.c
@@ -11,11 +11,23 @@ static void report_setget_bounds(const struct extent_buffer *eb,
 				 const void *ptr, unsigned off, int size)
 {
 	const unsigned long member_offset = (unsigned long)ptr + off;
+	struct btrfs_fs_info *fs_info;
 
 	btrfs_err_rl(eb->fs_info,
 		"bad eb member %s: ptr 0x%lx start %llu member offset %lu size %d",
 		(member_offset > eb->len ? "start" : "end"),
 		(unsigned long)ptr, eb->start, member_offset, size);
+
+	/* Count events and record more details about the first one */
+	fs_info = eb->fs_info;
+	atomic_inc(&fs_info->setget_failures);
+	if (test_and_set_bit(BTRFS_FS_SETGET_COMPLAINS, &eb->fs_info->flags))
+		return;
+
+	fs_info->setget_eb_start = eb->start;
+	fs_info->setget_ptr = ptr;
+	fs_info->setget_off = member_offset;
+	fs_info->setget_size = size;
 }
 
 static inline bool check_setget_bounds(const struct extent_buffer *eb,
-- 
2.34.1

[PATCH 4/5] btrfs: fail transaction when a setget bounds check failure is detected

[David Sterba]

As the setget check only sets the bit, we need to use it in the
transaction:

- when attempting to start a new one, fail with EROFS as if would be
  aborted in another way already

- in should_end_transaction

- when transaction is about to end, insert an explicit abort

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/transaction.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 6db634ebae17..f48194df6c33 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -591,6 +591,9 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
 	if (BTRFS_FS_ERROR(fs_info))
 		return ERR_PTR(-EROFS);
 
+	if (test_bit(BTRFS_FS_SETGET_COMPLAINS, &fs_info->flags))
+		return ERR_PTR(-EROFS);
+
 	if (current->journal_info) {
 		WARN_ON(type & TRANS_EXTWRITERS);
 		h = current->journal_info;
@@ -924,6 +927,9 @@ static bool should_end_transaction(struct btrfs_trans_handle *trans)
 {
 	struct btrfs_fs_info *fs_info = trans->fs_info;
 
+	if (test_bit(BTRFS_FS_SETGET_COMPLAINS, &fs_info->flags))
+		return true;
+
 	if (btrfs_check_space_for_delayed_refs(fs_info))
 		return true;
 
@@ -969,6 +975,11 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
 	struct btrfs_transaction *cur_trans = trans->transaction;
 	int err = 0;
 
+	/* If a serious error was detected abort the transaction early */
+	if (!TRANS_ABORTED(trans) &&
+	    test_bit(BTRFS_FS_SETGET_COMPLAINS, &info->flags))
+		btrfs_abort_transaction(trans, -EIO);
+
 	if (refcount_read(&trans->use_count) > 1) {
 		refcount_dec(&trans->use_count);
 		trans->block_rsv = trans->orig_rsv;
-- 
2.34.1

[PATCH 5/5] btrfs: move check_setget_bounds out of ASSERT

[David Sterba]

The bounds check should be done on all builds unconditionally. Now that
the whole checking and reporting machinery is done and optimized, the
impact should be minimal.  Assertion would normally fail, the helpers
will not try to access the memory and return, we can't do much else.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/struct-funcs.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/struct-funcs.c b/fs/btrfs/struct-funcs.c
index 28b9e62cdc86..1761111e8098 100644
--- a/fs/btrfs/struct-funcs.c
+++ b/fs/btrfs/struct-funcs.c
@@ -81,7 +81,8 @@ u##bits btrfs_get_token_##bits(struct btrfs_map_token *token,		\
 									\
 	ASSERT(token);							\
 	ASSERT(token->kaddr);						\
-	ASSERT(check_setget_bounds(token->eb, ptr, off, size));		\
+	if (check_setget_bounds(token->eb, ptr, off, size))		\
+		return 0;						\
 	if (token->offset <= member_offset &&				\
 	    member_offset + size <= token->offset + PAGE_SIZE) {	\
 		return get_unaligned_le##bits(token->kaddr + oip);	\
@@ -108,7 +109,8 @@ u##bits btrfs_get_##bits(const struct extent_buffer *eb,		\
 	const int part = PAGE_SIZE - oip;				\
 	u8 lebytes[sizeof(u##bits)];					\
 									\
-	ASSERT(check_setget_bounds(eb, ptr, off, size));		\
+	if (check_setget_bounds(eb, ptr, off, size))			\
+		return 0;						\
 	if (INLINE_EXTENT_BUFFER_PAGES == 1 || oip + size <= PAGE_SIZE)	\
 		return get_unaligned_le##bits(kaddr + oip);		\
 									\
@@ -131,7 +133,8 @@ void btrfs_set_token_##bits(struct btrfs_map_token *token,		\
 									\
 	ASSERT(token);							\
 	ASSERT(token->kaddr);						\
-	ASSERT(check_setget_bounds(token->eb, ptr, off, size));		\
+	if (check_setget_bounds(token->eb, ptr, off, size))		\
+		return;							\
 	if (token->offset <= member_offset &&				\
 	    member_offset + size <= token->offset + PAGE_SIZE) {	\
 		put_unaligned_le##bits(val, token->kaddr + oip);	\
@@ -160,7 +163,8 @@ void btrfs_set_##bits(const struct extent_buffer *eb, void *ptr,	\
 	const int part = PAGE_SIZE - oip;				\
 	u8 lebytes[sizeof(u##bits)];					\
 									\
-	ASSERT(check_setget_bounds(eb, ptr, off, size));		\
+	if (check_setget_bounds(eb, ptr, off, size))			\
+		return;							\
 	if (INLINE_EXTENT_BUFFER_PAGES == 1 || oip + size <= PAGE_SIZE) { \
 		put_unaligned_le##bits(val, kaddr + oip);		\
 		return;							\
-- 
2.34.1

Re: [PATCH 0/5] Speedups and check_setget_bounds reporting updates

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 4/5] btrfs: fail transaction when a setget bounds check failure is detected

[Filipe Manana]

On Thu, Feb 03, 2022 at 06:26:31PM +0100, David Sterba wrote:
> As the setget check only sets the bit, we need to use it in the
> transaction:
> 
> - when attempting to start a new one, fail with EROFS as if would be
>   aborted in another way already
> 
> - in should_end_transaction
> 
> - when transaction is about to end, insert an explicit abort
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/transaction.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 6db634ebae17..f48194df6c33 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -591,6 +591,9 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
>  	if (BTRFS_FS_ERROR(fs_info))
>  		return ERR_PTR(-EROFS);
>  
> +	if (test_bit(BTRFS_FS_SETGET_COMPLAINS, &fs_info->flags))
> +		return ERR_PTR(-EROFS);
> +
>  	if (current->journal_info) {
>  		WARN_ON(type & TRANS_EXTWRITERS);
>  		h = current->journal_info;
> @@ -924,6 +927,9 @@ static bool should_end_transaction(struct btrfs_trans_handle *trans)
>  {
>  	struct btrfs_fs_info *fs_info = trans->fs_info;
>  
> +	if (test_bit(BTRFS_FS_SETGET_COMPLAINS, &fs_info->flags))
> +		return true;
> +
>  	if (btrfs_check_space_for_delayed_refs(fs_info))
>  		return true;
>  
> @@ -969,6 +975,11 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
>  	struct btrfs_transaction *cur_trans = trans->transaction;
>  	int err = 0;
>  
> +	/* If a serious error was detected abort the transaction early */
> +	if (!TRANS_ABORTED(trans) &&
> +	    test_bit(BTRFS_FS_SETGET_COMPLAINS, &info->flags))
> +		btrfs_abort_transaction(trans, -EIO);

Instead of sprinkling the test for BTRFS_FS_SETGET_COMPLAINS in all
these places, it seems to me it could be included in BTRFS_FS_ERROR().
And then having check_setget_bounds() call btrfs_handle_fs_error().

That would remove the need for all this code. Wouldn't it?

> +
>  	if (refcount_read(&trans->use_count) > 1) {
>  		refcount_dec(&trans->use_count);
>  		trans->block_rsv = trans->orig_rsv;

This misses one important case:

  task starts/joins/attaches a transaction

  fails one of the bounds check when accessing some extent buffer

  calls btrfs_commit_transaction()

The transaction ends up committed.

So a check and abort in the commit path, right before writing the super blocks,
should be in place.

With the above suggestions for check_setget_bounds() and BTRFS_FS_ERROR(),
this case would be handled automatically like the others, so no need for
sprinkling the checks and aborts in several places.

Thanks.

> -- 
> 2.34.1
> 

Re: [PATCH 3/5] btrfs: store details about first setget bounds check failure

[Filipe Manana]

On Thu, Feb 03, 2022 at 06:26:29PM +0100, David Sterba wrote:
> The way the setget helpers are used makes it hard to return an error
> code at the call site, as this would require to clutter a lot of code
> with potential failures that are expected to be rare.
> 
> Instead, do a delayed reporting, tracked by a filesystem-wide bit that
> synchronizes potential races in the reporting function that records only
> the first event. To give a bit more insight into the scale, count the
> total number of events.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/ctree.h        | 16 ++++++++++++++--
>  fs/btrfs/struct-funcs.c | 12 ++++++++++++
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 9f7a950b8a69..5d12a80d09f5 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -127,6 +127,13 @@ static inline unsigned long btrfs_chunk_item_size(int num_stripes)
>  enum {
>  	/* Global indicator of serious filesystem errors */
>  	BTRFS_FS_STATE_ERROR,
> +	/* Track if a transaction abort has been reported on this filesystem */
> +	BTRFS_FS_STATE_TRANS_ABORTED,
> +	/*
> +	 * There was a failed bounds check in check_setget_bounds, set this on
> +	 * first event.
> +	 */
> +	BTRFS_FS_SETGET_COMPLAINS,
>  	/*
>  	 * Filesystem is being remounted, allow to skip some operations, like
>  	 * defrag
> @@ -134,8 +141,6 @@ enum {
>  	BTRFS_FS_STATE_REMOUNTING,
>  	/* Filesystem in RO mode */
>  	BTRFS_FS_STATE_RO,
> -	/* Track if a transaction abort has been reported on this filesystem */
> -	BTRFS_FS_STATE_TRANS_ABORTED,
>  	/*
>  	 * Bio operations should be blocked on this filesystem because a source
>  	 * or target device is being destroyed as part of a device replace
> @@ -1060,6 +1065,13 @@ struct btrfs_fs_info {
>  	spinlock_t zone_active_bgs_lock;
>  	struct list_head zone_active_bgs;
>  
> +	/* Store details about the first bounds check failure in report_setget_bounds */
> +	u64 setget_eb_start;
> +	const void *setget_ptr;
> +	unsigned setget_off;
> +	int setget_size;
> +	atomic_t setget_failures;
> +
>  #ifdef CONFIG_BTRFS_FS_REF_VERIFY
>  	spinlock_t ref_verify_lock;
>  	struct rb_root block_tree;
> diff --git a/fs/btrfs/struct-funcs.c b/fs/btrfs/struct-funcs.c
> index c97c69e29d64..28b9e62cdc86 100644
> --- a/fs/btrfs/struct-funcs.c
> +++ b/fs/btrfs/struct-funcs.c
> @@ -11,11 +11,23 @@ static void report_setget_bounds(const struct extent_buffer *eb,
>  				 const void *ptr, unsigned off, int size)
>  {
>  	const unsigned long member_offset = (unsigned long)ptr + off;
> +	struct btrfs_fs_info *fs_info;
>  
>  	btrfs_err_rl(eb->fs_info,
>  		"bad eb member %s: ptr 0x%lx start %llu member offset %lu size %d",
>  		(member_offset > eb->len ? "start" : "end"),
>  		(unsigned long)ptr, eb->start, member_offset, size);
> +
> +	/* Count events and record more details about the first one */
> +	fs_info = eb->fs_info;
> +	atomic_inc(&fs_info->setget_failures);
> +	if (test_and_set_bit(BTRFS_FS_SETGET_COMPLAINS, &eb->fs_info->flags))
> +		return;
> +
> +	fs_info->setget_eb_start = eb->start;
> +	fs_info->setget_ptr = ptr;
> +	fs_info->setget_off = member_offset;
> +	fs_info->setget_size = size;

These new fields at fs_info are set here, but then never read, neither in this
patch nor in the remaining of this patchset.

Do you plan to use them somewhere else? If not, it seems they could go away.

Thanks.

>  }
>  
>  static inline bool check_setget_bounds(const struct extent_buffer *eb,
> -- 
> 2.34.1
> 

Re: [PATCH 4/5] btrfs: fail transaction when a setget bounds check failure is detected

[Filipe Manana]

On Fri, Feb 04, 2022 at 11:29:51AM +0000, Filipe Manana wrote:
> On Thu, Feb 03, 2022 at 06:26:31PM +0100, David Sterba wrote:
> > As the setget check only sets the bit, we need to use it in the
> > transaction:
> > 
> > - when attempting to start a new one, fail with EROFS as if would be
> >   aborted in another way already
> > 
> > - in should_end_transaction
> > 
> > - when transaction is about to end, insert an explicit abort
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >  fs/btrfs/transaction.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> > index 6db634ebae17..f48194df6c33 100644
> > --- a/fs/btrfs/transaction.c
> > +++ b/fs/btrfs/transaction.c
> > @@ -591,6 +591,9 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
> >  	if (BTRFS_FS_ERROR(fs_info))
> >  		return ERR_PTR(-EROFS);
> >  
> > +	if (test_bit(BTRFS_FS_SETGET_COMPLAINS, &fs_info->flags))
> > +		return ERR_PTR(-EROFS);
> > +
> >  	if (current->journal_info) {
> >  		WARN_ON(type & TRANS_EXTWRITERS);
> >  		h = current->journal_info;
> > @@ -924,6 +927,9 @@ static bool should_end_transaction(struct btrfs_trans_handle *trans)
> >  {
> >  	struct btrfs_fs_info *fs_info = trans->fs_info;
> >  
> > +	if (test_bit(BTRFS_FS_SETGET_COMPLAINS, &fs_info->flags))
> > +		return true;
> > +
> >  	if (btrfs_check_space_for_delayed_refs(fs_info))
> >  		return true;
> >  
> > @@ -969,6 +975,11 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
> >  	struct btrfs_transaction *cur_trans = trans->transaction;
> >  	int err = 0;
> >  
> > +	/* If a serious error was detected abort the transaction early */
> > +	if (!TRANS_ABORTED(trans) &&
> > +	    test_bit(BTRFS_FS_SETGET_COMPLAINS, &info->flags))
> > +		btrfs_abort_transaction(trans, -EIO);
> 
> Instead of sprinkling the test for BTRFS_FS_SETGET_COMPLAINS in all
> these places, it seems to me it could be included in BTRFS_FS_ERROR().
> And then having check_setget_bounds() call btrfs_handle_fs_error().
> 
> That would remove the need for all this code. Wouldn't it?

In fact just calling btrfs_handle_fs_error() at report_setget_bounds()
or check_setget_bounds() would be all that is needed.
No need for adding the flag BTRFS_FS_SETGET_COMPLAINS, checking for it
at several places, then aborting transaction in those places, etc.

Way more simple and less intrusive.

> 
> > +
> >  	if (refcount_read(&trans->use_count) > 1) {
> >  		refcount_dec(&trans->use_count);
> >  		trans->block_rsv = trans->orig_rsv;
> 
> This misses one important case:
> 
>   task starts/joins/attaches a transaction
> 
>   fails one of the bounds check when accessing some extent buffer
> 
>   calls btrfs_commit_transaction()
> 
> The transaction ends up committed.
> 
> So a check and abort in the commit path, right before writing the super blocks,
> should be in place.
> 
> With the above suggestions for check_setget_bounds() and BTRFS_FS_ERROR(),
> this case would be handled automatically like the others, so no need for
> sprinkling the checks and aborts in several places.
> 
> Thanks.
> 
> > -- 
> > 2.34.1
> > 

Re: [PATCH 3/5] btrfs: store details about first setget bounds check failure

[David Sterba]

On Fri, Feb 04, 2022 at 11:31:54AM +0000, Filipe Manana wrote:
> On Thu, Feb 03, 2022 at 06:26:29PM +0100, David Sterba wrote:
> > +
> > +	/* Count events and record more details about the first one */
> > +	fs_info = eb->fs_info;
> > +	atomic_inc(&fs_info->setget_failures);
> > +	if (test_and_set_bit(BTRFS_FS_SETGET_COMPLAINS, &eb->fs_info->flags))
> > +		return;
> > +
> > +	fs_info->setget_eb_start = eb->start;
> > +	fs_info->setget_ptr = ptr;
> > +	fs_info->setget_off = member_offset;
> > +	fs_info->setget_size = size;
> 
> These new fields at fs_info are set here, but then never read, neither in this
> patch nor in the remaining of this patchset.
> 
> Do you plan to use them somewhere else? If not, it seems they could go away.

The original idea was to print them at some later point, eg. at umount
time or when transaction commit is attempted and the error bit is set.
But as the error message is printed for each detected out of bounds it's
probably not necessary to store it. I'll drop it unless I find a better
reason.

Re: [PATCH 4/5] btrfs: fail transaction when a setget bounds check failure is detected

[David Sterba]

On Fri, Feb 04, 2022 at 11:29:51AM +0000, Filipe Manana wrote:
> On Thu, Feb 03, 2022 at 06:26:31PM +0100, David Sterba wrote:
> > As the setget check only sets the bit, we need to use it in the
> > transaction:
> > 
> > - when attempting to start a new one, fail with EROFS as if would be
> >   aborted in another way already
> > 
> > - in should_end_transaction
> > 
> > - when transaction is about to end, insert an explicit abort
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >  fs/btrfs/transaction.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> > index 6db634ebae17..f48194df6c33 100644
> > --- a/fs/btrfs/transaction.c
> > +++ b/fs/btrfs/transaction.c
> > @@ -591,6 +591,9 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
> >  	if (BTRFS_FS_ERROR(fs_info))
> >  		return ERR_PTR(-EROFS);
> >  
> > +	if (test_bit(BTRFS_FS_SETGET_COMPLAINS, &fs_info->flags))
> > +		return ERR_PTR(-EROFS);
> > +
> >  	if (current->journal_info) {
> >  		WARN_ON(type & TRANS_EXTWRITERS);
> >  		h = current->journal_info;
> > @@ -924,6 +927,9 @@ static bool should_end_transaction(struct btrfs_trans_handle *trans)
> >  {
> >  	struct btrfs_fs_info *fs_info = trans->fs_info;
> >  
> > +	if (test_bit(BTRFS_FS_SETGET_COMPLAINS, &fs_info->flags))
> > +		return true;
> > +
> >  	if (btrfs_check_space_for_delayed_refs(fs_info))
> >  		return true;
> >  
> > @@ -969,6 +975,11 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
> >  	struct btrfs_transaction *cur_trans = trans->transaction;
> >  	int err = 0;
> >  
> > +	/* If a serious error was detected abort the transaction early */
> > +	if (!TRANS_ABORTED(trans) &&
> > +	    test_bit(BTRFS_FS_SETGET_COMPLAINS, &info->flags))
> > +		btrfs_abort_transaction(trans, -EIO);
> 
> Instead of sprinkling the test for BTRFS_FS_SETGET_COMPLAINS in all
> these places, it seems to me it could be included in BTRFS_FS_ERROR().

Yeah that's a good idea.

> And then having check_setget_bounds() call btrfs_handle_fs_error().

btrfs_handle_fs_error is a bit heavyweight for all the potential cases
where the eb member check could happen.

> That would remove the need for all this code. Wouldn't it?
> 
> > +
> >  	if (refcount_read(&trans->use_count) > 1) {
> >  		refcount_dec(&trans->use_count);
> >  		trans->block_rsv = trans->orig_rsv;
> 
> This misses one important case:
> 
>   task starts/joins/attaches a transaction
> 
>   fails one of the bounds check when accessing some extent buffer
> 
>   calls btrfs_commit_transaction()
> 
> The transaction ends up committed.
> 
> So a check and abort in the commit path, right before writing the super blocks,
> should be in place.
> 
> With the above suggestions for check_setget_bounds() and BTRFS_FS_ERROR(),
> this case would be handled automatically like the others, so no need for
> sprinkling the checks and aborts in several places.

Agreed with the BTRFS_FS_ERROR part, I'm not sure about calling the
btrfs_handle_fs_error. The function was introduced before the
transaction abort mechanism, which builds on top of it, but there are
still calls to btrfs_handle_fs_error that seem to substitute abort.
Conversions like ba51e2a11e38 ("btrfs: change handle_fs_error in
recover_log_trees to aborts") need to happen, there are still like 30 of
them.

Re: [PATCH 1/5] btrfs: remove redundant check in up check_setget_bounds

[David Sterba]

On Thu, Feb 03, 2022 at 06:26:24PM +0100, David Sterba wrote:
> There are two separate checks in the bounds checker, the first one being
> a special case of the second. As this function is performance critical
> due to checking access to any eb member, reducing the size can slightly
> improve performance.
> 
> On a release build on x86_64 the helper is completely inlined so the
> function call overhead is also gone.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

This patch can be applied standalone, it's a perf optimization. I'll
update the changelog with numbers that I measured back then (it's
mentioned in the cover letter). The rest of the series will be reworked.

Re: [PATCH 4/5] btrfs: fail transaction when a setget bounds check failure is detected

[Filipe Manana]

On Thu, Feb 10, 2022 at 06:50:17PM +0100, David Sterba wrote:
> On Fri, Feb 04, 2022 at 11:29:51AM +0000, Filipe Manana wrote:
> > On Thu, Feb 03, 2022 at 06:26:31PM +0100, David Sterba wrote:
> > > As the setget check only sets the bit, we need to use it in the
> > > transaction:
> > > 
> > > - when attempting to start a new one, fail with EROFS as if would be
> > >   aborted in another way already
> > > 
> > > - in should_end_transaction
> > > 
> > > - when transaction is about to end, insert an explicit abort
> > > 
> > > Signed-off-by: David Sterba <dsterba@suse.com>
> > > ---
> > >  fs/btrfs/transaction.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> > > index 6db634ebae17..f48194df6c33 100644
> > > --- a/fs/btrfs/transaction.c
> > > +++ b/fs/btrfs/transaction.c
> > > @@ -591,6 +591,9 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
> > >  	if (BTRFS_FS_ERROR(fs_info))
> > >  		return ERR_PTR(-EROFS);
> > >  
> > > +	if (test_bit(BTRFS_FS_SETGET_COMPLAINS, &fs_info->flags))
> > > +		return ERR_PTR(-EROFS);
> > > +
> > >  	if (current->journal_info) {
> > >  		WARN_ON(type & TRANS_EXTWRITERS);
> > >  		h = current->journal_info;
> > > @@ -924,6 +927,9 @@ static bool should_end_transaction(struct btrfs_trans_handle *trans)
> > >  {
> > >  	struct btrfs_fs_info *fs_info = trans->fs_info;
> > >  
> > > +	if (test_bit(BTRFS_FS_SETGET_COMPLAINS, &fs_info->flags))
> > > +		return true;
> > > +
> > >  	if (btrfs_check_space_for_delayed_refs(fs_info))
> > >  		return true;
> > >  
> > > @@ -969,6 +975,11 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
> > >  	struct btrfs_transaction *cur_trans = trans->transaction;
> > >  	int err = 0;
> > >  
> > > +	/* If a serious error was detected abort the transaction early */
> > > +	if (!TRANS_ABORTED(trans) &&
> > > +	    test_bit(BTRFS_FS_SETGET_COMPLAINS, &info->flags))
> > > +		btrfs_abort_transaction(trans, -EIO);
> > 
> > Instead of sprinkling the test for BTRFS_FS_SETGET_COMPLAINS in all
> > these places, it seems to me it could be included in BTRFS_FS_ERROR().
> 
> Yeah that's a good idea.
> 
> > And then having check_setget_bounds() call btrfs_handle_fs_error().
> 
> btrfs_handle_fs_error is a bit heavyweight for all the potential cases
> where the eb member check could happen.
> 
> > That would remove the need for all this code. Wouldn't it?
> > 
> > > +
> > >  	if (refcount_read(&trans->use_count) > 1) {
> > >  		refcount_dec(&trans->use_count);
> > >  		trans->block_rsv = trans->orig_rsv;
> > 
> > This misses one important case:
> > 
> >   task starts/joins/attaches a transaction
> > 
> >   fails one of the bounds check when accessing some extent buffer
> > 
> >   calls btrfs_commit_transaction()
> > 
> > The transaction ends up committed.
> > 
> > So a check and abort in the commit path, right before writing the super blocks,
> > should be in place.
> > 
> > With the above suggestions for check_setget_bounds() and BTRFS_FS_ERROR(),
> > this case would be handled automatically like the others, so no need for
> > sprinkling the checks and aborts in several places.
> 
> Agreed with the BTRFS_FS_ERROR part, I'm not sure about calling the
> btrfs_handle_fs_error. The function was introduced before the
> transaction abort mechanism, which builds on top of it, but there are
> still calls to btrfs_handle_fs_error that seem to substitute abort.
> Conversions like ba51e2a11e38 ("btrfs: change handle_fs_error in
> recover_log_trees to aborts") need to happen, there are still like 30 of
> them.

Well, btrfs_handle_fs_error() is handy when we don't have access to a
transaction and we need to prevent a future transaction from starting.

Another alternative, instead of adding that new bit, simply doing something
like the following at check_setget_bounds():

    if (current->journal_info)
        btrfs_abort_transaction(current->journal_info, -EUCLEAN);

For a task doing reads only, and that nevers joins/starts a transaction,
in case it calls a getter that does an out of bounds access, there's no
way to return an error back to user space anyway.

There's always the case a task may do such a bad get and later join/start
a transaction and call a setter with a value computed on top of a bad
value returned by the bad getter.

I still think btrfs_handle_fs_error() is the best to use here. It fits
perfectly for this scenario, without neither the need to add a new bit
nor to sprinkle more logic into transaction.c