| Message ID | 1478184265-9620-4-git-send-email-leon@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Nov 03, 2016 at 04:44:11PM +0200, Leon Romanovsky wrote: > The failure in ib_cache_setup_one function during > ib_register_device will leave leaked allocated memory. > > Fixes: 03db3a2d81e6 ("IB/core: Add RoCE GID table management") > Signed-off-by: Leon Romanovsky <leon@kernel.org> > --- > drivers/infiniband/core/cache.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c > index 1a2984c..ae04826 100644 > --- a/drivers/infiniband/core/cache.c > +++ b/drivers/infiniband/core/cache.c > @@ -770,12 +770,8 @@ static int _gid_table_setup_one(struct ib_device *ib_dev) > int err = 0; > > table = kcalloc(ib_dev->phys_port_cnt, sizeof(*table), GFP_KERNEL); > - > - if (!table) { > - pr_warn("failed to allocate ib gid cache for %s\n", > - ib_dev->name); > + if (!table) > return -ENOMEM; > - } NIT: I think this would be worth separating out. > > for (port = 0; port < ib_dev->phys_port_cnt; port++) { > u8 rdma_port = port + rdma_start_port(ib_dev); > @@ -1170,14 +1166,13 @@ int ib_cache_setup_one(struct ib_device *device) > GFP_KERNEL); > if (!device->cache.pkey_cache || > !device->cache.lmc_cache) { > - pr_warn("Couldn't allocate cache for %s\n", device->name); > - return -ENOMEM; > + err = -ENOMEM; > + goto free; > } > > err = gid_table_setup_one(device); > if (err) > - /* Allocated memory will be cleaned in the release function */ > - return err; > + goto free; > > for (p = 0; p <= rdma_end_port(device) - rdma_start_port(device); ++p) > ib_cache_update(device, p + rdma_start_port(device)); > @@ -1192,6 +1187,9 @@ int ib_cache_setup_one(struct ib_device *device) > > err: > gid_table_cleanup_one(device); > +free: > + kfree(device->cache.pkey_cache); > + kfree(device->cache.lmc_cache); Despite the fact that another thread said this is supposed to be ok because ib_cache_release_one free's these I much prefer what you have done here. However, don't you need to NULL these out so that ib_cache_release_one can safely call kfree again? Ira > return err; > } > > -- > 2.7.4 > > -- > 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 -- 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 Thu, Dec 15, 2016 at 10:42:19PM -0500, ira.weiny wrote: > > err: > > gid_table_cleanup_one(device); > > +free: > > + kfree(device->cache.pkey_cache); > > + kfree(device->cache.lmc_cache); > > Despite the fact that another thread said this is supposed to be ok because > ib_cache_release_one free's these I much prefer what you have done here. > > However, don't you need to NULL these out so that ib_cache_release_one can > safely call kfree again? Yes. I feel deja vue here, like I've pointed this out before and the resolution was to just rely on the release function so we don't have bugs like this. Jason -- 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 Thu, Dec 15, 2016 at 09:27:43PM -0700, Jason Gunthorpe wrote: > On Thu, Dec 15, 2016 at 10:42:19PM -0500, ira.weiny wrote: > > > err: > > > gid_table_cleanup_one(device); > > > +free: > > > + kfree(device->cache.pkey_cache); > > > + kfree(device->cache.lmc_cache); > > > > Despite the fact that another thread said this is supposed to be ok because > > ib_cache_release_one free's these I much prefer what you have done here. > > > > However, don't you need to NULL these out so that ib_cache_release_one can > > safely call kfree again? > > Yes. > > I feel deja vue here, like I've pointed this out before and the > resolution was to just rely on the release function so we don't have > bugs like this. I'm sure that I missed it, can you point me to that discussion? Thanks > > Jason
diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c index 1a2984c..ae04826 100644 --- a/drivers/infiniband/core/cache.c +++ b/drivers/infiniband/core/cache.c @@ -770,12 +770,8 @@ static int _gid_table_setup_one(struct ib_device *ib_dev) int err = 0; table = kcalloc(ib_dev->phys_port_cnt, sizeof(*table), GFP_KERNEL); - - if (!table) { - pr_warn("failed to allocate ib gid cache for %s\n", - ib_dev->name); + if (!table) return -ENOMEM; - } for (port = 0; port < ib_dev->phys_port_cnt; port++) { u8 rdma_port = port + rdma_start_port(ib_dev); @@ -1170,14 +1166,13 @@ int ib_cache_setup_one(struct ib_device *device) GFP_KERNEL); if (!device->cache.pkey_cache || !device->cache.lmc_cache) { - pr_warn("Couldn't allocate cache for %s\n", device->name); - return -ENOMEM; + err = -ENOMEM; + goto free; } err = gid_table_setup_one(device); if (err) - /* Allocated memory will be cleaned in the release function */ - return err; + goto free; for (p = 0; p <= rdma_end_port(device) - rdma_start_port(device); ++p) ib_cache_update(device, p + rdma_start_port(device)); @@ -1192,6 +1187,9 @@ int ib_cache_setup_one(struct ib_device *device) err: gid_table_cleanup_one(device); +free: + kfree(device->cache.pkey_cache); + kfree(device->cache.lmc_cache); return err; }
The failure in ib_cache_setup_one function during ib_register_device will leave leaked allocated memory. Fixes: 03db3a2d81e6 ("IB/core: Add RoCE GID table management") Signed-off-by: Leon Romanovsky <leon@kernel.org> --- drivers/infiniband/core/cache.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)