[v1] bcachefs: Annotate bch_replicas_entry_{v0,v1} with __counted_by()

[PATCH] bcachefs: Annotate bch_replicas_entry_{v0,v1} with __counted_by()

Posted by Thorsten Blum 1 year, 3 months ago

Add the __counted_by compiler attribute to the flexible array members
devs to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
CONFIG_FORTIFY_SOURCE.

Increment nr_devs before adding a new device to the devs array and
adjust the array indexes accordingly.

In bch2_journal_read(), explicitly set nr_devs to 0.

Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
---
 fs/bcachefs/buckets.c         | 3 ++-
 fs/bcachefs/journal_io.c      | 3 ++-
 fs/bcachefs/replicas.c        | 6 +++---
 fs/bcachefs/replicas_format.h | 4 ++--
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/bcachefs/buckets.c b/fs/bcachefs/buckets.c
index be2bbd248631..1e6badf9ddd2 100644
--- a/fs/bcachefs/buckets.c
+++ b/fs/bcachefs/buckets.c
@@ -740,7 +740,8 @@ static int __trigger_extent(struct btree_trans *trans,
 				return ret;
 		} else if (!p.has_ec) {
 			replicas_sectors       += disk_sectors;
-			acc_replicas_key.replicas.devs[acc_replicas_key.replicas.nr_devs++] = p.ptr.dev;
+			acc_replicas_key.replicas.nr_devs++;
+			acc_replicas_key.replicas.devs[acc_replicas_key.replicas.nr_devs - 1] = p.ptr.dev;
 		} else {
 			ret = bch2_trigger_stripe_ptr(trans, k, p, data_type, disk_sectors, flags);
 			if (ret)
diff --git a/fs/bcachefs/journal_io.c b/fs/bcachefs/journal_io.c
index 7664b68e6a15..d1bd883c2c55 100644
--- a/fs/bcachefs/journal_io.c
+++ b/fs/bcachefs/journal_io.c
@@ -1353,6 +1353,7 @@ int bch2_journal_read(struct bch_fs *c,
 	genradix_for_each(&c->journal_entries, radix_iter, _i) {
 		struct bch_replicas_padded replicas = {
 			.e.data_type = BCH_DATA_journal,
+			.e.nr_devs = 0,
 			.e.nr_required = 1,
 		};
 
@@ -1379,7 +1380,7 @@ int bch2_journal_read(struct bch_fs *c,
 			goto err;
 
 		darray_for_each(i->ptrs, ptr)
-			replicas.e.devs[replicas.e.nr_devs++] = ptr->dev;
+			replicas.e.devs[++replicas.e.nr_devs - 1] = ptr->dev;
 
 		bch2_replicas_entry_sort(&replicas.e);
 
diff --git a/fs/bcachefs/replicas.c b/fs/bcachefs/replicas.c
index 1223b710755d..90d9b7d761bc 100644
--- a/fs/bcachefs/replicas.c
+++ b/fs/bcachefs/replicas.c
@@ -122,7 +122,7 @@ static void extent_to_replicas(struct bkey_s_c k,
 			continue;
 
 		if (!p.has_ec)
-			r->devs[r->nr_devs++] = p.ptr.dev;
+			r->devs[++r->nr_devs - 1] = p.ptr.dev;
 		else
 			r->nr_required = 0;
 	}
@@ -139,7 +139,7 @@ static void stripe_to_replicas(struct bkey_s_c k,
 	for (ptr = s.v->ptrs;
 	     ptr < s.v->ptrs + s.v->nr_blocks;
 	     ptr++)
-		r->devs[r->nr_devs++] = ptr->dev;
+		r->devs[++r->nr_devs - 1] = ptr->dev;
 }
 
 void bch2_bkey_to_replicas(struct bch_replicas_entry_v1 *e,
@@ -180,7 +180,7 @@ void bch2_devlist_to_replicas(struct bch_replicas_entry_v1 *e,
 	e->nr_required	= 1;
 
 	darray_for_each(devs, i)
-		e->devs[e->nr_devs++] = *i;
+		e->devs[++e->nr_devs - 1] = *i;
 
 	bch2_replicas_entry_sort(e);
 }
diff --git a/fs/bcachefs/replicas_format.h b/fs/bcachefs/replicas_format.h
index b97208195d06..d2e080d0ecb7 100644
--- a/fs/bcachefs/replicas_format.h
+++ b/fs/bcachefs/replicas_format.h
@@ -5,7 +5,7 @@
 struct bch_replicas_entry_v0 {
 	__u8			data_type;
 	__u8			nr_devs;
-	__u8			devs[];
+	__u8			devs[] __counted_by(nr_devs);
 } __packed;
 
 struct bch_sb_field_replicas_v0 {
@@ -17,7 +17,7 @@ struct bch_replicas_entry_v1 {
 	__u8			data_type;
 	__u8			nr_devs;
 	__u8			nr_required;
-	__u8			devs[];
+	__u8			devs[] __counted_by(nr_devs);
 } __packed;
 
 struct bch_sb_field_replicas {
-- 
2.46.0

Re: [PATCH] bcachefs: Annotate bch_replicas_entry_{v0,v1} with __counted_by()

Posted by Kent Overstreet 1 year, 3 months ago

On Sun, Aug 25, 2024 at 03:36:02PM GMT, Thorsten Blum wrote:
> Add the __counted_by compiler attribute to the flexible array members
> devs to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
> CONFIG_FORTIFY_SOURCE.
> 
> Increment nr_devs before adding a new device to the devs array and
> adjust the array indexes accordingly.

The nr_devs changes are pretty gross - please add a helper for that

> 
> In bch2_journal_read(), explicitly set nr_devs to 0.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
> ---
>  fs/bcachefs/buckets.c         | 3 ++-
>  fs/bcachefs/journal_io.c      | 3 ++-
>  fs/bcachefs/replicas.c        | 6 +++---
>  fs/bcachefs/replicas_format.h | 4 ++--
>  4 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/bcachefs/buckets.c b/fs/bcachefs/buckets.c
> index be2bbd248631..1e6badf9ddd2 100644
> --- a/fs/bcachefs/buckets.c
> +++ b/fs/bcachefs/buckets.c
> @@ -740,7 +740,8 @@ static int __trigger_extent(struct btree_trans *trans,
>  				return ret;
>  		} else if (!p.has_ec) {
>  			replicas_sectors       += disk_sectors;
> -			acc_replicas_key.replicas.devs[acc_replicas_key.replicas.nr_devs++] = p.ptr.dev;
> +			acc_replicas_key.replicas.nr_devs++;
> +			acc_replicas_key.replicas.devs[acc_replicas_key.replicas.nr_devs - 1] = p.ptr.dev;
>  		} else {
>  			ret = bch2_trigger_stripe_ptr(trans, k, p, data_type, disk_sectors, flags);
>  			if (ret)
> diff --git a/fs/bcachefs/journal_io.c b/fs/bcachefs/journal_io.c
> index 7664b68e6a15..d1bd883c2c55 100644
> --- a/fs/bcachefs/journal_io.c
> +++ b/fs/bcachefs/journal_io.c
> @@ -1353,6 +1353,7 @@ int bch2_journal_read(struct bch_fs *c,
>  	genradix_for_each(&c->journal_entries, radix_iter, _i) {
>  		struct bch_replicas_padded replicas = {
>  			.e.data_type = BCH_DATA_journal,
> +			.e.nr_devs = 0,
>  			.e.nr_required = 1,
>  		};
>  
> @@ -1379,7 +1380,7 @@ int bch2_journal_read(struct bch_fs *c,
>  			goto err;
>  
>  		darray_for_each(i->ptrs, ptr)
> -			replicas.e.devs[replicas.e.nr_devs++] = ptr->dev;
> +			replicas.e.devs[++replicas.e.nr_devs - 1] = ptr->dev;
>  
>  		bch2_replicas_entry_sort(&replicas.e);
>  
> diff --git a/fs/bcachefs/replicas.c b/fs/bcachefs/replicas.c
> index 1223b710755d..90d9b7d761bc 100644
> --- a/fs/bcachefs/replicas.c
> +++ b/fs/bcachefs/replicas.c
> @@ -122,7 +122,7 @@ static void extent_to_replicas(struct bkey_s_c k,
>  			continue;
>  
>  		if (!p.has_ec)
> -			r->devs[r->nr_devs++] = p.ptr.dev;
> +			r->devs[++r->nr_devs - 1] = p.ptr.dev;
>  		else
>  			r->nr_required = 0;
>  	}
> @@ -139,7 +139,7 @@ static void stripe_to_replicas(struct bkey_s_c k,
>  	for (ptr = s.v->ptrs;
>  	     ptr < s.v->ptrs + s.v->nr_blocks;
>  	     ptr++)
> -		r->devs[r->nr_devs++] = ptr->dev;
> +		r->devs[++r->nr_devs - 1] = ptr->dev;
>  }
>  
>  void bch2_bkey_to_replicas(struct bch_replicas_entry_v1 *e,
> @@ -180,7 +180,7 @@ void bch2_devlist_to_replicas(struct bch_replicas_entry_v1 *e,
>  	e->nr_required	= 1;
>  
>  	darray_for_each(devs, i)
> -		e->devs[e->nr_devs++] = *i;
> +		e->devs[++e->nr_devs - 1] = *i;
>  
>  	bch2_replicas_entry_sort(e);
>  }
> diff --git a/fs/bcachefs/replicas_format.h b/fs/bcachefs/replicas_format.h
> index b97208195d06..d2e080d0ecb7 100644
> --- a/fs/bcachefs/replicas_format.h
> +++ b/fs/bcachefs/replicas_format.h
> @@ -5,7 +5,7 @@
>  struct bch_replicas_entry_v0 {
>  	__u8			data_type;
>  	__u8			nr_devs;
> -	__u8			devs[];
> +	__u8			devs[] __counted_by(nr_devs);
>  } __packed;
>  
>  struct bch_sb_field_replicas_v0 {
> @@ -17,7 +17,7 @@ struct bch_replicas_entry_v1 {
>  	__u8			data_type;
>  	__u8			nr_devs;
>  	__u8			nr_required;
> -	__u8			devs[];
> +	__u8			devs[] __counted_by(nr_devs);
>  } __packed;
>  
>  struct bch_sb_field_replicas {
> -- 
> 2.46.0
>

Re: [PATCH] bcachefs: Annotate bch_replicas_entry_{v0,v1} with __counted_by()

Posted by Thorsten Blum 1 year, 3 months ago

On 25. Aug 2024, at 20:43, Kent Overstreet <kent.overstreet@linux.dev> wrote:
> On Sun, Aug 25, 2024 at 03:36:02PM GMT, Thorsten Blum wrote:
>> Add the __counted_by compiler attribute to the flexible array members
>> devs to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
>> CONFIG_FORTIFY_SOURCE.
>> 
>> Increment nr_devs before adding a new device to the devs array and
>> adjust the array indexes accordingly.
> 
> The nr_devs changes are pretty gross - please add a helper for that

How about a macro in replicas_format.h like this:

#define replicas_entry_add_dev(e, d) ({
	(e)->nr_devs++;
	(e)->devs[(e)->nr_devs - 1] = (d);
})

Thanks,
Thorsten

Re: [PATCH] bcachefs: Annotate bch_replicas_entry_{v0,v1} with __counted_by()

Posted by Kent Overstreet 1 year, 3 months ago

On Sun, Aug 25, 2024 at 10:41:55PM GMT, Thorsten Blum wrote:
> On 25. Aug 2024, at 20:43, Kent Overstreet <kent.overstreet@linux.dev> wrote:
> > On Sun, Aug 25, 2024 at 03:36:02PM GMT, Thorsten Blum wrote:
> >> Add the __counted_by compiler attribute to the flexible array members
> >> devs to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
> >> CONFIG_FORTIFY_SOURCE.
> >> 
> >> Increment nr_devs before adding a new device to the devs array and
> >> adjust the array indexes accordingly.
> > 
> > The nr_devs changes are pretty gross - please add a helper for that
> 
> How about a macro in replicas_format.h like this:
> 
> #define replicas_entry_add_dev(e, d) ({
> 	(e)->nr_devs++;
> 	(e)->devs[(e)->nr_devs - 1] = (d);
> })

Does it need to be a macro?

Re: [PATCH] bcachefs: Annotate bch_replicas_entry_{v0,v1} with __counted_by()

Posted by Thorsten Blum 1 year, 3 months ago

On 26. Aug 2024, at 00:56, Kent Overstreet <kent.overstreet@linux.dev> wrote:
> On Sun, Aug 25, 2024 at 10:41:55PM GMT, Thorsten Blum wrote:
>> On 25. Aug 2024, at 20:43, Kent Overstreet <kent.overstreet@linux.dev> wrote:
>>> On Sun, Aug 25, 2024 at 03:36:02PM GMT, Thorsten Blum wrote:
>>>> Add the __counted_by compiler attribute to the flexible array members
>>>> devs to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
>>>> CONFIG_FORTIFY_SOURCE.
>>>> 
>>>> Increment nr_devs before adding a new device to the devs array and
>>>> adjust the array indexes accordingly.
>>> 
>>> The nr_devs changes are pretty gross - please add a helper for that
>> 
>> How about a macro in replicas_format.h like this:
>> 
>> #define replicas_entry_add_dev(e, d) ({
>> 	(e)->nr_devs++;
>> 	(e)->devs[(e)->nr_devs - 1] = (d);
>> })
> 
> Does it need to be a macro?

It could also be two functions, one for each struct.

Which one do you prefer?

Re: [PATCH] bcachefs: Annotate bch_replicas_entry_{v0,v1} with __counted_by()

Posted by Kent Overstreet 1 year, 3 months ago

On Mon, Aug 26, 2024 at 01:08:29AM GMT, Thorsten Blum wrote:
> On 26. Aug 2024, at 00:56, Kent Overstreet <kent.overstreet@linux.dev> wrote:
> > On Sun, Aug 25, 2024 at 10:41:55PM GMT, Thorsten Blum wrote:
> >> On 25. Aug 2024, at 20:43, Kent Overstreet <kent.overstreet@linux.dev> wrote:
> >>> On Sun, Aug 25, 2024 at 03:36:02PM GMT, Thorsten Blum wrote:
> >>>> Add the __counted_by compiler attribute to the flexible array members
> >>>> devs to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
> >>>> CONFIG_FORTIFY_SOURCE.
> >>>> 
> >>>> Increment nr_devs before adding a new device to the devs array and
> >>>> adjust the array indexes accordingly.
> >>> 
> >>> The nr_devs changes are pretty gross - please add a helper for that
> >> 
> >> How about a macro in replicas_format.h like this:
> >> 
> >> #define replicas_entry_add_dev(e, d) ({
> >> 	(e)->nr_devs++;
> >> 	(e)->devs[(e)->nr_devs - 1] = (d);
> >> })
> > 
> > Does it need to be a macro?
> 
> It could also be two functions, one for each struct.
> 
> Which one do you prefer?

I suppose the macro fits with what we're doing for
replicas_entry_bytes() - that's alright then