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

[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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature