| Message ID | 20241107071538.195340-3-dmitry.torokhov@gmail.com |
|---|---|
| State | Accepted |
| Commit | 5203b3a18c1bbf50ec5fff27489da8e9bce48ddb |
| Headers | show |
| Series | Convert input core to use new cleanup facilities | expand |
On 07.11.2024 08:15, Dmitry Torokhov wrote: > Annotate allocated memory with __free(kfree) to simplify the code and > make sure memory is released appropriately. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/input/ff-core.c | 19 ++++++------------- > 1 file changed, 6 insertions(+), 13 deletions(-) This patch landed in linux-next as commit 5203b3a18c1b ("Input: ff-core - make use of __free() cleanup facility"). In my tests I found that it causes the following kernel panic on some of my test boards. Reverting it, together with fd5ba0501d38 ("Input: ff-memless - make use of __free() cleanup facility") on top of next-20241220 fixes this panic issue. Here is the relevant log captured on Samsung Exynos4412 ARM 32bit-based Trats2 board: 8<--- cut here --- Unable to handle kernel NULL pointer dereference at virtual address 00000024 when read [00000024] *pgd=00000000 Internal error: Oops: 5 [#1] PREEMPT SMP ARM Modules linked in: CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.13.0-rc3-next-20241220 #15500 Hardware name: Samsung Exynos (Flattened Device Tree) PC is at input_ff_create+0xa0/0x13c LR is at input_ff_create+0xb8/0x13c pc : [<c08d7e14>] lr : [<c08d7e2c>] psr: 80000013 ... Process swapper/0 (pid: 1, stack limit = 0x(ptrval)) ... Call trace: input_ff_create from input_ff_create_memless+0x8c/0x160 input_ff_create_memless from max77693_haptic_probe+0x1b0/0x284 max77693_haptic_probe from platform_probe+0x80/0xc0 platform_probe from really_probe+0x154/0x3ac really_probe from __driver_probe_device+0xa0/0x1d4 __driver_probe_device from driver_probe_device+0x30/0xd0 driver_probe_device from __driver_attach+0x10c/0x190 __driver_attach from bus_for_each_dev+0x60/0xb4 bus_for_each_dev from bus_add_driver+0xe0/0x220 bus_add_driver from driver_register+0x7c/0x118 driver_register from do_one_initcall+0x6c/0x328 do_one_initcall from kernel_init_freeable+0x1c8/0x218 kernel_init_freeable from kernel_init+0x1c/0x12c kernel_init from ret_from_fork+0x14/0x28 Exception stack(0xf0845fb0 to 0xf0845ff8) ... ---[ end trace 0000000000000000 ]--- Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]--- > diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c > index eb01bcb69d00..a235d2eb6b31 100644 > --- a/drivers/input/ff-core.c > +++ b/drivers/input/ff-core.c > @@ -290,8 +290,6 @@ EXPORT_SYMBOL_GPL(input_ff_event); > */ > int input_ff_create(struct input_dev *dev, unsigned int max_effects) > { > - struct ff_device *ff; > - size_t ff_dev_size; > int i; > > if (!max_effects) { > @@ -304,25 +302,20 @@ int input_ff_create(struct input_dev *dev, unsigned int max_effects) > return -EINVAL; > } > > - ff_dev_size = struct_size(ff, effect_owners, max_effects); > - if (ff_dev_size == SIZE_MAX) /* overflow */ > - return -EINVAL; > - > - ff = kzalloc(ff_dev_size, GFP_KERNEL); > + struct ff_device *ff __free(kfree) = > + kzalloc(struct_size(ff, effect_owners, max_effects), > + GFP_KERNEL); > if (!ff) > return -ENOMEM; > > - ff->effects = kcalloc(max_effects, sizeof(struct ff_effect), > - GFP_KERNEL); > - if (!ff->effects) { > - kfree(ff); > + ff->effects = kcalloc(max_effects, sizeof(*ff->effects), GFP_KERNEL); > + if (!ff->effects) > return -ENOMEM; > - } > > ff->max_effects = max_effects; > mutex_init(&ff->mutex); > > - dev->ff = ff; > + dev->ff = no_free_ptr(ff); > dev->flush = input_ff_flush; > dev->event = input_ff_event; > __set_bit(EV_FF, dev->evbit); Best regards
On Fri, Dec 20, 2024 at 01:36:59PM +0100, Marek Szyprowski wrote: > On 07.11.2024 08:15, Dmitry Torokhov wrote: > > Annotate allocated memory with __free(kfree) to simplify the code and > > make sure memory is released appropriately. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/input/ff-core.c | 19 ++++++------------- > > 1 file changed, 6 insertions(+), 13 deletions(-) > > This patch landed in linux-next as commit 5203b3a18c1b ("Input: ff-core > - make use of __free() cleanup facility"). In my tests I found that it > causes the following kernel panic on some of my test boards. Reverting > it, together with fd5ba0501d38 ("Input: ff-memless - make use of > __free() cleanup facility") on top of next-20241220 fixes this panic > issue. Gah, I fixed this once and then undid it for some reason. I think the following should fix the problem: diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c index a235d2eb6b31..c25e05eeb8e5 100644 --- a/drivers/input/ff-core.c +++ b/drivers/input/ff-core.c @@ -315,7 +315,6 @@ int input_ff_create(struct input_dev *dev, unsigned int max_effects) ff->max_effects = max_effects; mutex_init(&ff->mutex); - dev->ff = no_free_ptr(ff); dev->flush = input_ff_flush; dev->event = input_ff_event; __set_bit(EV_FF, dev->evbit); @@ -328,6 +327,7 @@ int input_ff_create(struct input_dev *dev, unsigned int max_effects) if (test_bit(FF_PERIODIC, ff->ffbit)) __set_bit(FF_RUMBLE, dev->ffbit); + dev->ff = no_free_ptr(ff); return 0; } EXPORT_SYMBOL_GPL(input_ff_create); diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c index 0bbeceb35545..e9120ba6bae0 100644 --- a/drivers/input/ff-memless.c +++ b/drivers/input/ff-memless.c @@ -524,7 +524,6 @@ int input_ff_create_memless(struct input_dev *dev, void *data, return error; ff = dev->ff; - ff->private = no_free_ptr(ml); ff->upload = ml_ff_upload; ff->playback = ml_ff_playback; ff->set_gain = ml_ff_set_gain; @@ -541,6 +540,8 @@ int input_ff_create_memless(struct input_dev *dev, void *data, for (i = 0; i < FF_MEMLESS_EFFECTS; i++) ml->states[i].effect = &ff->effects[i]; + ff->private = no_free_ptr(ml); + return 0; } EXPORT_SYMBOL_GPL(input_ff_create_memless); Thanks.
On 20.12.2024 18:22, Dmitry Torokhov wrote: > On Fri, Dec 20, 2024 at 01:36:59PM +0100, Marek Szyprowski wrote: >> On 07.11.2024 08:15, Dmitry Torokhov wrote: >>> Annotate allocated memory with __free(kfree) to simplify the code and >>> make sure memory is released appropriately. >>> >>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> >>> --- >>> drivers/input/ff-core.c | 19 ++++++------------- >>> 1 file changed, 6 insertions(+), 13 deletions(-) >> This patch landed in linux-next as commit 5203b3a18c1b ("Input: ff-core >> - make use of __free() cleanup facility"). In my tests I found that it >> causes the following kernel panic on some of my test boards. Reverting >> it, together with fd5ba0501d38 ("Input: ff-memless - make use of >> __free() cleanup facility") on top of next-20241220 fixes this panic >> issue. > Gah, I fixed this once and then undid it for some reason. I think the > following should fix the problem: Yep, this fixes the problem. Feel free to add: Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c > index a235d2eb6b31..c25e05eeb8e5 100644 > --- a/drivers/input/ff-core.c > +++ b/drivers/input/ff-core.c > @@ -315,7 +315,6 @@ int input_ff_create(struct input_dev *dev, unsigned int max_effects) > ff->max_effects = max_effects; > mutex_init(&ff->mutex); > > - dev->ff = no_free_ptr(ff); > dev->flush = input_ff_flush; > dev->event = input_ff_event; > __set_bit(EV_FF, dev->evbit); > @@ -328,6 +327,7 @@ int input_ff_create(struct input_dev *dev, unsigned int max_effects) > if (test_bit(FF_PERIODIC, ff->ffbit)) > __set_bit(FF_RUMBLE, dev->ffbit); > > + dev->ff = no_free_ptr(ff); > return 0; > } > EXPORT_SYMBOL_GPL(input_ff_create); > diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c > index 0bbeceb35545..e9120ba6bae0 100644 > --- a/drivers/input/ff-memless.c > +++ b/drivers/input/ff-memless.c > @@ -524,7 +524,6 @@ int input_ff_create_memless(struct input_dev *dev, void *data, > return error; > > ff = dev->ff; > - ff->private = no_free_ptr(ml); > ff->upload = ml_ff_upload; > ff->playback = ml_ff_playback; > ff->set_gain = ml_ff_set_gain; > @@ -541,6 +540,8 @@ int input_ff_create_memless(struct input_dev *dev, void *data, > for (i = 0; i < FF_MEMLESS_EFFECTS; i++) > ml->states[i].effect = &ff->effects[i]; > > + ff->private = no_free_ptr(ml); > + > return 0; > } > EXPORT_SYMBOL_GPL(input_ff_create_memless); > > > Thanks. > Best regards
On Fri, Dec 20, 2024 at 06:38:04PM +0100, Marek Szyprowski wrote: > On 20.12.2024 18:22, Dmitry Torokhov wrote: > > On Fri, Dec 20, 2024 at 01:36:59PM +0100, Marek Szyprowski wrote: > >> On 07.11.2024 08:15, Dmitry Torokhov wrote: > >>> Annotate allocated memory with __free(kfree) to simplify the code and > >>> make sure memory is released appropriately. > >>> > >>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > >>> --- > >>> drivers/input/ff-core.c | 19 ++++++------------- > >>> 1 file changed, 6 insertions(+), 13 deletions(-) > >> This patch landed in linux-next as commit 5203b3a18c1b ("Input: ff-core > >> - make use of __free() cleanup facility"). In my tests I found that it > >> causes the following kernel panic on some of my test boards. Reverting > >> it, together with fd5ba0501d38 ("Input: ff-memless - make use of > >> __free() cleanup facility") on top of next-20241220 fixes this panic > >> issue. > > Gah, I fixed this once and then undid it for some reason. I think the > > following should fix the problem: > > Yep, this fixes the problem. Feel free to add: > > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> Thank you for the report and testing. Do you mind if I fold these fixes into original patches? Thanks.
On 20.12.2024 18:39, Dmitry Torokhov wrote: > On Fri, Dec 20, 2024 at 06:38:04PM +0100, Marek Szyprowski wrote: >> On 20.12.2024 18:22, Dmitry Torokhov wrote: >>> On Fri, Dec 20, 2024 at 01:36:59PM +0100, Marek Szyprowski wrote: >>>> On 07.11.2024 08:15, Dmitry Torokhov wrote: >>>>> Annotate allocated memory with __free(kfree) to simplify the code and >>>>> make sure memory is released appropriately. >>>>> >>>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> >>>>> --- >>>>> drivers/input/ff-core.c | 19 ++++++------------- >>>>> 1 file changed, 6 insertions(+), 13 deletions(-) >>>> This patch landed in linux-next as commit 5203b3a18c1b ("Input: ff-core >>>> - make use of __free() cleanup facility"). In my tests I found that it >>>> causes the following kernel panic on some of my test boards. Reverting >>>> it, together with fd5ba0501d38 ("Input: ff-memless - make use of >>>> __free() cleanup facility") on top of next-20241220 fixes this panic >>>> issue. >>> Gah, I fixed this once and then undid it for some reason. I think the >>> following should fix the problem: >> Yep, this fixes the problem. Feel free to add: >> >> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> >> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > Thank you for the report and testing. Do you mind if I fold these fixes > into original patches? Go ahead, no problem for me. Best regards
diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c index eb01bcb69d00..a235d2eb6b31 100644 --- a/drivers/input/ff-core.c +++ b/drivers/input/ff-core.c @@ -290,8 +290,6 @@ EXPORT_SYMBOL_GPL(input_ff_event); */ int input_ff_create(struct input_dev *dev, unsigned int max_effects) { - struct ff_device *ff; - size_t ff_dev_size; int i; if (!max_effects) { @@ -304,25 +302,20 @@ int input_ff_create(struct input_dev *dev, unsigned int max_effects) return -EINVAL; } - ff_dev_size = struct_size(ff, effect_owners, max_effects); - if (ff_dev_size == SIZE_MAX) /* overflow */ - return -EINVAL; - - ff = kzalloc(ff_dev_size, GFP_KERNEL); + struct ff_device *ff __free(kfree) = + kzalloc(struct_size(ff, effect_owners, max_effects), + GFP_KERNEL); if (!ff) return -ENOMEM; - ff->effects = kcalloc(max_effects, sizeof(struct ff_effect), - GFP_KERNEL); - if (!ff->effects) { - kfree(ff); + ff->effects = kcalloc(max_effects, sizeof(*ff->effects), GFP_KERNEL); + if (!ff->effects) return -ENOMEM; - } ff->max_effects = max_effects; mutex_init(&ff->mutex); - dev->ff = ff; + dev->ff = no_free_ptr(ff); dev->flush = input_ff_flush; dev->event = input_ff_event; __set_bit(EV_FF, dev->evbit);
Annotate allocated memory with __free(kfree) to simplify the code and make sure memory is released appropriately. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/ff-core.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)