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

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- 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 --]