Re: struct_size() using sizeof() vs offsetof()

Alejandro Colomar
2023-08-17T12:40:48Z

[-- Attachment #1.1: Type: text/plain, Size: 5937 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]