On Mon, May 18 2020 at 14:01, Gustavo A. R. Silva wrote:
> The current codebase makes use of one-element arrays in the following
> form:
>
> struct something {
> int length;
> u8 data[1];
> };
>
> struct something *instance;
>
> instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
> instance->length = size;
> memcpy(instance->data, source, size);
>
> but the preferred mechanism to declare variable-length types such as
> these ones is a flexible array member[1][2], introduced in C99:
>
> struct foo {
> int stuff;
> struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on. So, replace
> the one-element array with a flexible-array member.
>
> Also, make use of the new struct_size() helper to properly calculate the
> total size needed to allocate dynamic memory for struct uv_rtc_timer_head.
> Notice that, due to the use of a one-element array, space for an extra
> struct cpu:
>
> struct {
> int lcpu; /* systemwide logical cpu number */
> u64 expires; /* next timer expiration for this cpu */
> } cpu[1]
>
> was being allocated at the moment of applying the sizeof operator to
> struct uv_rtc_timer_head in the call to kmalloc_node() at line 159:
>
> 159 head = kmalloc_node(sizeof(struct uv_rtc_timer_head) +
> 160 (uv_blade_nr_possible_cpus(bid) *
> 161 2 * sizeof(u64)),
> 162 GFP_KERNEL, nid);
>
> but that extra cpu[] was never actually being accessed due to the
> following piece of code at line 168:
>
> 168 head->ncpus = uv_blade_nr_possible_cpus(bid);
>
> and the piece of code at line 187:
>
> 187 for (c = 0; c < head->ncpus; c++) {
> 188 u64 exp = head->cpu[c].expires;
> 189 if (exp < lowest) {
> 190 bcpu = c;
> 191 lowest = exp;
> 192 }
> 193 }
>
> so heap space was being wasted.
>
> Another thing important to notice is that through the use of the
> struct_size() helper, code at line 161:
>
> 161 2 * sizeof(u64)),
>
> is changed to now be the actual size of struct cpu; see
> sizeof(*(p)->member) at include/linux/overflow.h:314:
>
> 314 #define struct_size(p, member, n) \
> 315 __ab_c_size(n, \
> 316 sizeof(*(p)->member) + __must_be_array((p)->member),\
> 317 sizeof(*(p)))
>
> As a side note, the original developer could have implemented code at line
> 161: 2 * sizeof(64) as follows:
>
> sizeof(*head->cpu)
This changelog is an unparseable pile of word salad wasting brain heap
space.
The gist is:
Variable sized arrays at the end of a struct should be defined as
unsized array foo[] not as foo[1].
struct uv_rtc_timer_head contains a sized array cpu[1].
Switch it to an unsized array and use the struct_size() helper to
calculate the allocation size.
Thanks,
tglx
