Message ID | 20211014035338.3750416-1-yangyingliang@huawei.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | iio: accel: kxcjk-1013: Fix possible memory leak in probe and remove | expand |
Hi, On 10/14/21 5:53 AM, Yang Yingliang wrote: > When ACPI type is ACPI_SMO8500, the data->dready_trig will not be set, the > memory allocated by iio_triggered_buffer_setup() will not be freed, and cause > memory leak as follows: > > unreferenced object 0xffff888009551400 (size 512): > comm "i2c-SMO8500-125", pid 911, jiffies 4294911787 (age 83.852s) > hex dump (first 32 bytes): > 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 00 00 00 00 00 00 00 00 20 e2 e5 c0 ff ff ff ff ........ ....... > backtrace: > [<0000000041ce75ee>] kmem_cache_alloc_trace+0x16d/0x360 > [<000000000aeb17b0>] iio_kfifo_allocate+0x41/0x130 [kfifo_buf] > [<000000004b40c1f5>] iio_triggered_buffer_setup_ext+0x2c/0x210 [industrialio_triggered_buffer] > [<000000004375b15f>] kxcjk1013_probe+0x10c3/0x1d81 [kxcjk_1013] > [<0000000020115b9a>] i2c_device_probe+0xa31/0xbe0 > [<00000000d9f581a6>] really_probe+0x299/0xc30 > [<00000000c6c16cde>] __driver_probe_device+0x357/0x500 > [<00000000909852a1>] driver_probe_device+0x4e/0x140 > [<000000008419ba53>] __device_attach_driver+0x257/0x340 > [<00000000533bb466>] bus_for_each_drv+0x166/0x1e0 > [<000000005bf45d75>] __device_attach+0x272/0x420 > [<0000000075220311>] bus_probe_device+0x1eb/0x2a0 > [<0000000015587e85>] device_add+0xbf0/0x1f90 > [<0000000086901b9e>] i2c_new_client_device+0x622/0xb20 > [<000000000865ca18>] new_device_store+0x1fa/0x420 > [<0000000059a3d183>] dev_attr_store+0x58/0x80 > > Fix it by remove data->dready_trig condition in probe and remove. > > Reported-by: Hulk Robot <hulkci@huawei.com> > Fixes: a25691c1f967 ("iio: accel: kxcjk1013: allow using an external trigger") > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> Hmm, wouldn't the right fix be to also move the iio_triggered_buffer_setup() call to inside the: if (client->irq > 0 && data->acpi_type != ACPI_SMO8500) { } block ? Jonathan (jic23) can you take a look at this, to me it seems that having a triggered buffer allocated without any triggers is not useful ? Regards, Hans > --- > drivers/iio/accel/kxcjk-1013.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c > index a51fdd3c9b5b..24c9387c2968 100644 > --- a/drivers/iio/accel/kxcjk-1013.c > +++ b/drivers/iio/accel/kxcjk-1013.c > @@ -1595,8 +1595,7 @@ static int kxcjk1013_probe(struct i2c_client *client, > return 0; > > err_buffer_cleanup: > - if (data->dready_trig) > - iio_triggered_buffer_cleanup(indio_dev); > + iio_triggered_buffer_cleanup(indio_dev); > err_trigger_unregister: > if (data->dready_trig) > iio_trigger_unregister(data->dready_trig); > @@ -1618,8 +1617,8 @@ static int kxcjk1013_remove(struct i2c_client *client) > pm_runtime_disable(&client->dev); > pm_runtime_set_suspended(&client->dev); > > + iio_triggered_buffer_cleanup(indio_dev); > if (data->dready_trig) { > - iio_triggered_buffer_cleanup(indio_dev); > iio_trigger_unregister(data->dready_trig); > iio_trigger_unregister(data->motion_trig); > } >
Hi, On 2021/10/14 22:54, Andy Shevchenko wrote: > On Thu, Oct 14, 2021 at 11:53:38AM +0800, Yang Yingliang wrote: >> When ACPI type is ACPI_SMO8500, the data->dready_trig will not be set, the >> memory allocated by iio_triggered_buffer_setup() will not be freed, and cause >> memory leak as follows: > It seems it's not first time I'm telling you to shrink the noise in the commit > message. Can you please LEARN this once and forever? Thanks for you advice, I searched the whole kernel source tree commit message to learn how to make the memory leak report, I found most of them using the whole backtrace, so I make the report like they did in this patch. I will shrink the noise in v2. How about shrink it like this: unreferenced object 0xffff888009551400 (size 512): comm "i2c-SMO8500-125", pid 911, jiffies 4294911787 (age 83.852s) hex dump (first 32 bytes): 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00 00 00 00 00 00 00 00 20 e2 e5 c0 ff ff ff ff ........ ....... backtrace: [<0000000041ce75ee>] kmem_cache_alloc_trace+0x16d/0x360 [<000000000aeb17b0>] iio_kfifo_allocate+0x41/0x130 [kfifo_buf] [<000000004b40c1f5>] iio_triggered_buffer_setup_ext+0x2c/0x210 [industrialio_triggered_buffer] [<000000004375b15f>] kxcjk1013_probe+0x10c3/0x1d81 [kxcjk_1013] Thanks, Yang > >> unreferenced object 0xffff888009551400 (size 512): >> comm "i2c-SMO8500-125", pid 911, jiffies 4294911787 (age 83.852s) >> hex dump (first 32 bytes): >> 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >> 00 00 00 00 00 00 00 00 20 e2 e5 c0 ff ff ff ff ........ ....... >> backtrace: >> [<0000000041ce75ee>] kmem_cache_alloc_trace+0x16d/0x360 >> [<000000000aeb17b0>] iio_kfifo_allocate+0x41/0x130 [kfifo_buf] >> [<000000004b40c1f5>] iio_triggered_buffer_setup_ext+0x2c/0x210 [industrialio_triggered_buffer] >> [<000000004375b15f>] kxcjk1013_probe+0x10c3/0x1d81 [kxcjk_1013] >> [<0000000020115b9a>] i2c_device_probe+0xa31/0xbe0 >> [<00000000d9f581a6>] really_probe+0x299/0xc30 >> [<00000000c6c16cde>] __driver_probe_device+0x357/0x500 >> [<00000000909852a1>] driver_probe_device+0x4e/0x140 >> [<000000008419ba53>] __device_attach_driver+0x257/0x340 >> [<00000000533bb466>] bus_for_each_drv+0x166/0x1e0 >> [<000000005bf45d75>] __device_attach+0x272/0x420 >> [<0000000075220311>] bus_probe_device+0x1eb/0x2a0 >> [<0000000015587e85>] device_add+0xbf0/0x1f90 >> [<0000000086901b9e>] i2c_new_client_device+0x622/0xb20 >> [<000000000865ca18>] new_device_store+0x1fa/0x420 >> [<0000000059a3d183>] dev_attr_store+0x58/0x80
On Thu, Oct 14, 2021 at 11:53:38AM +0800, Yang Yingliang wrote: > When ACPI type is ACPI_SMO8500, the data->dready_trig will not be set, the > memory allocated by iio_triggered_buffer_setup() will not be freed, and cause > memory leak as follows: It seems it's not first time I'm telling you to shrink the noise in the commit message. Can you please LEARN this once and forever? > unreferenced object 0xffff888009551400 (size 512): > comm "i2c-SMO8500-125", pid 911, jiffies 4294911787 (age 83.852s) > hex dump (first 32 bytes): > 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 00 00 00 00 00 00 00 00 20 e2 e5 c0 ff ff ff ff ........ ....... > backtrace: > [<0000000041ce75ee>] kmem_cache_alloc_trace+0x16d/0x360 > [<000000000aeb17b0>] iio_kfifo_allocate+0x41/0x130 [kfifo_buf] > [<000000004b40c1f5>] iio_triggered_buffer_setup_ext+0x2c/0x210 [industrialio_triggered_buffer] > [<000000004375b15f>] kxcjk1013_probe+0x10c3/0x1d81 [kxcjk_1013] > [<0000000020115b9a>] i2c_device_probe+0xa31/0xbe0 > [<00000000d9f581a6>] really_probe+0x299/0xc30 > [<00000000c6c16cde>] __driver_probe_device+0x357/0x500 > [<00000000909852a1>] driver_probe_device+0x4e/0x140 > [<000000008419ba53>] __device_attach_driver+0x257/0x340 > [<00000000533bb466>] bus_for_each_drv+0x166/0x1e0 > [<000000005bf45d75>] __device_attach+0x272/0x420 > [<0000000075220311>] bus_probe_device+0x1eb/0x2a0 > [<0000000015587e85>] device_add+0xbf0/0x1f90 > [<0000000086901b9e>] i2c_new_client_device+0x622/0xb20 > [<000000000865ca18>] new_device_store+0x1fa/0x420 > [<0000000059a3d183>] dev_attr_store+0x58/0x80
On Thu, Oct 14, 2021 at 09:38:52PM +0800, Yang Yingliang wrote: > On 2021/10/14 22:54, Andy Shevchenko wrote: > > On Thu, Oct 14, 2021 at 11:53:38AM +0800, Yang Yingliang wrote: > > > When ACPI type is ACPI_SMO8500, the data->dready_trig will not be set, the > > > memory allocated by iio_triggered_buffer_setup() will not be freed, and cause > > > memory leak as follows: > > It seems it's not first time I'm telling you to shrink the noise in the commit > > message. Can you please LEARN this once and forever? > Thanks for you advice, I searched the whole kernel source tree commit > message to > learn how to make the memory leak report, I found most of them using the > whole > backtrace, so I make the report like they did in this patch. Some maintainers do not care about bloated sizes of the commit messages, however there are several advantages to have them shorter: 1/ saving resources (followed by disk storages and energy around the world, means being environment friendly for real); 2/ when reading log, noise make it much harder to understand, besides the fact that reading itself will be time consuming; 3/ nowadays some people are tending to read or even discuss the changes on the mobile devices, where screen size is not so big; 4/ the copied'n'pasted backtrace means that the contributor probably haven't thought through it and the quality of the change is in doubt. > I will shrink > the noise in v2. > How about shrink it like this: Much better!
On Thu, 14 Oct 2021 10:20:34 +0200 Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 10/14/21 5:53 AM, Yang Yingliang wrote: > > When ACPI type is ACPI_SMO8500, the data->dready_trig will not be set, the > > memory allocated by iio_triggered_buffer_setup() will not be freed, and cause > > memory leak as follows: > > > > unreferenced object 0xffff888009551400 (size 512): > > comm "i2c-SMO8500-125", pid 911, jiffies 4294911787 (age 83.852s) > > hex dump (first 32 bytes): > > 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > 00 00 00 00 00 00 00 00 20 e2 e5 c0 ff ff ff ff ........ ....... > > backtrace: > > [<0000000041ce75ee>] kmem_cache_alloc_trace+0x16d/0x360 > > [<000000000aeb17b0>] iio_kfifo_allocate+0x41/0x130 [kfifo_buf] > > [<000000004b40c1f5>] iio_triggered_buffer_setup_ext+0x2c/0x210 [industrialio_triggered_buffer] > > [<000000004375b15f>] kxcjk1013_probe+0x10c3/0x1d81 [kxcjk_1013] > > [<0000000020115b9a>] i2c_device_probe+0xa31/0xbe0 > > [<00000000d9f581a6>] really_probe+0x299/0xc30 > > [<00000000c6c16cde>] __driver_probe_device+0x357/0x500 > > [<00000000909852a1>] driver_probe_device+0x4e/0x140 > > [<000000008419ba53>] __device_attach_driver+0x257/0x340 > > [<00000000533bb466>] bus_for_each_drv+0x166/0x1e0 > > [<000000005bf45d75>] __device_attach+0x272/0x420 > > [<0000000075220311>] bus_probe_device+0x1eb/0x2a0 > > [<0000000015587e85>] device_add+0xbf0/0x1f90 > > [<0000000086901b9e>] i2c_new_client_device+0x622/0xb20 > > [<000000000865ca18>] new_device_store+0x1fa/0x420 > > [<0000000059a3d183>] dev_attr_store+0x58/0x80 > > > > Fix it by remove data->dready_trig condition in probe and remove. > > > > Reported-by: Hulk Robot <hulkci@huawei.com> > > Fixes: a25691c1f967 ("iio: accel: kxcjk1013: allow using an external trigger") > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > Hmm, wouldn't the right fix be to also move the > iio_triggered_buffer_setup() call to inside the: > > if (client->irq > 0 && data->acpi_type != ACPI_SMO8500) { > } > > block ? > > Jonathan (jic23) can you take a look at this, to me it seems that having > a triggered buffer allocated without any triggers is not useful ? It can use another trigger not supplied by this particular device. e.g. sysfs or hrtimer trigger. This is common for cases where we may or may not have an irq wired and the validate_* callbacks are not provided (which would indicate we had to use the device's own trigger). Jonathan > > Regards, > > Hans > > > > > --- > > drivers/iio/accel/kxcjk-1013.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c > > index a51fdd3c9b5b..24c9387c2968 100644 > > --- a/drivers/iio/accel/kxcjk-1013.c > > +++ b/drivers/iio/accel/kxcjk-1013.c > > @@ -1595,8 +1595,7 @@ static int kxcjk1013_probe(struct i2c_client *client, > > return 0; > > > > err_buffer_cleanup: > > - if (data->dready_trig) > > - iio_triggered_buffer_cleanup(indio_dev); > > + iio_triggered_buffer_cleanup(indio_dev); > > err_trigger_unregister: > > if (data->dready_trig) > > iio_trigger_unregister(data->dready_trig); > > @@ -1618,8 +1617,8 @@ static int kxcjk1013_remove(struct i2c_client *client) > > pm_runtime_disable(&client->dev); > > pm_runtime_set_suspended(&client->dev); > > > > + iio_triggered_buffer_cleanup(indio_dev); > > if (data->dready_trig) { > > - iio_triggered_buffer_cleanup(indio_dev); > > iio_trigger_unregister(data->dready_trig); > > iio_trigger_unregister(data->motion_trig); > > } > > >
Hi, On 10/14/21 7:38 PM, Jonathan Cameron wrote: > On Thu, 14 Oct 2021 10:20:34 +0200 > Hans de Goede <hdegoede@redhat.com> wrote: > >> Hi, >> >> On 10/14/21 5:53 AM, Yang Yingliang wrote: >>> When ACPI type is ACPI_SMO8500, the data->dready_trig will not be set, the >>> memory allocated by iio_triggered_buffer_setup() will not be freed, and cause >>> memory leak as follows: >>> >>> unreferenced object 0xffff888009551400 (size 512): >>> comm "i2c-SMO8500-125", pid 911, jiffies 4294911787 (age 83.852s) >>> hex dump (first 32 bytes): >>> 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >>> 00 00 00 00 00 00 00 00 20 e2 e5 c0 ff ff ff ff ........ ....... >>> backtrace: >>> [<0000000041ce75ee>] kmem_cache_alloc_trace+0x16d/0x360 >>> [<000000000aeb17b0>] iio_kfifo_allocate+0x41/0x130 [kfifo_buf] >>> [<000000004b40c1f5>] iio_triggered_buffer_setup_ext+0x2c/0x210 [industrialio_triggered_buffer] >>> [<000000004375b15f>] kxcjk1013_probe+0x10c3/0x1d81 [kxcjk_1013] >>> [<0000000020115b9a>] i2c_device_probe+0xa31/0xbe0 >>> [<00000000d9f581a6>] really_probe+0x299/0xc30 >>> [<00000000c6c16cde>] __driver_probe_device+0x357/0x500 >>> [<00000000909852a1>] driver_probe_device+0x4e/0x140 >>> [<000000008419ba53>] __device_attach_driver+0x257/0x340 >>> [<00000000533bb466>] bus_for_each_drv+0x166/0x1e0 >>> [<000000005bf45d75>] __device_attach+0x272/0x420 >>> [<0000000075220311>] bus_probe_device+0x1eb/0x2a0 >>> [<0000000015587e85>] device_add+0xbf0/0x1f90 >>> [<0000000086901b9e>] i2c_new_client_device+0x622/0xb20 >>> [<000000000865ca18>] new_device_store+0x1fa/0x420 >>> [<0000000059a3d183>] dev_attr_store+0x58/0x80 >>> >>> Fix it by remove data->dready_trig condition in probe and remove. >>> >>> Reported-by: Hulk Robot <hulkci@huawei.com> >>> Fixes: a25691c1f967 ("iio: accel: kxcjk1013: allow using an external trigger") >>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >> >> Hmm, wouldn't the right fix be to also move the >> iio_triggered_buffer_setup() call to inside the: >> >> if (client->irq > 0 && data->acpi_type != ACPI_SMO8500) { >> } >> >> block ? >> >> Jonathan (jic23) can you take a look at this, to me it seems that having >> a triggered buffer allocated without any triggers is not useful ? > > It can use another trigger not supplied by this particular device. > e.g. sysfs or hrtimer trigger. This is common for cases where > we may or may not have an irq wired and the validate_* callbacks are > not provided (which would indicate we had to use the device's own trigger). Ok, thank you for clarifying this. With that resolved, this patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans >>> --- >>> drivers/iio/accel/kxcjk-1013.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c >>> index a51fdd3c9b5b..24c9387c2968 100644 >>> --- a/drivers/iio/accel/kxcjk-1013.c >>> +++ b/drivers/iio/accel/kxcjk-1013.c >>> @@ -1595,8 +1595,7 @@ static int kxcjk1013_probe(struct i2c_client *client, >>> return 0; >>> >>> err_buffer_cleanup: >>> - if (data->dready_trig) >>> - iio_triggered_buffer_cleanup(indio_dev); >>> + iio_triggered_buffer_cleanup(indio_dev); >>> err_trigger_unregister: >>> if (data->dready_trig) >>> iio_trigger_unregister(data->dready_trig); >>> @@ -1618,8 +1617,8 @@ static int kxcjk1013_remove(struct i2c_client *client) >>> pm_runtime_disable(&client->dev); >>> pm_runtime_set_suspended(&client->dev); >>> >>> + iio_triggered_buffer_cleanup(indio_dev); >>> if (data->dready_trig) { >>> - iio_triggered_buffer_cleanup(indio_dev); >>> iio_trigger_unregister(data->dready_trig); >>> iio_trigger_unregister(data->motion_trig); >>> } >>> >> >
diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c index a51fdd3c9b5b..24c9387c2968 100644 --- a/drivers/iio/accel/kxcjk-1013.c +++ b/drivers/iio/accel/kxcjk-1013.c @@ -1595,8 +1595,7 @@ static int kxcjk1013_probe(struct i2c_client *client, return 0; err_buffer_cleanup: - if (data->dready_trig) - iio_triggered_buffer_cleanup(indio_dev); + iio_triggered_buffer_cleanup(indio_dev); err_trigger_unregister: if (data->dready_trig) iio_trigger_unregister(data->dready_trig); @@ -1618,8 +1617,8 @@ static int kxcjk1013_remove(struct i2c_client *client) pm_runtime_disable(&client->dev); pm_runtime_set_suspended(&client->dev); + iio_triggered_buffer_cleanup(indio_dev); if (data->dready_trig) { - iio_triggered_buffer_cleanup(indio_dev); iio_trigger_unregister(data->dready_trig); iio_trigger_unregister(data->motion_trig); }
When ACPI type is ACPI_SMO8500, the data->dready_trig will not be set, the memory allocated by iio_triggered_buffer_setup() will not be freed, and cause memory leak as follows: unreferenced object 0xffff888009551400 (size 512): comm "i2c-SMO8500-125", pid 911, jiffies 4294911787 (age 83.852s) hex dump (first 32 bytes): 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00 00 00 00 00 00 00 00 20 e2 e5 c0 ff ff ff ff ........ ....... backtrace: [<0000000041ce75ee>] kmem_cache_alloc_trace+0x16d/0x360 [<000000000aeb17b0>] iio_kfifo_allocate+0x41/0x130 [kfifo_buf] [<000000004b40c1f5>] iio_triggered_buffer_setup_ext+0x2c/0x210 [industrialio_triggered_buffer] [<000000004375b15f>] kxcjk1013_probe+0x10c3/0x1d81 [kxcjk_1013] [<0000000020115b9a>] i2c_device_probe+0xa31/0xbe0 [<00000000d9f581a6>] really_probe+0x299/0xc30 [<00000000c6c16cde>] __driver_probe_device+0x357/0x500 [<00000000909852a1>] driver_probe_device+0x4e/0x140 [<000000008419ba53>] __device_attach_driver+0x257/0x340 [<00000000533bb466>] bus_for_each_drv+0x166/0x1e0 [<000000005bf45d75>] __device_attach+0x272/0x420 [<0000000075220311>] bus_probe_device+0x1eb/0x2a0 [<0000000015587e85>] device_add+0xbf0/0x1f90 [<0000000086901b9e>] i2c_new_client_device+0x622/0xb20 [<000000000865ca18>] new_device_store+0x1fa/0x420 [<0000000059a3d183>] dev_attr_store+0x58/0x80 Fix it by remove data->dready_trig condition in probe and remove. Reported-by: Hulk Robot <hulkci@huawei.com> Fixes: a25691c1f967 ("iio: accel: kxcjk1013: allow using an external trigger") Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- drivers/iio/accel/kxcjk-1013.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)