| Message ID | 20240825133601.24036-2-thorsten.blum@toblux.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bcachefs: Annotate bch_replicas_entry_{v0,v1} with __counted_by() | expand |
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 >
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
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?
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?
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
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 {
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(-)