Re: [PATCH rdma-rc 6/7] IB/core: Fix incorrect array allocation

Doug Ledford
2016-06-07T13:46:20Z

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

On 6/7/2016 4:26 AM, Mark Bloch wrote:
>
>
>> -----Original Message-----
>> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
>> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Doug Ledford
>> Sent: Tuesday, June 07, 2016 2:54 AM
>> To: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> Subject: Re: [PATCH rdma-rc 6/7] IB/core: Fix incorrect array allocation
>>
>> This patch is both right and wrong. You're right with the intent (that
>> there should be a total of 2 extra entries in the array size, one for
>> the NULL termination and one for the lifespan entry), but wrong with the
>> mechanics (unless I've missed something). We already have two extra
>> entries because sizeof(*hsag) includes our first counter, so just
>> num_counters is actually enough to NULL terminate the list, and + 1
>> gives us lifespan plus a NULL terminate spot. The comment could be
>> cleaned up to make this more clear though, so I'll do that.
>
> Hi doug,
> I might be missing something, but:
>
> hsag = kzalloc(sizeof(*hsag) +
> sizeof(void *) * (stats->num_counters + 1)
> GFP_KERNEL);
>
> So now we have hsag and after it num_counters + 1 slots.
> Now we need attrs to point to a memory location, so we do:
>
> hsag->attrs = (void *)hsag + sizeof(*hsag);
>
> which means hsag->attrs is now pointing to a memory location right
> after hsag (remember we have num_counters + 1) slots there.
>
> for (i = 0; i < stats->num_counters; i++) {
> hsag->attrs[i] = alloc_hsa(i, port_num, stats->names[i]);
> if (!hsag->attrs[i])
> goto err;
> sysfs_attr_init(hsag->attrs[i]);
> }
>
> /* treat an error here as non-fatal */
> hsag->attrs[i] = alloc_hsa_lifespan("lifespan", port_num);
>
> The for loop fills num_counters slots, and after that alloc_hsa_lifespan uses another one.
> Which means we used all our slots and we are missing one as the NULL slot.
>
> Of course I might be wrong/missing something, it wouldn't be the first time :)
>
> Mark
>

Nope, you're right. It's fixed now though. with num_counters + 2 and
kzalloc, the final NULL terminator is in place.

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