diff mbox series

bcachefs: Use struct_size()

Message ID 120b638f99b088f91d5a4491c800463c554e70b8.1696144401.git.christophe.jaillet@wanadoo.fr
State New
Headers
Series bcachefs: Use struct_size() |

Commit Message

Christophe JAILLET Oct. 1, 2023, 7:13 a.m. UTC 
  Use struct_size() instead of hand writing it.
This is less verbose and more robust.

While at it, prepare for the coming implementation by GCC and Clang of the
__counted_by attribute. Flexible array members annotated with __counted_by
can have their accesses bounds-checked at run-time checking via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
strcpy/memcpy-family functions).

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch is part of a work done in parallel of what is currently worked
on by Kees Cook.

My patches are only related to corner cases that do NOT match the
semantic of his Coccinelle script[1].

In this case, struct_size() was not used to compute the size needed for the
structure and its flex array.

[1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
---
 fs/bcachefs/disk_groups.c | 3 +--
 fs/bcachefs/super_types.h | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

Comments

Gustavo A. R. Silva Oct. 1, 2023, 7:23 a.m. UTC | #1 
On 10/1/23 09:13, Christophe JAILLET wrote:
> Use struct_size() instead of hand writing it.
> This is less verbose and more robust.
> 
> While at it, prepare for the coming implementation by GCC and Clang of the
> __counted_by attribute. Flexible array members annotated with __counted_by
> can have their accesses bounds-checked at run-time checking via
> CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
> strcpy/memcpy-family functions).

I would prefer this as two separate patches.

> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

In any case:

Reviewed-by: Gustavo A. R. Silva <gustavors@kernel.org>

Thanks
--
Gustavo

> ---
> This patch is part of a work done in parallel of what is currently worked
> on by Kees Cook.
> 
> My patches are only related to corner cases that do NOT match the
> semantic of his Coccinelle script[1].
> 
> In this case, struct_size() was not used to compute the size needed for the
> structure and its flex array.
> 
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
> ---
>   fs/bcachefs/disk_groups.c | 3 +--
>   fs/bcachefs/super_types.h | 2 +-
>   2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/bcachefs/disk_groups.c b/fs/bcachefs/disk_groups.c
> index b292dbef7992..224efa917427 100644
> --- a/fs/bcachefs/disk_groups.c
> +++ b/fs/bcachefs/disk_groups.c
> @@ -166,8 +166,7 @@ int bch2_sb_disk_groups_to_cpu(struct bch_fs *c)
>   	if (!groups)
>   		return 0;
>   
> -	cpu_g = kzalloc(sizeof(*cpu_g) +
> -			sizeof(cpu_g->entries[0]) * nr_groups, GFP_KERNEL);
> +	cpu_g = kzalloc(struct_size(cpu_g, entries, nr_groups), GFP_KERNEL);
>   	if (!cpu_g)
>   		return -BCH_ERR_ENOMEM_disk_groups_to_cpu;
>   
> diff --git a/fs/bcachefs/super_types.h b/fs/bcachefs/super_types.h
> index 597a8db73585..78d6138db62d 100644
> --- a/fs/bcachefs/super_types.h
> +++ b/fs/bcachefs/super_types.h
> @@ -46,7 +46,7 @@ struct bch_disk_group_cpu {
>   struct bch_disk_groups_cpu {
>   	struct rcu_head			rcu;
>   	unsigned			nr;
> -	struct bch_disk_group_cpu	entries[];
> +	struct bch_disk_group_cpu	entries[] __counted_by(nr);
>   };
>   
>   #endif /* _BCACHEFS_SUPER_TYPES_H */
Dan Carpenter Oct. 2, 2023, 6:42 a.m. UTC | #2 
On Sun, Oct 01, 2023 at 09:23:17AM +0200, Gustavo A. R. Silva wrote:
> 
> 
> On 10/1/23 09:13, Christophe JAILLET wrote:
> > Use struct_size() instead of hand writing it.
> > This is less verbose and more robust.
> > 
> > While at it, prepare for the coming implementation by GCC and Clang of the
> > __counted_by attribute. Flexible array members annotated with __counted_by
> > can have their accesses bounds-checked at run-time checking via
> > CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
> > strcpy/memcpy-family functions).
> 
> I would prefer this as two separate patches.
> 

I kind of feel like it's all part of one thing.  It's easier to review
as one patch.

regards,
dan carpenter
Dan Carpenter Oct. 2, 2023, 7:06 a.m. UTC | #3 
On Mon, Oct 02, 2023 at 09:42:06AM +0300, Dan Carpenter wrote:
> On Sun, Oct 01, 2023 at 09:23:17AM +0200, Gustavo A. R. Silva wrote:
> > 
> > 
> > On 10/1/23 09:13, Christophe JAILLET wrote:
> > > Use struct_size() instead of hand writing it.
> > > This is less verbose and more robust.
> > > 
> > > While at it, prepare for the coming implementation by GCC and Clang of the
> > > __counted_by attribute. Flexible array members annotated with __counted_by
> > > can have their accesses bounds-checked at run-time checking via
> > > CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
> > > strcpy/memcpy-family functions).
> > 
> > I would prefer this as two separate patches.
> > 
> 
> I kind of feel like it's all part of one thing.  It's easier to review
> as one patch.

Also I think there is static analysis which sees struct_size()
allocations and pushes people to use __counted_by() so doing it in two
steps is sort of like introducing a static checker bug and then
silencing it in the next patch.

regards,
dan carpenter
Kent Overstreet Oct. 6, 2023, 5:32 p.m. UTC | #4 
On Sun, Oct 01, 2023 at 09:13:54AM +0200, Christophe JAILLET wrote:
> Use struct_size() instead of hand writing it.
> This is less verbose and more robust.
> 
> While at it, prepare for the coming implementation by GCC and Clang of the
> __counted_by attribute. Flexible array members annotated with __counted_by
> can have their accesses bounds-checked at run-time checking via
> CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
> strcpy/memcpy-family functions).
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Thanks! Applied.
diff mbox series

Patch

diff --git a/fs/bcachefs/disk_groups.c b/fs/bcachefs/disk_groups.c
index b292dbef7992..224efa917427 100644
--- a/fs/bcachefs/disk_groups.c
+++ b/fs/bcachefs/disk_groups.c
@@ -166,8 +166,7 @@  int bch2_sb_disk_groups_to_cpu(struct bch_fs *c)
 	if (!groups)
 		return 0;
 
-	cpu_g = kzalloc(sizeof(*cpu_g) +
-			sizeof(cpu_g->entries[0]) * nr_groups, GFP_KERNEL);
+	cpu_g = kzalloc(struct_size(cpu_g, entries, nr_groups), GFP_KERNEL);
 	if (!cpu_g)
 		return -BCH_ERR_ENOMEM_disk_groups_to_cpu;
 
diff --git a/fs/bcachefs/super_types.h b/fs/bcachefs/super_types.h
index 597a8db73585..78d6138db62d 100644
--- a/fs/bcachefs/super_types.h
+++ b/fs/bcachefs/super_types.h
@@ -46,7 +46,7 @@  struct bch_disk_group_cpu {
 struct bch_disk_groups_cpu {
 	struct rcu_head			rcu;
 	unsigned			nr;
-	struct bch_disk_group_cpu	entries[];
+	struct bch_disk_group_cpu	entries[] __counted_by(nr);
 };
 
 #endif /* _BCACHEFS_SUPER_TYPES_H */