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 >> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Mark Bloch >> 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.