| Message ID | 20240217135255.1128716-1-xiaolei.wang@windriver.com |
|---|---|
| State | New |
| Headers | show |
| Series | [linux-next,1/1] gpio: Delete excess allocated label memory | expand |
On Sat, Feb 17, 2024 at 2:53 PM Xiaolei Wang <xiaolei.wang@windriver.com> wrote: > > The changes in commit 1f2bcb8c8ccd ("gpio: protect the > descriptor label with SRCU"), desc_set_label has already > allocated memory space for the label, so there is no need > to allocate it again. otherwise memory leaks will be > introduced. > No, we *want* to copy it if it's not in .rodata for the same reason we introduced SRCU. This may be a valid report but the fix is wrong. > unreferenced object 0xffff0000c3e4d0c0 (size 32): > comm "kworker/u16:4", pid 60, jiffies 4294894555 > hex dump (first 32 bytes): > 72 65 67 75 6c 61 74 6f 72 2d 63 61 6e 32 2d 73 regulator-can2-s > 74 62 79 00 00 00 ff ff ff ff ff ff eb db ff ff tby............. > backtrace (crc 2c3a0350): > [<00000000e93c5cf4>] kmemleak_alloc+0x34/0x40 > [<0000000097a2657f>] __kmalloc_node_track_caller+0x2c4/0x524 > [<000000000dd1c057>] kstrdup+0x4c/0x98 > [<00000000b513a96a>] kstrdup_const+0x34/0x40 > [<000000008a7f0feb>] gpiod_request_commit+0xdc/0x358 > [<00000000fc71ad64>] gpiod_request+0xd8/0x204 > [<00000000fa24b091>] gpiod_find_and_request+0x170/0x780 > [<0000000086ecf92d>] gpiod_get_index+0x70/0xe0 > [<000000004aef97f9>] gpiod_get_optional+0x18/0x30 > [<00000000312f1b25>] reg_fixed_voltage_probe+0x58c/0xad8 > [<00000000e6f47635>] platform_probe+0xc4/0x198 > [<00000000cf78fbdb>] really_probe+0x204/0x5a8 > [<00000000e28d05ec>] __driver_probe_device+0x158/0x2c4 > [<00000000e4fe452b>] driver_probe_device+0x60/0x18c > [<00000000479fcf5d>] __device_attach_driver+0x168/0x208 > [<000000007d389f38>] bus_for_each_drv+0x104/0x190 > Can you post the full kmemleak report for this, please? Bart > Fixes: 1f2bcb8c8ccd ("gpio: protect the descriptor label with SRCU") > Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> > --- > drivers/gpio/gpiolib.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 02be0ba1a402..32191547dece 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -2250,12 +2250,6 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label) > if (test_and_set_bit(FLAG_REQUESTED, &desc->flags)) > return -EBUSY; > > - if (label) { > - label = kstrdup_const(label, GFP_KERNEL); > - if (!label) > - return -ENOMEM; > - } > - > /* NOTE: gpio_request() can be called in early boot, > * before IRQs are enabled, for non-sleeping (SOC) GPIOs. > */ > -- > 2.25.1 >
On Sun, Feb 18, 2024 at 1:55 AM xiaolei wang <xiaolei.wang@windriver.com> wrote: > > > On 2/18/24 02:52, Bartosz Golaszewski wrote: > > CAUTION: This email comes from a non Wind River email account! > > Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > > On Sat, 17 Feb 2024 19:35:43 +0100, Bartosz Golaszewski <brgl@bgdev.pl> said: > >> On Sat, Feb 17, 2024 at 2:53 PM Xiaolei Wang <xiaolei.wang@windriver.com> wrote: > >>> The changes in commit 1f2bcb8c8ccd ("gpio: protect the > >>> descriptor label with SRCU"), desc_set_label has already > >>> allocated memory space for the label, so there is no need > >>> to allocate it again. otherwise memory leaks will be > >>> introduced. > >>> > >> No, we *want* to copy it if it's not in .rodata for the same reason we > >> introduced SRCU. This may be a valid report but the fix is wrong. > >> > >>> unreferenced object 0xffff0000c3e4d0c0 (size 32): > >>> comm "kworker/u16:4", pid 60, jiffies 4294894555 > >>> hex dump (first 32 bytes): > >>> 72 65 67 75 6c 61 74 6f 72 2d 63 61 6e 32 2d 73 regulator-can2-s > >>> 74 62 79 00 00 00 ff ff ff ff ff ff eb db ff ff tby............. > >>> backtrace (crc 2c3a0350): > >>> [<00000000e93c5cf4>] kmemleak_alloc+0x34/0x40 > >>> [<0000000097a2657f>] __kmalloc_node_track_caller+0x2c4/0x524 > >>> [<000000000dd1c057>] kstrdup+0x4c/0x98 > >>> [<00000000b513a96a>] kstrdup_const+0x34/0x40 > >>> [<000000008a7f0feb>] gpiod_request_commit+0xdc/0x358 > >>> [<00000000fc71ad64>] gpiod_request+0xd8/0x204 > >>> [<00000000fa24b091>] gpiod_find_and_request+0x170/0x780 > >>> [<0000000086ecf92d>] gpiod_get_index+0x70/0xe0 > >>> [<000000004aef97f9>] gpiod_get_optional+0x18/0x30 > >>> [<00000000312f1b25>] reg_fixed_voltage_probe+0x58c/0xad8 > >>> [<00000000e6f47635>] platform_probe+0xc4/0x198 > >>> [<00000000cf78fbdb>] really_probe+0x204/0x5a8 > >>> [<00000000e28d05ec>] __driver_probe_device+0x158/0x2c4 > >>> [<00000000e4fe452b>] driver_probe_device+0x60/0x18c > >>> [<00000000479fcf5d>] __device_attach_driver+0x168/0x208 > >>> [<000000007d389f38>] bus_for_each_drv+0x104/0x190 > >>> > >> Can you post the full kmemleak report for this, please? > >> > >> Bart > >> > > Ah, I think I see the problem. Can you test the following diff: > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > index 02be0ba1a402..0fdd4ad242bd 100644 > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -695,10 +695,15 @@ EXPORT_SYMBOL_GPL(gpiochip_line_is_valid); > > static void gpiodev_release(struct device *dev) > > { > > struct gpio_device *gdev = to_gpio_device(dev); > > + struct gpio_desc *desc; > > unsigned int i; > > > > - for (i = 0; i < gdev->ngpio; i++) > > - cleanup_srcu_struct(&gdev->descs[i].srcu); > > + for (i = 0; i < gdev->ngpio; i++) { > > + desc = &gdev->descs[i]; > > + > > + kfree_const(desc->label); > > + cleanup_srcu_struct(&desc->srcu); > > + } > > > > ida_free(&gpio_ida, gdev->id); > > kfree_const(gdev->label); > > > > and let me know if it fixes the issue? > I tried the following modifications, which did not fix the problem, > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 02be0ba1a402..5940ef88399c 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -695,10 +695,14 @@ EXPORT_SYMBOL_GPL(gpiochip_line_is_valid); > static void gpiodev_release(struct device *dev) > { > struct gpio_device *gdev = to_gpio_device(dev); > + struct gpio_desc *desc; > unsigned int i; > > - for (i = 0; i < gdev->ngpio; i++) > - cleanup_srcu_struct(&gdev->descs[i].srcu); > + for (i = 0; i < gdev->ngpio; i++) { > + desc = &gdev->descs[i]; > + kfree_const(desc->label); > + cleanup_srcu_struct(&desc->srcu); > + } > > ida_free(&gpio_ida, gdev->id); > > kfree_const(gdev->label); > > > unreferenced object 0xffff0000c0e83740 (size 32): > comm "kworker/u16:0", pid 10, jiffies 4294894561 > hex dump (first 32 bytes): > 72 65 67 75 6c 61 74 6f 72 2d 75 73 64 68 63 32 regulator-usdhc2 > 00 37 e8 c0 00 00 ff ff 00 00 00 00 00 00 00 00 .7.............. > backtrace (crc 222fadd4): > [<00000000cd106dac>] kmemleak_alloc+0x34/0x40 > [<00000000e084cf5f>] __kmalloc_node_track_caller+0x2c4/0x524 > [<00000000746173b0>] kstrdup+0x4c/0x98 > [<00000000f79b3bf4>] kstrdup_const+0x34/0x40 > [<00000000f47eb728>] gpiod_request_commit+0xdc/0x358 > [<000000003a6899af>] gpiod_request+0xd8/0x204 > [<00000000c93118be>] gpiod_find_and_request+0x170/0x780 > [<00000000660eebdd>] gpiod_get_index+0x70/0xe0 > [<00000000eb599be3>] gpiod_get_optional+0x18/0x30 > [<00000000e8b3cfcb>] reg_fixed_voltage_probe+0x58c/0xad8 > [<000000009737579a>] platform_probe+0xc4/0x198 > [<00000000405158e3>] really_probe+0x204/0x5a8 > [<00000000772dd378>] __driver_probe_device+0x158/0x2c4 > [<00000000ddc25aba>] driver_probe_device+0x60/0x18c > [<00000000cc4413ff>] __device_attach_driver+0x168/0x208 > [<00000000e04f3755>] bus_for_each_drv+0x104/0x190 > > I'm a little confused. In the gpiod_request_commit() function, if label > is true, we allocate space kstrdup_const() for it, but desc_set_label() > will still be called. If label is true, space will be allocated to it > again, desc-> label will be pointed to the place where new is allocated, > Ah, right I see it now. I sent a fix, please leave your Tested-by if it works. Thanks for the report. Bart BTW: This is still not a complete kmemleak report, please next time paste the entire thing. > thanks > > xiaolei > > > > > Bart > > > >>> Fixes: 1f2bcb8c8ccd ("gpio: protect the descriptor label with SRCU") > >>> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> > >>> --- > >>> drivers/gpio/gpiolib.c | 6 ------ > >>> 1 file changed, 6 deletions(-) > >>> > >>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > >>> index 02be0ba1a402..32191547dece 100644 > >>> --- a/drivers/gpio/gpiolib.c > >>> +++ b/drivers/gpio/gpiolib.c > >>> @@ -2250,12 +2250,6 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label) > >>> if (test_and_set_bit(FLAG_REQUESTED, &desc->flags)) > >>> return -EBUSY; > >>> > >>> - if (label) { > >>> - label = kstrdup_const(label, GFP_KERNEL); > >>> - if (!label) > >>> - return -ENOMEM; > >>> - } > >>> - > >>> /* NOTE: gpio_request() can be called in early boot, > >>> * before IRQs are enabled, for non-sleeping (SOC) GPIOs. > >>> */ > >>> -- > >>> 2.25.1 > >>>
On Sat, Feb 17, 2024 at 2:53 PM Xiaolei Wang <xiaolei.wang@windriver.com> wrote: > > The changes in commit 1f2bcb8c8ccd ("gpio: protect the > descriptor label with SRCU"), desc_set_label has already > allocated memory space for the label, so there is no need > to allocate it again. otherwise memory leaks will be > introduced. > > unreferenced object 0xffff0000c3e4d0c0 (size 32): > comm "kworker/u16:4", pid 60, jiffies 4294894555 > hex dump (first 32 bytes): > 72 65 67 75 6c 61 74 6f 72 2d 63 61 6e 32 2d 73 regulator-can2-s > 74 62 79 00 00 00 ff ff ff ff ff ff eb db ff ff tby............. > backtrace (crc 2c3a0350): > [<00000000e93c5cf4>] kmemleak_alloc+0x34/0x40 > [<0000000097a2657f>] __kmalloc_node_track_caller+0x2c4/0x524 > [<000000000dd1c057>] kstrdup+0x4c/0x98 > [<00000000b513a96a>] kstrdup_const+0x34/0x40 > [<000000008a7f0feb>] gpiod_request_commit+0xdc/0x358 > [<00000000fc71ad64>] gpiod_request+0xd8/0x204 > [<00000000fa24b091>] gpiod_find_and_request+0x170/0x780 > [<0000000086ecf92d>] gpiod_get_index+0x70/0xe0 > [<000000004aef97f9>] gpiod_get_optional+0x18/0x30 > [<00000000312f1b25>] reg_fixed_voltage_probe+0x58c/0xad8 > [<00000000e6f47635>] platform_probe+0xc4/0x198 > [<00000000cf78fbdb>] really_probe+0x204/0x5a8 > [<00000000e28d05ec>] __driver_probe_device+0x158/0x2c4 > [<00000000e4fe452b>] driver_probe_device+0x60/0x18c > [<00000000479fcf5d>] __device_attach_driver+0x168/0x208 > [<000000007d389f38>] bus_for_each_drv+0x104/0x190 > > Fixes: 1f2bcb8c8ccd ("gpio: protect the descriptor label with SRCU") > Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> > --- > drivers/gpio/gpiolib.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 02be0ba1a402..32191547dece 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -2250,12 +2250,6 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label) > if (test_and_set_bit(FLAG_REQUESTED, &desc->flags)) > return -EBUSY; > > - if (label) { > - label = kstrdup_const(label, GFP_KERNEL); > - if (!label) > - return -ENOMEM; > - } > - > /* NOTE: gpio_request() can be called in early boot, > * before IRQs are enabled, for non-sleeping (SOC) GPIOs. > */ > -- > 2.25.1 > A queued this, sorry for the confusion, this is of course a correct fix. Bart
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 02be0ba1a402..32191547dece 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2250,12 +2250,6 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label) if (test_and_set_bit(FLAG_REQUESTED, &desc->flags)) return -EBUSY; - if (label) { - label = kstrdup_const(label, GFP_KERNEL); - if (!label) - return -ENOMEM; - } - /* NOTE: gpio_request() can be called in early boot, * before IRQs are enabled, for non-sleeping (SOC) GPIOs. */
The changes in commit 1f2bcb8c8ccd ("gpio: protect the descriptor label with SRCU"), desc_set_label has already allocated memory space for the label, so there is no need to allocate it again. otherwise memory leaks will be introduced. unreferenced object 0xffff0000c3e4d0c0 (size 32): comm "kworker/u16:4", pid 60, jiffies 4294894555 hex dump (first 32 bytes): 72 65 67 75 6c 61 74 6f 72 2d 63 61 6e 32 2d 73 regulator-can2-s 74 62 79 00 00 00 ff ff ff ff ff ff eb db ff ff tby............. backtrace (crc 2c3a0350): [<00000000e93c5cf4>] kmemleak_alloc+0x34/0x40 [<0000000097a2657f>] __kmalloc_node_track_caller+0x2c4/0x524 [<000000000dd1c057>] kstrdup+0x4c/0x98 [<00000000b513a96a>] kstrdup_const+0x34/0x40 [<000000008a7f0feb>] gpiod_request_commit+0xdc/0x358 [<00000000fc71ad64>] gpiod_request+0xd8/0x204 [<00000000fa24b091>] gpiod_find_and_request+0x170/0x780 [<0000000086ecf92d>] gpiod_get_index+0x70/0xe0 [<000000004aef97f9>] gpiod_get_optional+0x18/0x30 [<00000000312f1b25>] reg_fixed_voltage_probe+0x58c/0xad8 [<00000000e6f47635>] platform_probe+0xc4/0x198 [<00000000cf78fbdb>] really_probe+0x204/0x5a8 [<00000000e28d05ec>] __driver_probe_device+0x158/0x2c4 [<00000000e4fe452b>] driver_probe_device+0x60/0x18c [<00000000479fcf5d>] __device_attach_driver+0x168/0x208 [<000000007d389f38>] bus_for_each_drv+0x104/0x190 Fixes: 1f2bcb8c8ccd ("gpio: protect the descriptor label with SRCU") Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> --- drivers/gpio/gpiolib.c | 6 ------ 1 file changed, 6 deletions(-)