Messages in this thread | | | Date | Thu, 17 Aug 2023 14:39:56 +0200 | Subject | Re: struct_size() using sizeof() vs offsetof() | From | Alejandro Colomar <> |
| |
Hi!
On 2023-08-17 05:05, Kees Cook wrote: > On Thu, Aug 17, 2023 at 02:23:21AM +0200, Alejandro Colomar wrote: [...]
> > When struct_size() was originally implemented this topic came up, and we > opted for potential over-estimation rather than using offsetof() which > could result in under-allocation, and using max() of two different > calculations just seemed like overkill. Additionally, almost all cases of > struct_size() was replacing a literal open-coded version of > > sizeof(*ptr) + sizeof(*ptr->array) * count > > So avoiding a difference in calculation was nice too.
Yup.
[...]
>> >> MAX(sizeof(s), offsetof(s, fam) + sizeof_member(s, fam) * count) > > Ironically, this has been under careful examination recently by GCC[1] > too. Though that has mainly been looking at it from the perspective > of how __builtin_object_size() should behave in the face of the new > __counted_by attribute.
Heh, I've found that there are actually a lot of discussions about flex arrays going on this summer. Glibc also has something.
[...]
> > We opted for simple over complex, with the understanding that > over-allocation will be a relatively rare issue that will only waste > limited space (as opposed to potential under-allocation and risking > writing beyond the end of the region).
Thanks.
[...]
> But, yes, at the end of the day, struct_size() could be defined as > max(sizeof, offsetof-based struct-size). > > Note that struct_size() has been designed to have two additional > behaviors: > - be usable as a constant expression > - saturate at SIZE_MAX > > So as long as the max() could do the same (which it should be able to), > it'd likely be fine.
Yep. It should be able to do that.
> I'm open to patches as long as we can validate any > binary differences found in allmodconfig builds. :)
Thanks! I'm preparing a patch. It's being more complex than I thought it would be. There's some thing that's not compiling for me.
net/sched/cls_u32.c: In function ‘u32_init’: net/sched/cls_u32.c:369:17: error: cannot apply ‘offsetof’ to a non constant address 369 | tp_c = kzalloc(struct_size(tp_c, hlist->ht, 1), GFP_KERNEL); | ^~~~ In file included from ./include/linux/kernel.h:27, from ./arch/x86/include/asm/percpu.h:27, from ./arch/x86/include/asm/nospec-branch.h:14, from ./arch/x86/include/asm/paravirt_types.h:27, from ./arch/x86/include/asm/ptrace.h:97, from ./arch/x86/include/asm/math_emu.h:5, from ./arch/x86/include/asm/processor.h:13, from ./arch/x86/include/asm/timex.h:5, from ./include/linux/timex.h:67, from ./include/linux/time32.h:13, from ./include/linux/time.h:60, from ./include/linux/stat.h:19, from ./include/linux/module.h:13, from net/sched/cls_u32.c:26:
Here's the entire function:
$ grepc u32_init ./net/sched/cls_u32.c:352: static int u32_init(struct tcf_proto *tp) { struct tc_u_hnode *root_ht; void *key = tc_u_common_ptr(tp); struct tc_u_common *tp_c = tc_u_common_find(key);
root_ht = kzalloc(struct_size(root_ht, ht, 1), GFP_KERNEL); if (root_ht == NULL) return -ENOBUFS;
root_ht->refcnt++; root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : 0x80000000; root_ht->prio = tp->prio; root_ht->is_root = true; idr_init(&root_ht->handle_idr);
if (tp_c == NULL) { tp_c = kzalloc(struct_size(tp_c, hlist->ht, 1), GFP_KERNEL); if (tp_c == NULL) { kfree(root_ht); return -ENOBUFS; } tp_c->ptr = key; INIT_HLIST_NODE(&tp_c->hnode); idr_init(&tp_c->handle_idr);
hlist_add_head(&tp_c->hnode, tc_u_hash(key)); }
tp_c->refcnt++; RCU_INIT_POINTER(root_ht->next, tp_c->hlist); rcu_assign_pointer(tp_c->hlist, root_ht);
root_ht->refcnt++; rcu_assign_pointer(tp->root, root_ht); tp->data = tp_c; return 0; }
Let's see the structure type:
$ grepc -tt tc_u_common ./net/sched/cls_u32.c:86: struct tc_u_common { struct tc_u_hnode __rcu *hlist; void *ptr; int refcnt; struct idr handle_idr; struct hlist_node hnode; long knodes; };
Huh, hlist is the first field and is a pointer. I'm not at all sure of what was being done here. Here's the type of the pointee:
$ grepc -tt tc_u_hnode ./net/sched/cls_u32.c:70: struct tc_u_hnode { struct tc_u_hnode __rcu *next; u32 handle; u32 prio; int refcnt; unsigned int divisor; struct idr handle_idr; bool is_root; struct rcu_head rcu; u32 flags; /* The 'ht' field MUST be the last field in structure to allow for * more entries allocated at end of structure. */ struct tc_u_knode __rcu *ht[]; };
So, that struct_size() was, at best, doing black magic. At worst, a bug. I would need to investigate that code a little bit more, but a first guess tells me that struct_size() was returning the size of the outer structure plus the size of the flex array in the inner structure, but not including the size of the inner structure; i.e.:
sizeof(outer) + flex_array_size(inner-flex)
which seems a weird calcualtion.
That line of code was written by Gustavo, in d61491a51f7e ("net/sched: cls_u32: Replace one-element array with flexible-array member"), so can you please confirm that code, and maybe explain why it's that way, Gustavo?
- tp_c = kzalloc(sizeof(*tp_c), GFP_KERNEL); + tp_c = kzalloc(struct_size(tp_c, hlist->ht, 1), GFP_KERNEL);
Cheers, Alex
> > -Kees > > [1] https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626672.html >
-- <http://www.alejandro-colomar.es/> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
[unhandled content-type:application/pgp-signature] | |