| Message ID | 1465042524-25852-7-git-send-email-leon@kernel.org (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On 6/4/2016 8:15 AM, Leon Romanovsky wrote: > From: Mark Bloch <markb@mellanox.com> > > In a attribute group struct, attrs should point to a NULL > terminated list. Which means we need to allocate one more > slot in the array. 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. > Fixes: b40f4757daa1 ("IB/core: Make device counter infrastructure dynamic") > Signed-off-by: Mark Bloch <markb@mellanox.com> > Signed-off-by: Leon Romanovsky <leon@kernel.org> > --- > drivers/infiniband/core/sysfs.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c > index 5e573bb..c68f132 100644 > --- a/drivers/infiniband/core/sysfs.c > +++ b/drivers/infiniband/core/sysfs.c > @@ -902,8 +902,11 @@ static void setup_hw_stats(struct ib_device *device, struct ib_port *port, > goto err; > > hsag = kzalloc(sizeof(*hsag) + > - // 1 extra for the lifespan config entry > - sizeof(void *) * (stats->num_counters + 1), > + /* 1 extra for the lifespan config entry, > + * and 1 more because attrs should be > + * NULL terminated. > + */ > + sizeof(void *) * (stats->num_counters + 2), > GFP_KERNEL); > if (!hsag) > return; >
On Mon, Jun 06, 2016 at 07:53:43PM -0400, Doug Ledford wrote: > On 6/4/2016 8:15 AM, Leon Romanovsky wrote: > > From: Mark Bloch <markb@mellanox.com> > > > > In a attribute group struct, attrs should point to a NULL > > terminated list. Which means we need to allocate one more > > slot in the array. > > 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. Thanks Doug.
> -----Original Message----- > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- > owner@vger.kernel.org] On Behalf Of Doug Ledford > Sent: Tuesday, June 07, 2016 2:54 AM > To: Leon Romanovsky <leon@kernel.org> > Cc: linux-rdma@vger.kernel.org; Mark Bloch <markb@mellanox.com> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/7/2016 3:16 AM, Leon Romanovsky wrote: > On Mon, Jun 06, 2016 at 07:53:43PM -0400, Doug Ledford wrote: >> On 6/4/2016 8:15 AM, Leon Romanovsky wrote: >>> From: Mark Bloch <markb@mellanox.com> >>> >>> In a attribute group struct, attrs should point to a NULL >>> terminated list. Which means we need to allocate one more >>> slot in the array. >> >> 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. > > Thanks Doug. > I was wrong with this BTW (I tested to make sure). Even though the struct includes the name for the array, because it's 0 length, it reserves 0 space. I thought it would reserve one u64. In any case, I ended up writing a fix of my own since I had already marked your patch as not needed in patchworks. I listed Mark as the reporter in that patch.
On 6/7/2016 4:26 AM, Mark Bloch wrote: > > >> -----Original Message----- >> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- >> owner@vger.kernel.org] On Behalf Of Doug Ledford >> Sent: Tuesday, June 07, 2016 2:54 AM >> To: Leon Romanovsky <leon@kernel.org> >> Cc: linux-rdma@vger.kernel.org; Mark Bloch <markb@mellanox.com> >> 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.
> -----Original Message----- > From: Doug Ledford [mailto:dledford@redhat.com] > Sent: Tuesday, June 07, 2016 4:46 PM > To: Mark Bloch <markb@mellanox.com>; Leon Romanovsky > <leon@kernel.org> > Cc: linux-rdma@vger.kernel.org > Subject: Re: [PATCH rdma-rc 6/7] IB/core: Fix incorrect array allocation > > Nope, you're right. It's fixed now though. with num_counters + 2 and > kzalloc, the final NULL terminator is in place. Cool, thank you, Mark -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 07, 2016 at 09:43:29AM -0400, Doug Ledford wrote: > On 6/7/2016 3:16 AM, Leon Romanovsky wrote: > > On Mon, Jun 06, 2016 at 07:53:43PM -0400, Doug Ledford wrote: > >> On 6/4/2016 8:15 AM, Leon Romanovsky wrote: > >>> From: Mark Bloch <markb@mellanox.com> > >>> > >>> In a attribute group struct, attrs should point to a NULL > >>> terminated list. Which means we need to allocate one more > >>> slot in the array. > >> > >> 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. > > > > Thanks Doug. > > > > I was wrong with this BTW (I tested to make sure). Even though the > struct includes the name for the array, because it's 0 length, it > reserves 0 space. I thought it would reserve one u64. In any case, I > ended up writing a fix of my own since I had already marked your patch > as not needed in patchworks. I listed Mark as the reporter in that patch. Since I know that you are testing the patches, and your "I'll do that." was enough for me. Thanks. >
diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c index 5e573bb..c68f132 100644 --- a/drivers/infiniband/core/sysfs.c +++ b/drivers/infiniband/core/sysfs.c @@ -902,8 +902,11 @@ static void setup_hw_stats(struct ib_device *device, struct ib_port *port, goto err; hsag = kzalloc(sizeof(*hsag) + - // 1 extra for the lifespan config entry - sizeof(void *) * (stats->num_counters + 1), + /* 1 extra for the lifespan config entry, + * and 1 more because attrs should be + * NULL terminated. + */ + sizeof(void *) * (stats->num_counters + 2), GFP_KERNEL); if (!hsag) return;