| Message ID | a62560af06ba82c88ef9194982bfa63d14768ff9.1716900410.git.leon@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Batch of mlx5 fixes for v6.10 | expand |
On Tue, May 28, 2024 at 03:52:51PM +0300, Leon Romanovsky wrote: > From: Leon Romanovsky <leonro@nvidia.com> > > When the table is released, we nullify pointer to GID table, it means > that in case GID entry leak is detected, we will leak table too. > > Delete code that prevents table destruction. This converts a memory leak into a UAF, it doesn't seem like a good direction?? Jason
On Tue, Jun 04, 2024 at 01:36:36PM -0300, Jason Gunthorpe wrote: > On Tue, May 28, 2024 at 03:52:51PM +0300, Leon Romanovsky wrote: > > From: Leon Romanovsky <leonro@nvidia.com> > > > > When the table is released, we nullify pointer to GID table, it means > > that in case GID entry leak is detected, we will leak table too. > > > > Delete code that prevents table destruction. > > This converts a memory leak into a UAF, it doesn't seem like a good direction?? Maybe we should convert dev_err() to be WARN_ON(). I didn't see any complains about GID entry leaks. It is debug print. Thanks > > Jason >
On Wed, Jun 05, 2024 at 12:44:56PM +0300, Leon Romanovsky wrote: > On Tue, Jun 04, 2024 at 01:36:36PM -0300, Jason Gunthorpe wrote: > > On Tue, May 28, 2024 at 03:52:51PM +0300, Leon Romanovsky wrote: > > > From: Leon Romanovsky <leonro@nvidia.com> > > > > > > When the table is released, we nullify pointer to GID table, it means > > > that in case GID entry leak is detected, we will leak table too. > > > > > > Delete code that prevents table destruction. > > > > This converts a memory leak into a UAF, it doesn't seem like a good direction?? > > Maybe we should convert dev_err() to be WARN_ON(). I didn't see any > complains about GID entry leaks. It is debug print. Yes WARN_ON is better Jason
On Tue, May 28, 2024 at 03:52:51PM +0300, Leon Romanovsky wrote: > From: Leon Romanovsky <leonro@nvidia.com> > > When the table is released, we nullify pointer to GID table, it means > that in case GID entry leak is detected, we will leak table too. > > Delete code that prevents table destruction. > > Fixes: b150c3862d21 ("IB/core: Introduce GID entry reference counts") > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > --- > drivers/infiniband/core/cache.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) Since this is causing syzkaller failures and it really doesn't seem like stable material I'm dropping it from the rc branch and putting it in for-next. Lets see if we can fix the failures before the merge window. Jason
On Fri, Jun 21, 2024 at 10:21:39AM -0300, Jason Gunthorpe wrote: > On Tue, May 28, 2024 at 03:52:51PM +0300, Leon Romanovsky wrote: > > From: Leon Romanovsky <leonro@nvidia.com> > > > > When the table is released, we nullify pointer to GID table, it means > > that in case GID entry leak is detected, we will leak table too. > > > > Delete code that prevents table destruction. > > > > Fixes: b150c3862d21 ("IB/core: Introduce GID entry reference counts") > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > > --- > > drivers/infiniband/core/cache.c | 13 ++++--------- > > 1 file changed, 4 insertions(+), 9 deletions(-) > > Since this is causing syzkaller failures and it really doesn't seem > like stable material I'm dropping it from the rc branch and putting it > in for-next. Lets see if we can fix the failures before the merge window. I don't see it in the for-next branch, can you please add it? Thanks > > Jason
diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c index c02a96d3572a..aa62c8c7ca75 100644 --- a/drivers/infiniband/core/cache.c +++ b/drivers/infiniband/core/cache.c @@ -794,7 +794,6 @@ static struct ib_gid_table *alloc_gid_table(int sz) static void release_gid_table(struct ib_device *device, struct ib_gid_table *table) { - bool leak = false; int i; if (!table) @@ -803,15 +802,11 @@ static void release_gid_table(struct ib_device *device, for (i = 0; i < table->sz; i++) { if (is_gid_entry_free(table->data_vec[i])) continue; - if (kref_read(&table->data_vec[i]->kref) > 1) { - dev_err(&device->dev, - "GID entry ref leak for index %d ref=%u\n", i, - kref_read(&table->data_vec[i]->kref)); - leak = true; - } + + dev_err(&device->dev, + "GID entry ref leak for index %d ref=%u\n", i, + kref_read(&table->data_vec[i]->kref)); } - if (leak) - return; mutex_destroy(&table->lock); kfree(table->data_vec);