| Message ID | ZR1maZoAh2W/0Vw6@work (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c4d49196ceec80e30e8d981410d73331b49b7850 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: sched: cls_u32: Fix allocation size in u32_init() | expand |
On Wed, Oct 4, 2023 at 9:19 AM Gustavo A. R. Silva <gustavoars@kernel.org> wrote: > > commit d61491a51f7e ("net/sched: cls_u32: Replace one-element array > with flexible-array member") incorrecly replaced an instance of > `sizeof(*tp_c)` with `struct_size(tp_c, hlist->ht, 1)`. This results > in a an over-allocation of 8 bytes. > > This change is wrong because `hlist` in `struct tc_u_common` is a > pointer: > > net/sched/cls_u32.c: > struct tc_u_common { > struct tc_u_hnode __rcu *hlist; > void *ptr; > int refcnt; > struct idr handle_idr; > struct hlist_node hnode; > long knodes; > }; > > So, the use of `struct_size()` makes no sense: we don't need to allocate > any extra space for a flexible-array member. `sizeof(*tp_c)` is just fine. > > So, `struct_size(tp_c, hlist->ht, 1)` translates to: > > sizeof(*tp_c) + sizeof(tp_c->hlist->ht) == > sizeof(struct tc_u_common) + sizeof(struct tc_u_knode *) == > 144 + 8 == 0x98 (byes) > ^^^ > | > unnecessary extra > allocation size > > $ pahole -C tc_u_common net/sched/cls_u32.o > struct tc_u_common { > struct tc_u_hnode * hlist; /* 0 8 */ > void * ptr; /* 8 8 */ > int refcnt; /* 16 4 */ > > /* XXX 4 bytes hole, try to pack */ > > struct idr handle_idr; /* 24 96 */ > /* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */ > struct hlist_node hnode; /* 120 16 */ > /* --- cacheline 2 boundary (128 bytes) was 8 bytes ago --- */ > long int knodes; /* 136 8 */ > > /* size: 144, cachelines: 3, members: 6 */ > /* sum members: 140, holes: 1, sum holes: 4 */ > /* last cacheline: 16 bytes */ > }; > > And with `sizeof(*tp_c)`, we have: > > sizeof(*tp_c) == sizeof(struct tc_u_common) == 144 == 0x90 (bytes) > > which is the correct and original allocation size. > > Fix this issue by replacing `struct_size(tp_c, hlist->ht, 1)` with > `sizeof(*tp_c)`, and avoid allocating 8 too many bytes. > > The following difference in binary output is expected and reflects the > desired change: > > | net/sched/cls_u32.o > | @@ -6148,7 +6148,7 @@ > | include/linux/slab.h:599 > | 2cf5: mov 0x0(%rip),%rdi # 2cfc <u32_init+0xfc> > | 2cf8: R_X86_64_PC32 kmalloc_caches+0xc > |- 2cfc: mov $0x98,%edx > |+ 2cfc: mov $0x90,%edx > > Reported-by: Alejandro Colomar <alx@kernel.org> > Closes: https://lore.kernel.org/lkml/09b4a2ce-da74-3a19-6961-67883f634d98@kernel.org/ > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> cheers, jamal > Changes in v2: > - Update subject line. > - Update changelog text. > > v1: > - Link: https://lore.kernel.org/linux-hardening/ZN5DvRyq6JNz20l1@work/ > > net/sched/cls_u32.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c > index da4c179a4d41..6663e971a13e 100644 > --- a/net/sched/cls_u32.c > +++ b/net/sched/cls_u32.c > @@ -366,7 +366,7 @@ static int u32_init(struct tcf_proto *tp) > idr_init(&root_ht->handle_idr); > > if (tp_c == NULL) { > - tp_c = kzalloc(struct_size(tp_c, hlist->ht, 1), GFP_KERNEL); > + tp_c = kzalloc(sizeof(*tp_c), GFP_KERNEL); > if (tp_c == NULL) { > kfree(root_ht); > return -ENOBUFS; > -- > 2.34.1 >
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Wed, 4 Oct 2023 15:19:37 +0200 you wrote: > commit d61491a51f7e ("net/sched: cls_u32: Replace one-element array > with flexible-array member") incorrecly replaced an instance of > `sizeof(*tp_c)` with `struct_size(tp_c, hlist->ht, 1)`. This results > in a an over-allocation of 8 bytes. > > This change is wrong because `hlist` in `struct tc_u_common` is a > pointer: > > [...] Here is the summary with links: - [v2] net: sched: cls_u32: Fix allocation size in u32_init() https://git.kernel.org/netdev/net/c/c4d49196ceec You are awesome, thank you!
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c index da4c179a4d41..6663e971a13e 100644 --- a/net/sched/cls_u32.c +++ b/net/sched/cls_u32.c @@ -366,7 +366,7 @@ static int u32_init(struct tcf_proto *tp) idr_init(&root_ht->handle_idr); if (tp_c == NULL) { - tp_c = kzalloc(struct_size(tp_c, hlist->ht, 1), GFP_KERNEL); + tp_c = kzalloc(sizeof(*tp_c), GFP_KERNEL); if (tp_c == NULL) { kfree(root_ht); return -ENOBUFS;
commit d61491a51f7e ("net/sched: cls_u32: Replace one-element array with flexible-array member") incorrecly replaced an instance of `sizeof(*tp_c)` with `struct_size(tp_c, hlist->ht, 1)`. This results in a an over-allocation of 8 bytes. This change is wrong because `hlist` in `struct tc_u_common` is a pointer: net/sched/cls_u32.c: struct tc_u_common { struct tc_u_hnode __rcu *hlist; void *ptr; int refcnt; struct idr handle_idr; struct hlist_node hnode; long knodes; }; So, the use of `struct_size()` makes no sense: we don't need to allocate any extra space for a flexible-array member. `sizeof(*tp_c)` is just fine. So, `struct_size(tp_c, hlist->ht, 1)` translates to: sizeof(*tp_c) + sizeof(tp_c->hlist->ht) == sizeof(struct tc_u_common) + sizeof(struct tc_u_knode *) == 144 + 8 == 0x98 (byes) ^^^ | unnecessary extra allocation size $ pahole -C tc_u_common net/sched/cls_u32.o struct tc_u_common { struct tc_u_hnode * hlist; /* 0 8 */ void * ptr; /* 8 8 */ int refcnt; /* 16 4 */ /* XXX 4 bytes hole, try to pack */ struct idr handle_idr; /* 24 96 */ /* --- cacheline 1 boundary (64 bytes) was 56 bytes ago --- */ struct hlist_node hnode; /* 120 16 */ /* --- cacheline 2 boundary (128 bytes) was 8 bytes ago --- */ long int knodes; /* 136 8 */ /* size: 144, cachelines: 3, members: 6 */ /* sum members: 140, holes: 1, sum holes: 4 */ /* last cacheline: 16 bytes */ }; And with `sizeof(*tp_c)`, we have: sizeof(*tp_c) == sizeof(struct tc_u_common) == 144 == 0x90 (bytes) which is the correct and original allocation size. Fix this issue by replacing `struct_size(tp_c, hlist->ht, 1)` with `sizeof(*tp_c)`, and avoid allocating 8 too many bytes. The following difference in binary output is expected and reflects the desired change: | net/sched/cls_u32.o | @@ -6148,7 +6148,7 @@ | include/linux/slab.h:599 | 2cf5: mov 0x0(%rip),%rdi # 2cfc <u32_init+0xfc> | 2cf8: R_X86_64_PC32 kmalloc_caches+0xc |- 2cfc: mov $0x98,%edx |+ 2cfc: mov $0x90,%edx Reported-by: Alejandro Colomar <alx@kernel.org> Closes: https://lore.kernel.org/lkml/09b4a2ce-da74-3a19-6961-67883f634d98@kernel.org/ Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- Changes in v2: - Update subject line. - Update changelog text. v1: - Link: https://lore.kernel.org/linux-hardening/ZN5DvRyq6JNz20l1@work/ net/sched/cls_u32.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)