* Re: [syzbot] [input?] KASAN: slab-use-after-free Read in input_dev_uevent
From: Maxime Ripard @ 2023-08-22 9:12 UTC (permalink / raw)
To: syzbot
Cc: davidgow, dmitry.torokhov, gregkh, linux-input, linux-kernel,
rydberg, syzkaller-bugs, benjamin.tissoires
In-Reply-To: <00000000000035beba060371a468@google.com>
[-- Attachment #1: Type: text/plain, Size: 13246 bytes --]
Hi,
On Mon, Aug 21, 2023 at 09:48:01AM -0700, syzbot wrote:
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: 7271b2a53042 Add linux-next specific files for 20230818
> git tree: linux-next
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=11edc0d3a80000
> kernel config: https://syzkaller.appspot.com/x/.config?x=1936af09cdef7dd6
> dashboard link: https://syzkaller.appspot.com/bug?extid=3a0ebe8a52b89c63739d
> compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=17998f03a80000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17b81223a80000
>
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/d81109bc02c1/disk-7271b2a5.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/4b3bf8e2a4f7/vmlinux-7271b2a5.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/6404cd473c1e/bzImage-7271b2a5.xz
>
> The issue was bisected to:
>
> commit 699fb50d99039a50e7494de644f96c889279aca3
> Author: David Gow <davidgow@google.com>
> Date: Thu Jul 20 12:45:09 2023 +0000
>
> drivers: base: Free devm resources when unregistering a device
>
> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=13140083a80000
> final oops: https://syzkaller.appspot.com/x/report.txt?x=10940083a80000
> console output: https://syzkaller.appspot.com/x/log.txt?x=17140083a80000
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+3a0ebe8a52b89c63739d@syzkaller.appspotmail.com
> Fixes: 699fb50d9903 ("drivers: base: Free devm resources when unregistering a device")
>
> usb 1-1: USB disconnect, device number 2
> ==================================================================
> BUG: KASAN: slab-use-after-free in string_nocheck lib/vsprintf.c:645 [inline]
> BUG: KASAN: slab-use-after-free in string+0x394/0x3d0 lib/vsprintf.c:727
> Read of size 1 at addr ffff88801c8c6ca8 by task kworker/1:3/4508
>
> CPU: 1 PID: 4508 Comm: kworker/1:3 Not tainted 6.5.0-rc6-next-20230818-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/26/2023
> Workqueue: usb_hub_wq hub_event
> Call Trace:
> <TASK>
> __dump_stack lib/dump_stack.c:88 [inline]
> dump_stack_lvl+0xd9/0x1b0 lib/dump_stack.c:106
> print_address_description mm/kasan/report.c:364 [inline]
> print_report+0xc4/0x620 mm/kasan/report.c:475
> kasan_report+0xda/0x110 mm/kasan/report.c:588
> string_nocheck lib/vsprintf.c:645 [inline]
> string+0x394/0x3d0 lib/vsprintf.c:727
> vsnprintf+0xc5f/0x1870 lib/vsprintf.c:2818
> add_uevent_var+0x17c/0x390 lib/kobject_uevent.c:665
> input_dev_uevent+0x162/0x8f0 drivers/input/input.c:1691
> dev_uevent+0x305/0x760 drivers/base/core.c:2599
> kobject_uevent_env+0x623/0x1800 lib/kobject_uevent.c:557
> device_del+0x642/0xa50 drivers/base/core.c:3830
> input_unregister_device+0xb9/0x100 drivers/input/input.c:2440
> hidinput_disconnect+0x160/0x3e0 drivers/hid/hid-input.c:2386
> hid_disconnect+0x143/0x1b0 drivers/hid/hid-core.c:2273
> hid_hw_stop+0x16/0x70 drivers/hid/hid-core.c:2322
> uclogic_remove+0x47/0x90 drivers/hid/hid-uclogic-core.c:485
> hid_device_remove+0xce/0x250 drivers/hid/hid-core.c:2682
> device_remove+0xc8/0x170 drivers/base/dd.c:567
> __device_release_driver drivers/base/dd.c:1272 [inline]
> device_release_driver_internal+0x44a/0x610 drivers/base/dd.c:1295
> bus_remove_device+0x22c/0x420 drivers/base/bus.c:574
> device_del+0x39a/0xa50 drivers/base/core.c:3812
> hid_remove_device drivers/hid/hid-core.c:2859 [inline]
> hid_destroy_device+0xe5/0x150 drivers/hid/hid-core.c:2879
> usbhid_disconnect+0xa0/0xe0 drivers/hid/usbhid/hid-core.c:1456
> usb_unbind_interface+0x1dd/0x8d0 drivers/usb/core/driver.c:458
> device_remove drivers/base/dd.c:569 [inline]
> device_remove+0x11f/0x170 drivers/base/dd.c:561
> __device_release_driver drivers/base/dd.c:1272 [inline]
> device_release_driver_internal+0x44a/0x610 drivers/base/dd.c:1295
> bus_remove_device+0x22c/0x420 drivers/base/bus.c:574
> device_del+0x39a/0xa50 drivers/base/core.c:3812
> usb_disable_device+0x36c/0x7f0 drivers/usb/core/message.c:1416
> usb_disconnect+0x2e1/0x890 drivers/usb/core/hub.c:2252
> hub_port_connect drivers/usb/core/hub.c:5280 [inline]
> hub_port_connect_change drivers/usb/core/hub.c:5580 [inline]
> port_event drivers/usb/core/hub.c:5740 [inline]
> hub_event+0x1db7/0x4e00 drivers/usb/core/hub.c:5822
> process_one_work+0x887/0x15d0 kernel/workqueue.c:2630
> process_scheduled_works kernel/workqueue.c:2703 [inline]
> worker_thread+0x8bb/0x1290 kernel/workqueue.c:2784
> kthread+0x33a/0x430 kernel/kthread.c:388
> ret_from_fork+0x45/0x80 arch/x86/kernel/process.c:147
> ret_from_fork_asm+0x11/0x20 arch/x86/entry/entry_64.S:304
> </TASK>
>
> Allocated by task 782:
> kasan_save_stack+0x33/0x50 mm/kasan/common.c:45
> kasan_set_track+0x25/0x30 mm/kasan/common.c:52
> ____kasan_kmalloc mm/kasan/common.c:374 [inline]
> __kasan_kmalloc+0xa2/0xb0 mm/kasan/common.c:383
> kasan_kmalloc include/linux/kasan.h:198 [inline]
> __do_kmalloc_node mm/slab_common.c:1004 [inline]
> __kmalloc_node_track_caller+0x61/0x100 mm/slab_common.c:1024
> alloc_dr drivers/base/devres.c:119 [inline]
> devm_kmalloc+0xa5/0x230 drivers/base/devres.c:829
> devm_kzalloc include/linux/device.h:314 [inline]
> uclogic_input_configured+0x251/0x610 drivers/hid/hid-uclogic-core.c:151
> hidinput_connect+0x1bf4/0x2b60 drivers/hid/hid-input.c:2344
> hid_connect+0x139e/0x18a0 drivers/hid/hid-core.c:2187
> hid_hw_start drivers/hid/hid-core.c:2302 [inline]
> hid_hw_start+0xa0/0x130 drivers/hid/hid-core.c:2293
> uclogic_probe+0x235/0x380 drivers/hid/hid-uclogic-core.c:221
> __hid_device_probe drivers/hid/hid-core.c:2626 [inline]
> hid_device_probe+0x2e4/0x480 drivers/hid/hid-core.c:2663
> call_driver_probe drivers/base/dd.c:579 [inline]
> really_probe+0x234/0xc90 drivers/base/dd.c:658
> __driver_probe_device+0x1de/0x4b0 drivers/base/dd.c:800
> driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:830
> __device_attach_driver+0x1d4/0x300 drivers/base/dd.c:958
> bus_for_each_drv+0x157/0x1d0 drivers/base/bus.c:457
> __device_attach+0x1e8/0x4b0 drivers/base/dd.c:1030
> bus_probe_device+0x17c/0x1c0 drivers/base/bus.c:532
> device_add+0x11f1/0x1b40 drivers/base/core.c:3623
> hid_add_device+0x371/0xa60 drivers/hid/hid-core.c:2809
> usbhid_probe+0xd0a/0x1360 drivers/hid/usbhid/hid-core.c:1429
> usb_probe_interface+0x307/0x930 drivers/usb/core/driver.c:396
> call_driver_probe drivers/base/dd.c:579 [inline]
> really_probe+0x234/0xc90 drivers/base/dd.c:658
> __driver_probe_device+0x1de/0x4b0 drivers/base/dd.c:800
> driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:830
> __device_attach_driver+0x1d4/0x300 drivers/base/dd.c:958
> bus_for_each_drv+0x157/0x1d0 drivers/base/bus.c:457
> __device_attach+0x1e8/0x4b0 drivers/base/dd.c:1030
> bus_probe_device+0x17c/0x1c0 drivers/base/bus.c:532
> device_add+0x11f1/0x1b40 drivers/base/core.c:3623
> usb_set_configuration+0x10cb/0x1c40 drivers/usb/core/message.c:2207
> usb_generic_driver_probe+0xca/0x130 drivers/usb/core/generic.c:238
> usb_probe_device+0xda/0x2c0 drivers/usb/core/driver.c:293
> call_driver_probe drivers/base/dd.c:579 [inline]
> really_probe+0x234/0xc90 drivers/base/dd.c:658
> __driver_probe_device+0x1de/0x4b0 drivers/base/dd.c:800
> driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:830
> __device_attach_driver+0x1d4/0x300 drivers/base/dd.c:958
> bus_for_each_drv+0x157/0x1d0 drivers/base/bus.c:457
> __device_attach+0x1e8/0x4b0 drivers/base/dd.c:1030
> bus_probe_device+0x17c/0x1c0 drivers/base/bus.c:532
> device_add+0x11f1/0x1b40 drivers/base/core.c:3623
> usb_new_device+0xd80/0x1960 drivers/usb/core/hub.c:2589
> hub_port_connect drivers/usb/core/hub.c:5440 [inline]
> hub_port_connect_change drivers/usb/core/hub.c:5580 [inline]
> port_event drivers/usb/core/hub.c:5740 [inline]
> hub_event+0x2daf/0x4e00 drivers/usb/core/hub.c:5822
> process_one_work+0x887/0x15d0 kernel/workqueue.c:2630
> process_scheduled_works kernel/workqueue.c:2703 [inline]
> worker_thread+0x8bb/0x1290 kernel/workqueue.c:2784
> kthread+0x33a/0x430 kernel/kthread.c:388
> ret_from_fork+0x45/0x80 arch/x86/kernel/process.c:147
> ret_from_fork_asm+0x11/0x20 arch/x86/entry/entry_64.S:304
>
> Freed by task 4508:
> kasan_save_stack+0x33/0x50 mm/kasan/common.c:45
> kasan_set_track+0x25/0x30 mm/kasan/common.c:52
> kasan_save_free_info+0x2b/0x40 mm/kasan/generic.c:522
> ____kasan_slab_free mm/kasan/common.c:236 [inline]
> ____kasan_slab_free+0x15b/0x1b0 mm/kasan/common.c:200
> kasan_slab_free include/linux/kasan.h:164 [inline]
> slab_free_hook mm/slub.c:1800 [inline]
> slab_free_freelist_hook+0x114/0x1e0 mm/slub.c:1826
> slab_free mm/slub.c:3809 [inline]
> __kmem_cache_free+0xb8/0x2f0 mm/slub.c:3822
> release_nodes drivers/base/devres.c:506 [inline]
> devres_release_all+0x192/0x240 drivers/base/devres.c:535
> device_del+0x628/0xa50 drivers/base/core.c:3827
> input_unregister_device+0xb9/0x100 drivers/input/input.c:2440
> hidinput_disconnect+0x160/0x3e0 drivers/hid/hid-input.c:2386
> hid_disconnect+0x143/0x1b0 drivers/hid/hid-core.c:2273
> hid_hw_stop+0x16/0x70 drivers/hid/hid-core.c:2322
> uclogic_remove+0x47/0x90 drivers/hid/hid-uclogic-core.c:485
> hid_device_remove+0xce/0x250 drivers/hid/hid-core.c:2682
> device_remove+0xc8/0x170 drivers/base/dd.c:567
> __device_release_driver drivers/base/dd.c:1272 [inline]
> device_release_driver_internal+0x44a/0x610 drivers/base/dd.c:1295
> bus_remove_device+0x22c/0x420 drivers/base/bus.c:574
> device_del+0x39a/0xa50 drivers/base/core.c:3812
> hid_remove_device drivers/hid/hid-core.c:2859 [inline]
> hid_destroy_device+0xe5/0x150 drivers/hid/hid-core.c:2879
> usbhid_disconnect+0xa0/0xe0 drivers/hid/usbhid/hid-core.c:1456
> usb_unbind_interface+0x1dd/0x8d0 drivers/usb/core/driver.c:458
> device_remove drivers/base/dd.c:569 [inline]
> device_remove+0x11f/0x170 drivers/base/dd.c:561
> __device_release_driver drivers/base/dd.c:1272 [inline]
> device_release_driver_internal+0x44a/0x610 drivers/base/dd.c:1295
> bus_remove_device+0x22c/0x420 drivers/base/bus.c:574
> device_del+0x39a/0xa50 drivers/base/core.c:3812
> usb_disable_device+0x36c/0x7f0 drivers/usb/core/message.c:1416
> usb_disconnect+0x2e1/0x890 drivers/usb/core/hub.c:2252
> hub_port_connect drivers/usb/core/hub.c:5280 [inline]
> hub_port_connect_change drivers/usb/core/hub.c:5580 [inline]
> port_event drivers/usb/core/hub.c:5740 [inline]
> hub_event+0x1db7/0x4e00 drivers/usb/core/hub.c:5822
> process_one_work+0x887/0x15d0 kernel/workqueue.c:2630
> process_scheduled_works kernel/workqueue.c:2703 [inline]
> worker_thread+0x8bb/0x1290 kernel/workqueue.c:2784
> kthread+0x33a/0x430 kernel/kthread.c:388
> ret_from_fork+0x45/0x80 arch/x86/kernel/process.c:147
> ret_from_fork_asm+0x11/0x20 arch/x86/entry/entry_64.S:304
So, we discussed it this morning with Benjamin, and I think the culprit
is that the uclogic driver will allocate a char array with devm_kzalloc
in uclogic_input_configured()
(https://elixir.bootlin.com/linux/latest/source/drivers/hid/hid-uclogic-core.c#L149),
and will assign input_dev->name to that pointer.
When the device is removed, the devm-allocated array is freed, and the
input framework will send a uevent in input_dev_uevent() using the
input_dev->name field:
https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1688
So it's a classic dangling pointer situation.
And even though it was revealed by that patch, I think the issue is
unrelated. The fundamental issue seems to be that the usage of devm in
that situation is wrong.
input_dev->name is accessed by input_dev_uevent, which for KOBJ_UNBIND
and KOBJ_REMOVE will be called after remove.
For example, in __device_release_driver() (with the driver remove hook
being called in device_remove() and devres_release_all() being called in
device_unbind_cleanup()):
https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L1278
So, it looks to me that, with or without the patch we merged recently,
the core has always sent uevent after device-managed resources were
freed. Thus, the uclogic (and any other input driver) was wrong in
allocating its input_dev name with devm_kzalloc (or the phys and uniq
fields in that struct).
Note that freeing input_dev->name in remove would have been just as bad.
Looking at the code quickly, at least hid-playstation,
hid-nvidia-shield, hid-logitech-hidpp, mms114 and tsc200x seem to be
affected by the same issue.
We discussed a couple of solutions with Benjamin, such as creating a
helper devm action to free and clear the input_dev->name field, droping
the name, phys and uniq fields from the uevent, or converting name, phys
and uniq to char arrays so drivers don't have to allocate them.
We couldn't find a perfect one though, so... yeah.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v3 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50
From: Jeff LaBundy @ 2023-08-22 12:43 UTC (permalink / raw)
To: James Ogletree
Cc: Dmitry Torokhov, Fred Treven, Ben Bright, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Lee Jones, Joel Stanley,
Arnd Bergmann, Jacky Bai, Jean Delvare, Eddie James,
Markus Schneider-Pargmann, ChiYuan Huang, Randy Dunlap,
Wolfram Sang, patches@opensource.cirrus.com,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <7C3D1F80-556D-463E-85AD-AFA48CADAF5E@cirrus.com>
Hi James,
On Wed, Aug 16, 2023 at 09:02:26PM +0000, James Ogletree wrote:
>
>
> > On Aug 10, 2023, at 11:07 PM, Jeff LaBundy <jeff@labundy.com> wrote:
> >
> > On Wed, Aug 09, 2023 at 07:10:28PM +0000, James Ogletree wrote:
> >> Introduce support for Cirrus Logic Device CS40L50: a
> >> haptics driver with waveform memory DSP and closed-loop
> >> algorithms.
> >
> > From my extremely naive point of view, some of the code that follows
> > bares resemblance to the recently reviewed L26. My assumption is that
> > these devices are different enough in nature to warrant completely
> > different drivers; is that accurate?
> >
> > One reason for asking is because the L26 driver included a cornucopia
> > of power-management overhead, yet we see none of that here. Assuming
> > both L50 and L26 are built around the same Halo DSP, why is there such
> > a fundamental difference between the two in terms of power management?
> >
> > To that end, how does this driver handle hibernation? Is hibernation
> > not supported, or do we simply defer to the DSP? In the case of the
> > latter, why is L50 given this freedom but not L26?
>
> One key difference is that L50’s Halo Core DSP is self-booting; the firmware
> is burned in and no firmware download is required. On L26, firmware
> downloading is compulsory. This differentiates dealing with the DSP in the
> two drivers, because the L50 driver does not need to do a look up every
> time it reads or writes to a firmware control. The registers are all static.
Interesting stuff; thanks for sharing that background information.
>
> Minor reasons are that they have different power supply configurations that
> require different register settings, they have errata differences, and a different set
> of exposed features (L50 being much more simplistic). I think taken cumulatively
> these differences warrant separate drivers. Though, I will take Charles’
> recommendation to factor out the similarities into a shared library that both L50
> and L26 can use.
>
> Let me know whether you disagree on the above points or have followup
> questions.
Makes sense to me.
>
> With respect to power management, I did not think that that there was any merit
> in itself in maintaining equality with L26’s approach, and I was inclined to accept
> your reasoning for using retry logic over the runtime PM facilities (not that the
> latter way is incorrect).
>
> Regarding the need for I2S streaming support, signs point to maybe. I will
> migrate this driver to MFD for V4.
MFD seems like the right (i.e. scalable) solution if A2H is on the roadmap.
In early L25 days, very early adopters simply asked for a sysfs control
exposed from the core (then LED !!) driver to enable/disable I2S streaming
mode, but this got hairy in case customers needed to control BCLK/Fs ratio,
bit depth, etc. during runtime.
From my naive point of view, maybe the solution looks something like this:
* drivers/mfd/cs40l50-i2c.c: I2C client
* drivers/mfd/cs40l50-spi.c: SPI client
* drivers/mfd/cs40l50-core.c: common tasks such as FW loading, OTP management, etc;
perhaps L26 can leverage it as well
* drivers/input/misc/cs40l50-vibra.c: FF-specific support (name as you like, I just
picked what seems to be most common)
* sound/soc/codecs/cs40l50-codec.c: codec-specific support
In case I have misunderstood, please let me know.
>
> >
> >> + return cs40l50_dsp_write(cs40l50, CS40L50_BST_LPMODE_SEL, CS40L50_DCM_LOW_POWER);
> >> +}
> >> +
> >> +static int cs40l50_patch_firmware(struct cs40l50_private *cs40l50)
> >> +{
> >> + const struct firmware *bin = NULL, *wmfw = NULL;
> >> + int error = 0;
> >> +
> >> + if (request_firmware(&bin, "cs40l50.bin", cs40l50->dev))
> >> + dev_info(cs40l50->dev, "Could not request wavetable file: %d\n", error);
> >> +
> >> + if (request_firmware(&wmfw, "cs40l50.wmfw", cs40l50->dev))
> >> + dev_info(cs40l50->dev, "Could not request firmware patch file: %d\n", error);
> >> +
> >> + if (wmfw) {
> >
> > It is a much more common design pattern to bail if request_firmware() returns
> > an error, than to proceed and check against the FW pointer remaining NULL.
> >
> > Is this done because cs_dsp_power_up() must be called, with or without a wmfw
> > or coefficient file being available?
>
> I don’t think that cs_dsp_power_up() must be called in the case of non-existent .wmfw
> and .bin files, so I will take your suggestion and optimize this function. I will also switch
> to asynchronous firmware requesting for V4.
Since L50 can work out of the box without FW, another option is to follow the
approach used by some codec drivers and wait to load FW until the first request
to stream I2S, and/or trigger an FF effect in this case. By that point, rootfs
has been available for some time and request_firmware() will succeed.
This is advantageous because even though request_firmware_nowait() can solve
the deadlock problem for FW loaded at probe, it could still return -ENOENT right
away on some platforms. The disadvantage is that the first effect would be very
delayed due to the overhead of transferring several kB of FW over I2C for the
first time. That's OK for audio applications, but not haptics where delay normally
must be within single-digit ms.
Ultimately, I think the right approach is to look for FW from probe as you have
done, but use request_firmware_nowait(). However, I recommend structuring the
driver such that rip-up would be minimal in case you had to retry FW loading from
the first FF trigger, to support a customer whose rootfs is not available at probe.
In such a case, the customer could work around the first-time latency problem by
triggering a dummy effect early in their boot (e.g. init.rc).
>
> I will also incorporate the rest of your review comments not mentioned here in V4.
> Thank you for the thorough review.
Thank you for the productive discussion!
>
> Regards,
> James Ogletree
Kind regards,
Jeff LaBundy
^ permalink raw reply
* [PATCH] HID: cp2112: make the irqchip immutable
From: Bartosz Golaszewski @ 2023-08-22 15:22 UTC (permalink / raw)
To: Linus Walleij, Andy Shevchenko, Jiri Kosina, Benjamin Tissoires
Cc: linux-gpio, linux-kernel, linux-input, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
This make the GPIO irqchip exposed by the CP2112 driver use an immutable
irq_chip struct thus addressing the following warning on probe:
(cp2112_gpio): not an immutable chip, please consider fixing it!
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
drivers/hid/hid-cp2112.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 27cadadda7c9..01f2a7211033 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -163,7 +163,6 @@ struct cp2112_device {
atomic_t read_avail;
atomic_t xfer_avail;
struct gpio_chip gc;
- struct irq_chip irq;
u8 *in_out_buffer;
struct mutex lock;
@@ -1082,6 +1081,7 @@ static void cp2112_gpio_irq_mask(struct irq_data *d)
struct cp2112_device *dev = gpiochip_get_data(gc);
__clear_bit(d->hwirq, &dev->irq_mask);
+ gpiochip_disable_irq(gc, irqd_to_hwirq(d));
}
static void cp2112_gpio_irq_unmask(struct irq_data *d)
@@ -1089,6 +1089,7 @@ static void cp2112_gpio_irq_unmask(struct irq_data *d)
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
struct cp2112_device *dev = gpiochip_get_data(gc);
+ gpiochip_enable_irq(gc, irqd_to_hwirq(d));
__set_bit(d->hwirq, &dev->irq_mask);
}
@@ -1228,6 +1229,18 @@ static int __maybe_unused cp2112_allocate_irq(struct cp2112_device *dev,
return ret;
}
+static const struct irq_chip cp2112_irq_chip = {
+ .name = "cp2112-gpio",
+ .irq_startup = cp2112_gpio_irq_startup,
+ .irq_shutdown = cp2112_gpio_irq_shutdown,
+ .irq_ack = cp2112_gpio_irq_ack,
+ .irq_mask = cp2112_gpio_irq_mask,
+ .irq_unmask = cp2112_gpio_irq_unmask,
+ .irq_set_type = cp2112_gpio_irq_type,
+ .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE,
+ GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
{
struct cp2112_device *dev;
@@ -1337,17 +1350,8 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
dev->gc.can_sleep = 1;
dev->gc.parent = &hdev->dev;
- dev->irq.name = "cp2112-gpio";
- dev->irq.irq_startup = cp2112_gpio_irq_startup;
- dev->irq.irq_shutdown = cp2112_gpio_irq_shutdown;
- dev->irq.irq_ack = cp2112_gpio_irq_ack;
- dev->irq.irq_mask = cp2112_gpio_irq_mask;
- dev->irq.irq_unmask = cp2112_gpio_irq_unmask;
- dev->irq.irq_set_type = cp2112_gpio_irq_type;
- dev->irq.flags = IRQCHIP_MASK_ON_SUSPEND;
-
girq = &dev->gc.irq;
- girq->chip = &dev->irq;
+ gpio_irq_chip_set_chip(girq, &cp2112_irq_chip);
/* The event comes from the outside so no parent handler */
girq->parent_handler = NULL;
girq->num_parents = 0;
--
2.39.2
^ permalink raw reply related
* Re: [PATCH] HID: cp2112: make the irqchip immutable
From: Andy Shevchenko @ 2023-08-22 15:28 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Jiri Kosina, Benjamin Tissoires, linux-gpio,
linux-kernel, linux-input, Bartosz Golaszewski
In-Reply-To: <20230822152244.214394-1-brgl@bgdev.pl>
On Tue, Aug 22, 2023 at 05:22:44PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> This make the GPIO irqchip exposed by the CP2112 driver use an immutable
> irq_chip struct thus addressing the following warning on probe:
>
> (cp2112_gpio): not an immutable chip, please consider fixing it!
https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-next&id=3e2977c425ad2789ca18084fff913cceacae75a2
Can you test HID for-next for this hardware?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] HID: cp2112: make the irqchip immutable
From: Bartosz Golaszewski @ 2023-08-22 15:38 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, Jiri Kosina, Benjamin Tissoires, linux-gpio,
linux-kernel, linux-input, Bartosz Golaszewski
In-Reply-To: <ZOTUHzKKBNcjQLRi@smile.fi.intel.com>
On Tue, Aug 22, 2023 at 5:28 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Aug 22, 2023 at 05:22:44PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > This make the GPIO irqchip exposed by the CP2112 driver use an immutable
> > irq_chip struct thus addressing the following warning on probe:
> >
> > (cp2112_gpio): not an immutable chip, please consider fixing it!
>
> https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-next&id=3e2977c425ad2789ca18084fff913cceacae75a2
>
> Can you test HID for-next for this hardware?
>
Ah you beat me to it. I didn't see this one.
Bart
^ permalink raw reply
* Re: [PATCH] HID: cp2112: make the irqchip immutable
From: Andy Shevchenko @ 2023-08-22 15:49 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Jiri Kosina, Benjamin Tissoires, linux-gpio,
linux-kernel, linux-input, Bartosz Golaszewski
In-Reply-To: <CAMRc=MdWbQPuNh=ziyqD_xVaXDD3nE5yNeq1+d4F_P4_3H=xxQ@mail.gmail.com>
On Tue, Aug 22, 2023 at 05:38:45PM +0200, Bartosz Golaszewski wrote:
> On Tue, Aug 22, 2023 at 5:28 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Aug 22, 2023 at 05:22:44PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > This make the GPIO irqchip exposed by the CP2112 driver use an immutable
> > > irq_chip struct thus addressing the following warning on probe:
> > >
> > > (cp2112_gpio): not an immutable chip, please consider fixing it!
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-next&id=3e2977c425ad2789ca18084fff913cceacae75a2
> >
> > Can you test HID for-next for this hardware?
>
> Ah you beat me to it. I didn't see this one.
Now looking at it, I don't understand why I added a call to IRQ shutdown
callback... (Looks like it slipped from experimental patch.) If you can
test this, would be nice.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [syzbot] [input?] KASAN: slab-use-after-free Read in input_dev_uevent
From: Rahul Rameshbabu @ 2023-08-22 15:57 UTC (permalink / raw)
To: Maxime Ripard
Cc: syzbot, davidgow, dmitry.torokhov, gregkh, linux-input,
linux-kernel, rydberg, syzkaller-bugs, benjamin.tissoires
In-Reply-To: <ijh2qmdtj452nq3idu2tohkrmfwr2qhbhrnyqzxjkkw2lrby53@v2dffcqdohsx>
Hi Maxime,
On Tue, 22 Aug, 2023 11:12:28 +0200 Maxime Ripard <mripard@kernel.org> wrote:
> Hi,
>
> So, we discussed it this morning with Benjamin, and I think the culprit
> is that the uclogic driver will allocate a char array with devm_kzalloc
> in uclogic_input_configured()
> (https://elixir.bootlin.com/linux/latest/source/drivers/hid/hid-uclogic-core.c#L149),
> and will assign input_dev->name to that pointer.
>
> When the device is removed, the devm-allocated array is freed, and the
> input framework will send a uevent in input_dev_uevent() using the
> input_dev->name field:
>
> https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1688
>
> So it's a classic dangling pointer situation.
>
> And even though it was revealed by that patch, I think the issue is
> unrelated. The fundamental issue seems to be that the usage of devm in
> that situation is wrong.
>
> input_dev->name is accessed by input_dev_uevent, which for KOBJ_UNBIND
> and KOBJ_REMOVE will be called after remove.
>
> For example, in __device_release_driver() (with the driver remove hook
> being called in device_remove() and devres_release_all() being called in
> device_unbind_cleanup()):
> https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L1278
>
> So, it looks to me that, with or without the patch we merged recently,
> the core has always sent uevent after device-managed resources were
> freed. Thus, the uclogic (and any other input driver) was wrong in
> allocating its input_dev name with devm_kzalloc (or the phys and uniq
> fields in that struct).
>
> Note that freeing input_dev->name in remove would have been just as bad.
>
> Looking at the code quickly, at least hid-playstation,
> hid-nvidia-shield, hid-logitech-hidpp, mms114 and tsc200x seem to be
> affected by the same issue.
I agree with this analysis overall. At least in hid-nvidia-shield, I can
not use devm for allocating the input name string and explicitly free it
after calling input_unregister_device. In this scenario, the name string
would have been freed explicitly after input_put_device was called
(since the input device is not devres managed). input_put_device would
drop the reference count to zero and the device would be cleaned up at
that point triggering KOBJ_REMOVE and firing off that final
input_dev_uevent.
I think this can be done for a number of the drivers as a workaround
till this issue is properly resolved. If this seems appropriate, I can
send out a series later in the day. This is just a workaround till the
discussion below converges (which I am interested in).
>
> We discussed a couple of solutions with Benjamin, such as creating a
> helper devm action to free and clear the input_dev->name field, droping
> the name, phys and uniq fields from the uevent, or converting name, phys
> and uniq to char arrays so drivers don't have to allocate them.
>
> We couldn't find a perfect one though, so... yeah.
>
> Maxime
--
Thanks,
Rahul Rameshbabu
^ permalink raw reply
* Re: [PATCH v5 0/4] input: touchscreen: add initial support for Goodix Berlin touchscreen IC
From: Neil Armstrong @ 2023-08-22 16:19 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Hans de Goede, Henrik Rydberg,
Jeff LaBundy
Cc: linux-input, linux-arm-msm, devicetree, linux-kernel, Rob Herring
In-Reply-To: <20230801-topic-goodix-berlin-upstream-initial-v5-0-079252935593@linaro.org>
Hi,
On 01/08/2023 14:15, Neil Armstrong wrote:
> These touchscreen ICs support SPI, I2C and I3C interface, up to
> 10 finger touch, stylus and gestures events.
>
> This initial driver is derived from the Goodix goodix_ts_berlin
> available at [1] and [2] and only supports the GT9916 IC
> present on the Qualcomm SM8550 MTP & QRD touch panel.
>
> The current implementation only supports BerlinD, aka GT9916.
>
> Support for advanced features like:
> - Firmware & config update
> - Stylus events
> - Gestures events
> - Previous revisions support (BerlinA or BerlinB)
> is not included in current version.
>
> The current support will work with currently flashed firmware
> and config, and bail out if firmware or config aren't flashed yet.
Gentle ping, is there any changes to be made in order to get this driver in ?
Thanks,
Neil
>
> [1] https://github.com/goodix/goodix_ts_berlin
> [2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
> Changes in v5:
> - rebased on next-20230801
> - Link to v4: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v4-0-0947c489be17@linaro.org
>
> Changes in v4:
> - Core updates:
> - drop kconfig depends, deps will be handled by _SPI and _I2C
> - change power_on() error labels
> - print errors on all dev_err() prints
> - remove useless default variable initialization
> - switch irq touch checksum error to dev_err()
> - add Jeff's review tag
> - I2C changes
> - change REGMAP_I2C Kconfig from depends to select
> - add Jeff's review tag
> - SPI changes
> - add select REGMAP to Kconfig
> - added GOODIX_BERLIN_ prefix to defines
> - switched from ret to error
> - add Jeff's review tag
> - Link to v3: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v3-0-f0577cead709@linaro.org
>
> Changes in v3:
> - Another guge cleanups after Jeff's review:
> - appended goodix_berlin_ before all defines
> - removed some unused defines
> - removed retries on most of read functions, can be added back later
> - added __le to ic_info structures
> - reworked and simplified irq handling, dropped enum and ts_event structs
> - added struct for touch data
> - simplified and cleaned goodix_berlin_check_checksum & goodix_berlin_is_dummy_data
> - moved touch_data_addr to the end of the main code_data
> - reworked probe to get_irq last and right before setip input device
> - cleaned probe by removing the "cd->dev"
> - added short paragraph to justify new driver for berlin devices
> - defined all offsets & masks
> - Added bindings review tag
> - Link to v2: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v2-0-26bc8fe1e90e@linaro.org
>
> Changes in v2:
> - Huge cleanups after Jeff's review:
> - switch to error instead of ret
> - drop dummy vendor/product ids
> - drop unused defined/enums
> - drop unused ic_info and only keep needes values
> - cleanup namings and use goodix_berlin_ everywhere
> - fix regulator setup
> - fix default variables value when assigned afterwars
> - removed indirections
> - dropped debugfs
> - cleaned input_dev setup
> - dropped _remove()
> - sync'ed i2c and spi drivers
> - fixed yaml bindings
> - Link to v1: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v1-0-4a0741b8aefd@linaro.org
>
> ---
> Neil Armstrong (4):
> dt-bindings: input: document Goodix Berlin Touchscreen IC
> input: touchscreen: add core support for Goodix Berlin Touchscreen IC
> input: touchscreen: add I2C support for Goodix Berlin Touchscreen IC
> input: touchscreen: add SPI support for Goodix Berlin Touchscreen IC
>
> .../bindings/input/touchscreen/goodix,gt9916.yaml | 95 ++++
> drivers/input/touchscreen/Kconfig | 31 ++
> drivers/input/touchscreen/Makefile | 3 +
> drivers/input/touchscreen/goodix_berlin.h | 159 ++++++
> drivers/input/touchscreen/goodix_berlin_core.c | 581 +++++++++++++++++++++
> drivers/input/touchscreen/goodix_berlin_i2c.c | 69 +++
> drivers/input/touchscreen/goodix_berlin_spi.c | 173 ++++++
> 7 files changed, 1111 insertions(+)
> ---
> base-commit: a734662572708cf062e974f659ae50c24fc1ad17
> change-id: 20230606-topic-goodix-berlin-upstream-initial-ba97e8ec8f4c
>
> Best regards,
^ permalink raw reply
* Re: [PATCH] HID: logitech-dj: Fix error handling in logi_dj_recv_switch_to_dj_mode()
From: Benjamin Tissoires @ 2023-08-22 18:30 UTC (permalink / raw)
To: Filipe Laíns, Nikita Zhandarovich
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
lvc-project
In-Reply-To: <20230613101635.77820-1-n.zhandarovich@fintech.ru>
On Tue, 13 Jun 2023 03:16:35 -0700, Nikita Zhandarovich wrote:
> Presently, if a call to logi_dj_recv_send_report() fails, we do
> not learn about the error until after sending short
> HID_OUTPUT_REPORT with hid_hw_raw_request().
> To handle this somewhat unlikely issue, return on error in
> logi_dj_recv_send_report() (minding ugly sleep workaround) and
> take into account the result of hid_hw_raw_request().
>
> [...]
Applied to hid/hid.git (for-6.6/logitech), thanks!
[1/1] HID: logitech-dj: Fix error handling in logi_dj_recv_switch_to_dj_mode()
https://git.kernel.org/hid/hid/c/6f20d3261265
Cheers,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* Re: [PATCH] HID: cp2112: make the irqchip immutable
From: Bartosz Golaszewski @ 2023-08-22 19:27 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, Jiri Kosina, Benjamin Tissoires, linux-gpio,
linux-kernel, linux-input, Bartosz Golaszewski
In-Reply-To: <ZOTZHn1BzO6oZYOH@smile.fi.intel.com>
On Tue, Aug 22, 2023 at 5:50 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Aug 22, 2023 at 05:38:45PM +0200, Bartosz Golaszewski wrote:
> > On Tue, Aug 22, 2023 at 5:28 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Tue, Aug 22, 2023 at 05:22:44PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > This make the GPIO irqchip exposed by the CP2112 driver use an immutable
> > > > irq_chip struct thus addressing the following warning on probe:
> > > >
> > > > (cp2112_gpio): not an immutable chip, please consider fixing it!
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-next&id=3e2977c425ad2789ca18084fff913cceacae75a2
> > >
> > > Can you test HID for-next for this hardware?
> >
> > Ah you beat me to it. I didn't see this one.
>
> Now looking at it, I don't understand why I added a call to IRQ shutdown
> callback... (Looks like it slipped from experimental patch.) If you can
> test this, would be nice.
>
Yeah it works alright.
It's already in next but FWIW:
Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
^ permalink raw reply
* Re: [PATCH v3 03/32] dt-bindings: bus: convert qcom,ssbi schema to YAML format
From: Rob Herring @ 2023-08-22 20:24 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: linux-input, devicetree, Krzysztof Kozlowski, linux-arm-msm,
Rob Herring, Bjorn Andersson, Andy Gross, Konrad Dybcio
In-Reply-To: <20230822001349.899298-4-dmitry.baryshkov@linaro.org>
On Tue, 22 Aug 2023 03:13:20 +0300, Dmitry Baryshkov wrote:
> Convert arm/msm/ssbi.txt yo YAML, moving it to the directory with bus
> bindings.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> .../devicetree/bindings/arm/msm/ssbi.txt | 18 ------
> .../devicetree/bindings/bus/qcom,ssbi.yaml | 63 +++++++++++++++++++
> 2 files changed, 63 insertions(+), 18 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/arm/msm/ssbi.txt
> create mode 100644 Documentation/devicetree/bindings/bus/qcom,ssbi.yaml
>
Applied, thanks!
^ permalink raw reply
* selftests: hid: trouble building with clang due to missing header
From: Justin Stitt @ 2023-08-22 20:33 UTC (permalink / raw)
To: linux-kselftest
Cc: bpf, linux-input, Linux Kernel Mailing List, Nathan Chancellor,
Nick Desaulniers, Kees Cook
Hi, I'd like to get some help with building the kselftest target.
I am running into some warnings within the hid tree:
| progs/hid_bpf_helpers.h:9:38: error: declaration of 'struct
hid_bpf_ctx' will \
| not be visible outside of this function [-Werror,-Wvisibility]
| 9 | extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
| | ^
| progs/hid.c:23:35: error: incompatible pointer types passing 'struct
hid_bpf_ctx *' \
| to parameter of type 'struct hid_bpf_ctx *'
[-Werror,-Wincompatible-pointer-types]
| 23 | __u8 *rw_data = hid_bpf_get_data(hid_ctx, 0 /*
offset */, 3 /* size */);
This warning, amongst others, is due to some symbol not being included.
In this case, `struct hid_bpf_ctx` is not being defined anywhere that I
can see inside of the testing tree itself.
Instead, `struct hid_bpf_ctx` is defined and implemented at
`include/linux/hid_bpf.h`. AFAIK, I cannot just include this header as
the tools directory is a separate entity from kbuild and these tests are
meant to be built/ran without relying on kernel headers. Am I correct in
this assumption? At any rate, the include itself doesn't work. How can I
properly include this struct definition and fix the warning(s)?
Please note that we cannot just forward declare the struct as it is
being dereferenced and would then yield a completely different
error/warning for an incomplete type. We need the entire implementation
for the struct included.
Other symbols also defined in `include/linux/hid_bpf.h` that we need are
`struct hid_report_type` and `HID_BPF_FLAG...`
Here's the invocation I am running to build kselftest:
`$ make LLVM=1 ARCH=x86_64 mrproper headers && make LLVM=1 ARCH=x86_64
-j128 V=1 -C tools/testing/selftests`
If anyone is currently getting clean builds of kselftest with clang,
what invocation works for you?
Link: https://github.com/ClangBuiltLinux/linux/issues/1698
Full-build-log:
https://gist.github.com/JustinStitt/b217f6e47c1d762e5e1cc6c3532f1bbb
(V=1)
Thanks.
Justin
^ permalink raw reply
* Re: selftests: hid: trouble building with clang due to missing header
From: Nick Desaulniers @ 2023-08-22 20:44 UTC (permalink / raw)
To: Justin Stitt, Benjamin Tissoires
Cc: linux-kselftest, bpf, linux-input, Linux Kernel Mailing List,
Nathan Chancellor, Kees Cook
In-Reply-To: <CAFhGd8ryUcu2yPC+dFyDKNuVFHxT-=iayG+n2iErotBxgd0FVw@mail.gmail.com>
+ Ben, author of commit dbb60c8a26da ("selftests: add tests for the
HID-bpf initial implementation")
On Tue, Aug 22, 2023 at 1:34 PM Justin Stitt <justinstitt@google.com> wrote:
>
> Hi, I'd like to get some help with building the kselftest target.
>
> I am running into some warnings within the hid tree:
> | progs/hid_bpf_helpers.h:9:38: error: declaration of 'struct
> hid_bpf_ctx' will \
> | not be visible outside of this function [-Werror,-Wvisibility]
> | 9 | extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
> | | ^
> | progs/hid.c:23:35: error: incompatible pointer types passing 'struct
> hid_bpf_ctx *' \
> | to parameter of type 'struct hid_bpf_ctx *'
> [-Werror,-Wincompatible-pointer-types]
> | 23 | __u8 *rw_data = hid_bpf_get_data(hid_ctx, 0 /*
> offset */, 3 /* size */);
>
> This warning, amongst others, is due to some symbol not being included.
> In this case, `struct hid_bpf_ctx` is not being defined anywhere that I
> can see inside of the testing tree itself.
>
> Instead, `struct hid_bpf_ctx` is defined and implemented at
> `include/linux/hid_bpf.h`. AFAIK, I cannot just include this header as
> the tools directory is a separate entity from kbuild and these tests are
> meant to be built/ran without relying on kernel headers. Am I correct in
> this assumption? At any rate, the include itself doesn't work. How can I
> properly include this struct definition and fix the warning(s)?
>
> Please note that we cannot just forward declare the struct as it is
> being dereferenced and would then yield a completely different
> error/warning for an incomplete type. We need the entire implementation
> for the struct included.
>
> Other symbols also defined in `include/linux/hid_bpf.h` that we need are
> `struct hid_report_type` and `HID_BPF_FLAG...`
>
> Here's the invocation I am running to build kselftest:
> `$ make LLVM=1 ARCH=x86_64 mrproper headers && make LLVM=1 ARCH=x86_64
> -j128 V=1 -C tools/testing/selftests`
>
> If anyone is currently getting clean builds of kselftest with clang,
> what invocation works for you?
>
>
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1698
> Full-build-log:
> https://gist.github.com/JustinStitt/b217f6e47c1d762e5e1cc6c3532f1bbb
> (V=1)
>
> Thanks.
> Justin
--
Thanks,
~Nick Desaulniers
^ permalink raw reply
* Re: selftests: hid: trouble building with clang due to missing header
From: Benjamin Tissoires @ 2023-08-22 20:51 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Justin Stitt, linux-kselftest, bpf, linux-input,
Linux Kernel Mailing List, Nathan Chancellor, Kees Cook
In-Reply-To: <CAKwvOd=p_7gWwBnR_RHUPukkG1A25GQy6iOnX_eih7u65u=oxw@mail.gmail.com>
Justin,
On Tue, Aug 22, 2023 at 10:44 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> + Ben, author of commit dbb60c8a26da ("selftests: add tests for the
> HID-bpf initial implementation")
>
> On Tue, Aug 22, 2023 at 1:34 PM Justin Stitt <justinstitt@google.com> wrote:
> >
> > Hi, I'd like to get some help with building the kselftest target.
> >
> > I am running into some warnings within the hid tree:
> > | progs/hid_bpf_helpers.h:9:38: error: declaration of 'struct
> > hid_bpf_ctx' will \
> > | not be visible outside of this function [-Werror,-Wvisibility]
> > | 9 | extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
> > | | ^
> > | progs/hid.c:23:35: error: incompatible pointer types passing 'struct
> > hid_bpf_ctx *' \
> > | to parameter of type 'struct hid_bpf_ctx *'
> > [-Werror,-Wincompatible-pointer-types]
> > | 23 | __u8 *rw_data = hid_bpf_get_data(hid_ctx, 0 /*
> > offset */, 3 /* size */);
> >
> > This warning, amongst others, is due to some symbol not being included.
> > In this case, `struct hid_bpf_ctx` is not being defined anywhere that I
> > can see inside of the testing tree itself.
> >
> > Instead, `struct hid_bpf_ctx` is defined and implemented at
> > `include/linux/hid_bpf.h`. AFAIK, I cannot just include this header as
> > the tools directory is a separate entity from kbuild and these tests are
> > meant to be built/ran without relying on kernel headers. Am I correct in
> > this assumption? At any rate, the include itself doesn't work. How can I
> > properly include this struct definition and fix the warning(s)?
> >
> > Please note that we cannot just forward declare the struct as it is
> > being dereferenced and would then yield a completely different
> > error/warning for an incomplete type. We need the entire implementation
> > for the struct included.
> >
> > Other symbols also defined in `include/linux/hid_bpf.h` that we need are
> > `struct hid_report_type` and `HID_BPF_FLAG...`
> >
> > Here's the invocation I am running to build kselftest:
> > `$ make LLVM=1 ARCH=x86_64 mrproper headers && make LLVM=1 ARCH=x86_64
> > -j128 V=1 -C tools/testing/selftests`
I think I fixed the same issue in the script I am running to launch
those tests in a VM. This was in commit
f9abdcc617dad5f14bbc2ebe96ee99f3e6de0c4e (in the v6.5-rc+ series).
And in the commit log, I wrote:
```
According to commit 01d6c48a828b ("Documentation: kselftest:
"make headers" is a prerequisite"), running the kselftests requires
to run "make headers" first.
```
So my assumption is that you also need to run "make headers" with the
proper flags before compiling the selftests themselves (I might be
wrong but that's how I read the commit).
Cheers,
Benjamin
> >
> > If anyone is currently getting clean builds of kselftest with clang,
> > what invocation works for you?
> >
> >
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1698
> > Full-build-log:
> > https://gist.github.com/JustinStitt/b217f6e47c1d762e5e1cc6c3532f1bbb
> > (V=1)
> >
> > Thanks.
> > Justin
>
>
>
> --
> Thanks,
> ~Nick Desaulniers
>
^ permalink raw reply
* Re: selftests: hid: trouble building with clang due to missing header
From: Justin Stitt @ 2023-08-22 20:57 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Nick Desaulniers, linux-kselftest, bpf, linux-input,
Linux Kernel Mailing List, Nathan Chancellor, Kees Cook
In-Reply-To: <CAO-hwJLio2dWs01VAhCgmub5GVxRU-3RFQifviOL0OTaqj9Ktg@mail.gmail.com>
On Tue, Aug 22, 2023 at 1:52 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Justin,
>
> On Tue, Aug 22, 2023 at 10:44 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > + Ben, author of commit dbb60c8a26da ("selftests: add tests for the
> > HID-bpf initial implementation")
> >
> > On Tue, Aug 22, 2023 at 1:34 PM Justin Stitt <justinstitt@google.com> wrote:
> > >
> > > Hi, I'd like to get some help with building the kselftest target.
> > >
> > > I am running into some warnings within the hid tree:
> > > | progs/hid_bpf_helpers.h:9:38: error: declaration of 'struct
> > > hid_bpf_ctx' will \
> > > | not be visible outside of this function [-Werror,-Wvisibility]
> > > | 9 | extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
> > > | | ^
> > > | progs/hid.c:23:35: error: incompatible pointer types passing 'struct
> > > hid_bpf_ctx *' \
> > > | to parameter of type 'struct hid_bpf_ctx *'
> > > [-Werror,-Wincompatible-pointer-types]
> > > | 23 | __u8 *rw_data = hid_bpf_get_data(hid_ctx, 0 /*
> > > offset */, 3 /* size */);
> > >
> > > This warning, amongst others, is due to some symbol not being included.
> > > In this case, `struct hid_bpf_ctx` is not being defined anywhere that I
> > > can see inside of the testing tree itself.
> > >
> > > Instead, `struct hid_bpf_ctx` is defined and implemented at
> > > `include/linux/hid_bpf.h`. AFAIK, I cannot just include this header as
> > > the tools directory is a separate entity from kbuild and these tests are
> > > meant to be built/ran without relying on kernel headers. Am I correct in
> > > this assumption? At any rate, the include itself doesn't work. How can I
> > > properly include this struct definition and fix the warning(s)?
> > >
> > > Please note that we cannot just forward declare the struct as it is
> > > being dereferenced and would then yield a completely different
> > > error/warning for an incomplete type. We need the entire implementation
> > > for the struct included.
> > >
> > > Other symbols also defined in `include/linux/hid_bpf.h` that we need are
> > > `struct hid_report_type` and `HID_BPF_FLAG...`
> > >
> > > Here's the invocation I am running to build kselftest:
> > > `$ make LLVM=1 ARCH=x86_64 mrproper headers && make LLVM=1 ARCH=x86_64
> > > -j128 V=1 -C tools/testing/selftests`
>
> I think I fixed the same issue in the script I am running to launch
> those tests in a VM. This was in commit
> f9abdcc617dad5f14bbc2ebe96ee99f3e6de0c4e (in the v6.5-rc+ series).
>
> And in the commit log, I wrote:
> ```
> According to commit 01d6c48a828b ("Documentation: kselftest:
> "make headers" is a prerequisite"), running the kselftests requires
> to run "make headers" first.
> ```
>
> So my assumption is that you also need to run "make headers" with the
> proper flags before compiling the selftests themselves (I might be
> wrong but that's how I read the commit).
In my original email I pasted the invocation I used which includes the
headers target. What are the "proper flags" in this case?
>
> Cheers,
> Benjamin
>
> > >
> > > If anyone is currently getting clean builds of kselftest with clang,
> > > what invocation works for you?
> > >
> > >
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/1698
> > > Full-build-log:
> > > https://gist.github.com/JustinStitt/b217f6e47c1d762e5e1cc6c3532f1bbb
> > > (V=1)
> > >
> > > Thanks.
> > > Justin
> >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
> >
>
^ permalink raw reply
* Re: selftests: hid: trouble building with clang due to missing header
From: Benjamin Tissoires @ 2023-08-22 21:06 UTC (permalink / raw)
To: Justin Stitt
Cc: Nick Desaulniers, linux-kselftest, bpf, linux-input,
Linux Kernel Mailing List, Nathan Chancellor, Kees Cook
In-Reply-To: <CAFhGd8qmXD6VN+nuXKtV_Uz14gzY1Kqo7tmOAhgYpTBdCnoJRQ@mail.gmail.com>
On Tue, Aug 22, 2023 at 10:57 PM Justin Stitt <justinstitt@google.com> wrote:
>
[...]
> > > > Here's the invocation I am running to build kselftest:
> > > > `$ make LLVM=1 ARCH=x86_64 mrproper headers && make LLVM=1 ARCH=x86_64
> > > > -j128 V=1 -C tools/testing/selftests`
> >
> > I think I fixed the same issue in the script I am running to launch
> > those tests in a VM. This was in commit
> > f9abdcc617dad5f14bbc2ebe96ee99f3e6de0c4e (in the v6.5-rc+ series).
> >
> > And in the commit log, I wrote:
> > ```
> > According to commit 01d6c48a828b ("Documentation: kselftest:
> > "make headers" is a prerequisite"), running the kselftests requires
> > to run "make headers" first.
> > ```
> >
> > So my assumption is that you also need to run "make headers" with the
> > proper flags before compiling the selftests themselves (I might be
> > wrong but that's how I read the commit).
>
> In my original email I pasted the invocation I used which includes the
> headers target. What are the "proper flags" in this case?
>
"make LLVM=1 ARCH=x86_64 headers" no?
But now I'm starting to wonder if that was not the intent of your
combined "make mrproper headers". I honestly never tried to combine
the 2. It's worth a try to split them I would say.
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH v2] dt-bindings: input: convert syna,rmi4 to DT schema
From: Rob Herring @ 2023-08-22 21:09 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rob Herring, Jason A. Donenfeld, devicetree, Dmitry Torokhov,
linux-input, Krzysztof Kozlowski, Conor Dooley, Matthias Schiffer,
linux-kernel, Vincent Huang
In-Reply-To: <20230728165054.88678-1-krzysztof.kozlowski@linaro.org>
On Fri, 28 Jul 2023 18:50:54 +0200, Krzysztof Kozlowski wrote:
> Convert the bindings for Synaptics RMI4 bus and devices to DT schema.
> Changes during conversion:
> 1. Add reset-gpios already used in DTS and mentioned by RMI4
> specification.
> 2. Do not require address/size cells, because without functions
> (children) they aren't really needed.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> ---
>
> Changes in v2:
> 1. Correct [0-9a-z] to [0-9a-f] (Rob).
>
> Jason, Matthias, Vincent,
> I put your names as maintainers, because moderately recently you were
> changing the driver. Let me know if this is okay or you prefer not to
> maintain the hardware.
> ---
> .../bindings/input/rmi4/rmi_2d_sensor.txt | 56 ----
> .../bindings/input/rmi4/rmi_f01.txt | 39 ---
> .../bindings/input/rmi4/rmi_i2c.txt | 61 ----
> .../bindings/input/rmi4/rmi_spi.txt | 56 ----
> .../devicetree/bindings/input/syna,rmi4.yaml | 271 ++++++++++++++++++
> 5 files changed, 271 insertions(+), 212 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/input/rmi4/rmi_2d_sensor.txt
> delete mode 100644 Documentation/devicetree/bindings/input/rmi4/rmi_f01.txt
> delete mode 100644 Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
> delete mode 100644 Documentation/devicetree/bindings/input/rmi4/rmi_spi.txt
> create mode 100644 Documentation/devicetree/bindings/input/syna,rmi4.yaml
>
Since this hasn't been applied yet, I applied it, thanks!
^ permalink raw reply
* Re: selftests: hid: trouble building with clang due to missing header
From: Benjamin Tissoires @ 2023-08-22 21:14 UTC (permalink / raw)
To: Justin Stitt
Cc: Nick Desaulniers, linux-kselftest, bpf, linux-input,
Linux Kernel Mailing List, Nathan Chancellor, Kees Cook
In-Reply-To: <CAO-hwJJ_ipXwLjyhGC6_4r-uZ-sDbrb_W7um6F2vgws0d-hvTQ@mail.gmail.com>
On Tue, Aug 22, 2023 at 11:06 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Tue, Aug 22, 2023 at 10:57 PM Justin Stitt <justinstitt@google.com> wrote:
> >
> [...]
> > > > > Here's the invocation I am running to build kselftest:
> > > > > `$ make LLVM=1 ARCH=x86_64 mrproper headers && make LLVM=1 ARCH=x86_64
> > > > > -j128 V=1 -C tools/testing/selftests`
> > >
> > > I think I fixed the same issue in the script I am running to launch
> > > those tests in a VM. This was in commit
> > > f9abdcc617dad5f14bbc2ebe96ee99f3e6de0c4e (in the v6.5-rc+ series).
> > >
> > > And in the commit log, I wrote:
> > > ```
> > > According to commit 01d6c48a828b ("Documentation: kselftest:
> > > "make headers" is a prerequisite"), running the kselftests requires
> > > to run "make headers" first.
> > > ```
> > >
> > > So my assumption is that you also need to run "make headers" with the
> > > proper flags before compiling the selftests themselves (I might be
> > > wrong but that's how I read the commit).
> >
> > In my original email I pasted the invocation I used which includes the
> > headers target. What are the "proper flags" in this case?
> >
>
> "make LLVM=1 ARCH=x86_64 headers" no?
>
> But now I'm starting to wonder if that was not the intent of your
> combined "make mrproper headers". I honestly never tried to combine
> the 2. It's worth a try to split them I would say.
Apologies, I just tested it, and it works (combining the 2).
Which kernel are you trying to test?
I tested your 2 commands on v6.5-rc7 and it just works.
FTR:
$> git checkout v6.5-rc7
$> make LLVM=1 ARCH=x86_64 mrproper headers
$> make LLVM=1 ARCH=x86_64 -j8 -C tools/testing/selftests TARGETS=hid
-> BINARY hid_bpf
Cheers,
Benjamin
^ permalink raw reply
* Re: selftests: hid: trouble building with clang due to missing header
From: Justin Stitt @ 2023-08-22 21:26 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Nick Desaulniers, linux-kselftest, bpf, linux-input,
Linux Kernel Mailing List, Nathan Chancellor, Kees Cook
In-Reply-To: <CAO-hwJ+DTPXWbpNaBDvCkyAsWZHbeLiBwYo4k93ZW79Jt-HAkg@mail.gmail.com>
On Tue, Aug 22, 2023 at 2:15 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Tue, Aug 22, 2023 at 11:06 PM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > On Tue, Aug 22, 2023 at 10:57 PM Justin Stitt <justinstitt@google.com> wrote:
> > >
> > [...]
> > > > > > Here's the invocation I am running to build kselftest:
> > > > > > `$ make LLVM=1 ARCH=x86_64 mrproper headers && make LLVM=1 ARCH=x86_64
> > > > > > -j128 V=1 -C tools/testing/selftests`
> > > >
> > > > I think I fixed the same issue in the script I am running to launch
> > > > those tests in a VM. This was in commit
> > > > f9abdcc617dad5f14bbc2ebe96ee99f3e6de0c4e (in the v6.5-rc+ series).
> > > >
> > > > And in the commit log, I wrote:
> > > > ```
> > > > According to commit 01d6c48a828b ("Documentation: kselftest:
> > > > "make headers" is a prerequisite"), running the kselftests requires
> > > > to run "make headers" first.
> > > > ```
> > > >
> > > > So my assumption is that you also need to run "make headers" with the
> > > > proper flags before compiling the selftests themselves (I might be
> > > > wrong but that's how I read the commit).
> > >
> > > In my original email I pasted the invocation I used which includes the
> > > headers target. What are the "proper flags" in this case?
> > >
> >
> > "make LLVM=1 ARCH=x86_64 headers" no?
> >
> > But now I'm starting to wonder if that was not the intent of your
> > combined "make mrproper headers". I honestly never tried to combine
> > the 2. It's worth a try to split them I would say.
>
> Apologies, I just tested it, and it works (combining the 2).
>
> Which kernel are you trying to test?
> I tested your 2 commands on v6.5-rc7 and it just works.
I'm also on v6.5-rc7 (706a741595047797872e669b3101429ab8d378ef)
I ran these exact commands:
| $ make mrproper
| $ make LLVM=1 ARCH=x86_64 headers
| $ make LLVM=1 ARCH=x86_64 -j128 -C tools/testing/selftests
TARGETS=hid &> out
and here's the contents of `out` (still warnings/errors):
https://gist.github.com/JustinStitt/d0c30180a2a2e046c32d5f0ce5f59c6d
I have a feeling I'm doing something fundamentally incorrectly. Any ideas?
To be clear, I can build the Kernel itself just fine across many
configs and architectures. I have, at the very least, the dependencies
required to accomplish that.
>
> FTR:
> $> git checkout v6.5-rc7
> $> make LLVM=1 ARCH=x86_64 mrproper headers
> $> make LLVM=1 ARCH=x86_64 -j8 -C tools/testing/selftests TARGETS=hid
>
> -> BINARY hid_bpf
>
> Cheers,
> Benjamin
>
^ permalink raw reply
* Re: selftests: hid: trouble building with clang due to missing header
From: Benjamin Tissoires @ 2023-08-22 21:36 UTC (permalink / raw)
To: Justin Stitt
Cc: Nick Desaulniers, linux-kselftest, bpf, linux-input,
Linux Kernel Mailing List, Nathan Chancellor, Kees Cook
In-Reply-To: <CAFhGd8pVjUPpukHxxbQCEnmgDUqy-tgBa7POkmgrYyFXVRAMEw@mail.gmail.com>
On Tue, Aug 22, 2023 at 11:26 PM Justin Stitt <justinstitt@google.com> wrote:
>
> On Tue, Aug 22, 2023 at 2:15 PM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > On Tue, Aug 22, 2023 at 11:06 PM Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > >
> > > On Tue, Aug 22, 2023 at 10:57 PM Justin Stitt <justinstitt@google.com> wrote:
> > > >
> > > [...]
> > > > > > > Here's the invocation I am running to build kselftest:
> > > > > > > `$ make LLVM=1 ARCH=x86_64 mrproper headers && make LLVM=1 ARCH=x86_64
> > > > > > > -j128 V=1 -C tools/testing/selftests`
> > > > >
> > > > > I think I fixed the same issue in the script I am running to launch
> > > > > those tests in a VM. This was in commit
> > > > > f9abdcc617dad5f14bbc2ebe96ee99f3e6de0c4e (in the v6.5-rc+ series).
> > > > >
> > > > > And in the commit log, I wrote:
> > > > > ```
> > > > > According to commit 01d6c48a828b ("Documentation: kselftest:
> > > > > "make headers" is a prerequisite"), running the kselftests requires
> > > > > to run "make headers" first.
> > > > > ```
> > > > >
> > > > > So my assumption is that you also need to run "make headers" with the
> > > > > proper flags before compiling the selftests themselves (I might be
> > > > > wrong but that's how I read the commit).
> > > >
> > > > In my original email I pasted the invocation I used which includes the
> > > > headers target. What are the "proper flags" in this case?
> > > >
> > >
> > > "make LLVM=1 ARCH=x86_64 headers" no?
> > >
> > > But now I'm starting to wonder if that was not the intent of your
> > > combined "make mrproper headers". I honestly never tried to combine
> > > the 2. It's worth a try to split them I would say.
> >
> > Apologies, I just tested it, and it works (combining the 2).
> >
> > Which kernel are you trying to test?
> > I tested your 2 commands on v6.5-rc7 and it just works.
>
> I'm also on v6.5-rc7 (706a741595047797872e669b3101429ab8d378ef)
>
> I ran these exact commands:
> | $ make mrproper
> | $ make LLVM=1 ARCH=x86_64 headers
> | $ make LLVM=1 ARCH=x86_64 -j128 -C tools/testing/selftests
> TARGETS=hid &> out
>
> and here's the contents of `out` (still warnings/errors):
> https://gist.github.com/JustinStitt/d0c30180a2a2e046c32d5f0ce5f59c6d
>
> I have a feeling I'm doing something fundamentally incorrectly. Any ideas?
Sigh... there is a high chance my Makefile is not correct and uses the
installed headers (I was running the exact same commands, but on a
v6.4-rc7+ kernel).
But sorry, it will have to wait for tomorrow if you want me to have a
look at it. It's 11:35 PM here, and I need to go to bed
Cheers,
Benjamin
>
> To be clear, I can build the Kernel itself just fine across many
> configs and architectures. I have, at the very least, the dependencies
> required to accomplish that.
>
> >
> > FTR:
> > $> git checkout v6.5-rc7
> > $> make LLVM=1 ARCH=x86_64 mrproper headers
> > $> make LLVM=1 ARCH=x86_64 -j8 -C tools/testing/selftests TARGETS=hid
> >
> > -> BINARY hid_bpf
> >
> > Cheers,
> > Benjamin
> >
>
^ permalink raw reply
* Re: selftests: hid: trouble building with clang due to missing header
From: Justin Stitt @ 2023-08-22 21:38 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Nick Desaulniers, linux-kselftest, bpf, linux-input,
Linux Kernel Mailing List, Nathan Chancellor, Kees Cook
In-Reply-To: <CAO-hwJJntQTzcJH5nf9RM1bVWGVW1kb28rJ3tgew1AEH00PmJQ@mail.gmail.com>
On Tue, Aug 22, 2023 at 2:36 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Tue, Aug 22, 2023 at 11:26 PM Justin Stitt <justinstitt@google.com> wrote:
> >
> > On Tue, Aug 22, 2023 at 2:15 PM Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > >
> > > On Tue, Aug 22, 2023 at 11:06 PM Benjamin Tissoires
> > > <benjamin.tissoires@redhat.com> wrote:
> > > >
> > > > On Tue, Aug 22, 2023 at 10:57 PM Justin Stitt <justinstitt@google.com> wrote:
> > > > >
> > > > [...]
> > > > > > > > Here's the invocation I am running to build kselftest:
> > > > > > > > `$ make LLVM=1 ARCH=x86_64 mrproper headers && make LLVM=1 ARCH=x86_64
> > > > > > > > -j128 V=1 -C tools/testing/selftests`
> > > > > >
> > > > > > I think I fixed the same issue in the script I am running to launch
> > > > > > those tests in a VM. This was in commit
> > > > > > f9abdcc617dad5f14bbc2ebe96ee99f3e6de0c4e (in the v6.5-rc+ series).
> > > > > >
> > > > > > And in the commit log, I wrote:
> > > > > > ```
> > > > > > According to commit 01d6c48a828b ("Documentation: kselftest:
> > > > > > "make headers" is a prerequisite"), running the kselftests requires
> > > > > > to run "make headers" first.
> > > > > > ```
> > > > > >
> > > > > > So my assumption is that you also need to run "make headers" with the
> > > > > > proper flags before compiling the selftests themselves (I might be
> > > > > > wrong but that's how I read the commit).
> > > > >
> > > > > In my original email I pasted the invocation I used which includes the
> > > > > headers target. What are the "proper flags" in this case?
> > > > >
> > > >
> > > > "make LLVM=1 ARCH=x86_64 headers" no?
> > > >
> > > > But now I'm starting to wonder if that was not the intent of your
> > > > combined "make mrproper headers". I honestly never tried to combine
> > > > the 2. It's worth a try to split them I would say.
> > >
> > > Apologies, I just tested it, and it works (combining the 2).
> > >
> > > Which kernel are you trying to test?
> > > I tested your 2 commands on v6.5-rc7 and it just works.
> >
> > I'm also on v6.5-rc7 (706a741595047797872e669b3101429ab8d378ef)
> >
> > I ran these exact commands:
> > | $ make mrproper
> > | $ make LLVM=1 ARCH=x86_64 headers
> > | $ make LLVM=1 ARCH=x86_64 -j128 -C tools/testing/selftests
> > TARGETS=hid &> out
> >
> > and here's the contents of `out` (still warnings/errors):
> > https://gist.github.com/JustinStitt/d0c30180a2a2e046c32d5f0ce5f59c6d
> >
> > I have a feeling I'm doing something fundamentally incorrectly. Any ideas?
>
> Sigh... there is a high chance my Makefile is not correct and uses the
> installed headers (I was running the exact same commands, but on a
> v6.4-rc7+ kernel).
>
> But sorry, it will have to wait for tomorrow if you want me to have a
> look at it. It's 11:35 PM here, and I need to go to bed
Take it easy. Thanks for the prompt responses here! I'd like to get
the entire kselftest make target building with Clang so that we can
close [1].
>
> Cheers,
> Benjamin
>
> >
> > To be clear, I can build the Kernel itself just fine across many
> > configs and architectures. I have, at the very least, the dependencies
> > required to accomplish that.
> >
> > >
> > > FTR:
> > > $> git checkout v6.5-rc7
> > > $> make LLVM=1 ARCH=x86_64 mrproper headers
> > > $> make LLVM=1 ARCH=x86_64 -j8 -C tools/testing/selftests TARGETS=hid
> > >
> > > -> BINARY hid_bpf
> > >
> > > Cheers,
> > > Benjamin
> > >
> >
>
[1]: https://github.com/ClangBuiltLinux/linux/issues/1910
^ permalink raw reply
* Re: selftests: hid: trouble building with clang due to missing header
From: Justin Stitt @ 2023-08-22 21:42 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Nick Desaulniers, linux-kselftest, bpf, linux-input,
Linux Kernel Mailing List, Nathan Chancellor, Kees Cook
In-Reply-To: <CAFhGd8rgdszt5vgWuGKkcpTZbKvihGCJXRKKq7RP17+71dTYww@mail.gmail.com>
On Tue, Aug 22, 2023 at 02:38:29PM -0700, Justin Stitt wrote:
> On Tue, Aug 22, 2023 at 2:36 PM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > On Tue, Aug 22, 2023 at 11:26 PM Justin Stitt <justinstitt@google.com> wrote:
> > >
> > > On Tue, Aug 22, 2023 at 2:15 PM Benjamin Tissoires
> > > <benjamin.tissoires@redhat.com> wrote:
> > > >
> > > > On Tue, Aug 22, 2023 at 11:06 PM Benjamin Tissoires
> > > > <benjamin.tissoires@redhat.com> wrote:
> > > > >
> > > > > On Tue, Aug 22, 2023 at 10:57 PM Justin Stitt <justinstitt@google.com> wrote:
> > > > > >
> > > > > [...]
> > > > > > > > > Here's the invocation I am running to build kselftest:
> > > > > > > > > `$ make LLVM=1 ARCH=x86_64 mrproper headers && make LLVM=1 ARCH=x86_64
> > > > > > > > > -j128 V=1 -C tools/testing/selftests`
> > > > > > >
> > > > > > > I think I fixed the same issue in the script I am running to launch
> > > > > > > those tests in a VM. This was in commit
> > > > > > > f9abdcc617dad5f14bbc2ebe96ee99f3e6de0c4e (in the v6.5-rc+ series).
> > > > > > >
> > > > > > > And in the commit log, I wrote:
> > > > > > > ```
> > > > > > > According to commit 01d6c48a828b ("Documentation: kselftest:
> > > > > > > "make headers" is a prerequisite"), running the kselftests requires
> > > > > > > to run "make headers" first.
> > > > > > > ```
> > > > > > >
> > > > > > > So my assumption is that you also need to run "make headers" with the
> > > > > > > proper flags before compiling the selftests themselves (I might be
> > > > > > > wrong but that's how I read the commit).
> > > > > >
> > > > > > In my original email I pasted the invocation I used which includes the
> > > > > > headers target. What are the "proper flags" in this case?
> > > > > >
> > > > >
> > > > > "make LLVM=1 ARCH=x86_64 headers" no?
> > > > >
> > > > > But now I'm starting to wonder if that was not the intent of your
> > > > > combined "make mrproper headers". I honestly never tried to combine
> > > > > the 2. It's worth a try to split them I would say.
> > > >
> > > > Apologies, I just tested it, and it works (combining the 2).
> > > >
> > > > Which kernel are you trying to test?
> > > > I tested your 2 commands on v6.5-rc7 and it just works.
> > >
> > > I'm also on v6.5-rc7 (706a741595047797872e669b3101429ab8d378ef)
> > >
> > > I ran these exact commands:
> > > | $ make mrproper
> > > | $ make LLVM=1 ARCH=x86_64 headers
> > > | $ make LLVM=1 ARCH=x86_64 -j128 -C tools/testing/selftests
> > > TARGETS=hid &> out
> > >
> > > and here's the contents of `out` (still warnings/errors):
> > > https://gist.github.com/JustinStitt/d0c30180a2a2e046c32d5f0ce5f59c6d
> > >
> > > I have a feeling I'm doing something fundamentally incorrectly. Any ideas?
> >
> > Sigh... there is a high chance my Makefile is not correct and uses the
> > installed headers (I was running the exact same commands, but on a
> > v6.4-rc7+ kernel).
> >
> > But sorry, it will have to wait for tomorrow if you want me to have a
> > look at it. It's 11:35 PM here, and I need to go to bed
> Take it easy. Thanks for the prompt responses here! I'd like to get
> the entire kselftest make target building with Clang so that we can
> close [1].
>
> >
> > Cheers,
> > Benjamin
> >
> > >
> > > To be clear, I can build the Kernel itself just fine across many
> > > configs and architectures. I have, at the very least, the dependencies
> > > required to accomplish that.
> > >
> > > >
> > > > FTR:
> > > > $> git checkout v6.5-rc7
> > > > $> make LLVM=1 ARCH=x86_64 mrproper headers
> > > > $> make LLVM=1 ARCH=x86_64 -j8 -C tools/testing/selftests TARGETS=hid
> > > >
> > > > -> BINARY hid_bpf
> > > >
> > > > Cheers,
> > > > Benjamin
> > > >
> > >
> >
>
> [1]: https://github.com/ClangBuiltLinux/linux/issues/1910
Erhm, I meant [1]: https://github.com/ClangBuiltLinux/linux/issues/1698
^ permalink raw reply
* Re: [syzbot] [input?] KASAN: slab-use-after-free Read in input_dev_uevent
From: Rahul Rameshbabu @ 2023-08-22 22:34 UTC (permalink / raw)
To: Maxime Ripard
Cc: syzbot, davidgow, dmitry.torokhov, gregkh, linux-input,
linux-kernel, rydberg, syzkaller-bugs, benjamin.tissoires
In-Reply-To: <878ra3m5my.fsf@nvidia.com>
On Tue, 22 Aug, 2023 08:57:41 -0700 Rahul Rameshbabu <rrameshbabu@nvidia.com> wrote:
> Hi Maxime,
>
> On Tue, 22 Aug, 2023 11:12:28 +0200 Maxime Ripard <mripard@kernel.org> wrote:
>> Hi,
>>
>> So, we discussed it this morning with Benjamin, and I think the culprit
>> is that the uclogic driver will allocate a char array with devm_kzalloc
>> in uclogic_input_configured()
>> (https://elixir.bootlin.com/linux/latest/source/drivers/hid/hid-uclogic-core.c#L149),
>> and will assign input_dev->name to that pointer.
>>
>> When the device is removed, the devm-allocated array is freed, and the
>> input framework will send a uevent in input_dev_uevent() using the
>> input_dev->name field:
>>
>> https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1688
>>
>> So it's a classic dangling pointer situation.
>>
>> And even though it was revealed by that patch, I think the issue is
>> unrelated. The fundamental issue seems to be that the usage of devm in
>> that situation is wrong.
>>
>> input_dev->name is accessed by input_dev_uevent, which for KOBJ_UNBIND
>> and KOBJ_REMOVE will be called after remove.
>>
>> For example, in __device_release_driver() (with the driver remove hook
>> being called in device_remove() and devres_release_all() being called in
>> device_unbind_cleanup()):
>> https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L1278
>>
>> So, it looks to me that, with or without the patch we merged recently,
>> the core has always sent uevent after device-managed resources were
>> freed. Thus, the uclogic (and any other input driver) was wrong in
>> allocating its input_dev name with devm_kzalloc (or the phys and uniq
>> fields in that struct).
>>
>> Note that freeing input_dev->name in remove would have been just as bad.
>>
>> Looking at the code quickly, at least hid-playstation,
>> hid-nvidia-shield, hid-logitech-hidpp, mms114 and tsc200x seem to be
>> affected by the same issue.
>
> I agree with this analysis overall. At least in hid-nvidia-shield, I can
> not use devm for allocating the input name string and explicitly free it
> after calling input_unregister_device. In this scenario, the name string
> would have been freed explicitly after input_put_device was called
> (since the input device is not devres managed). input_put_device would
> drop the reference count to zero and the device would be cleaned up at
> that point triggering KOBJ_REMOVE and firing off that final
> input_dev_uevent.
>
> I think this can be done for a number of the drivers as a workaround
> till this issue is properly resolved. If this seems appropriate, I can
> send out a series later in the day. This is just a workaround till the
> discussion below converges (which I am interested in).
After thinking about it a bit further, I believe hid-nvidia-shield is
not impacted by this issue in its current state. hid_hw_stop will
trigger devres_release_all, which will free the input name allocated.
This is problematic in the case that the input_dev is devres managed,
since device resource management will hook the uevent callback on
device_del triggered by input_unregister_device invoked by hid_hw_stop.
However, hid-nvidia-shield does not use devm to allocate the input
device and explicitly calls input_unregister_device before the call to
hid_hw_stop. I believe this leads to the uevent being fired before name
is freed by devres after the hid_hw_stop call. Let me know if this
analysis seems off. Hoping this can be helpful in general if this is the
case.
Freed by task 4508:
kasan_save_stack+0x33/0x50 mm/kasan/common.c:45
kasan_set_track+0x25/0x30 mm/kasan/common.c:52
kasan_save_free_info+0x2b/0x40 mm/kasan/generic.c:522
____kasan_slab_free mm/kasan/common.c:236 [inline]
____kasan_slab_free+0x15b/0x1b0 mm/kasan/common.c:200
kasan_slab_free include/linux/kasan.h:164 [inline]
slab_free_hook mm/slub.c:1800 [inline]
slab_free_freelist_hook+0x114/0x1e0 mm/slub.c:1826
slab_free mm/slub.c:3809 [inline]
__kmem_cache_free+0xb8/0x2f0 mm/slub.c:3822
release_nodes drivers/base/devres.c:506 [inline]
devres_release_all+0x192/0x240 drivers/base/devres.c:535
device_del+0x628/0xa50 drivers/base/core.c:3827
input_unregister_device+0xb9/0x100 drivers/input/input.c:2440
hidinput_disconnect+0x160/0x3e0 drivers/hid/hid-input.c:2386
hid_disconnect+0x143/0x1b0 drivers/hid/hid-core.c:2273
hid_hw_stop+0x16/0x70 drivers/hid/hid-core.c:2322
uclogic_remove+0x47/0x90 drivers/hid/hid-uclogic-core.c:485
>
>>
>> We discussed a couple of solutions with Benjamin, such as creating a
>> helper devm action to free and clear the input_dev->name field, droping
>> the name, phys and uniq fields from the uevent, or converting name, phys
>> and uniq to char arrays so drivers don't have to allocate them.
>>
>> We couldn't find a perfect one though, so... yeah.
>>
>> Maxime
>
--
Thanks,
Rahul Rameshbabu
^ permalink raw reply
* Re; Interest,
From: Chen Yun @ 2023-08-22 23:31 UTC (permalink / raw)
To: linux-input
Re; Interest,
I am interested in discussing the Investment proposal as I explained
in my previous mail. May you let me know your interest and the
possibility of a cooperation aimed for mutual interest.
Looking forward to your mail for further discussion.
Regards
------
Chen Yun - Chairman of CREC
China Railway Engineering Corporation - CRECG
China Railway Plaza, No.69 Fuxing Road, Haidian District, Beijing, P.R.
China
^ permalink raw reply
* Re: [syzbot] [input?] KASAN: slab-use-after-free Read in input_dev_uevent
From: Maxime Ripard @ 2023-08-23 7:44 UTC (permalink / raw)
To: Rahul Rameshbabu
Cc: syzbot, davidgow, dmitry.torokhov, gregkh, linux-input,
linux-kernel, rydberg, syzkaller-bugs, benjamin.tissoires
In-Reply-To: <878ra3m5my.fsf@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 3415 bytes --]
Hi Rahul,
On Tue, Aug 22, 2023 at 08:57:41AM -0700, Rahul Rameshbabu wrote:
> On Tue, 22 Aug, 2023 11:12:28 +0200 Maxime Ripard <mripard@kernel.org> wrote:
> > Hi,
> >
> > So, we discussed it this morning with Benjamin, and I think the culprit
> > is that the uclogic driver will allocate a char array with devm_kzalloc
> > in uclogic_input_configured()
> > (https://elixir.bootlin.com/linux/latest/source/drivers/hid/hid-uclogic-core.c#L149),
> > and will assign input_dev->name to that pointer.
> >
> > When the device is removed, the devm-allocated array is freed, and the
> > input framework will send a uevent in input_dev_uevent() using the
> > input_dev->name field:
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1688
> >
> > So it's a classic dangling pointer situation.
> >
> > And even though it was revealed by that patch, I think the issue is
> > unrelated. The fundamental issue seems to be that the usage of devm in
> > that situation is wrong.
> >
> > input_dev->name is accessed by input_dev_uevent, which for KOBJ_UNBIND
> > and KOBJ_REMOVE will be called after remove.
> >
> > For example, in __device_release_driver() (with the driver remove hook
> > being called in device_remove() and devres_release_all() being called in
> > device_unbind_cleanup()):
> > https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L1278
> >
> > So, it looks to me that, with or without the patch we merged recently,
> > the core has always sent uevent after device-managed resources were
> > freed. Thus, the uclogic (and any other input driver) was wrong in
> > allocating its input_dev name with devm_kzalloc (or the phys and uniq
> > fields in that struct).
> >
> > Note that freeing input_dev->name in remove would have been just as bad.
> >
> > Looking at the code quickly, at least hid-playstation,
> > hid-nvidia-shield, hid-logitech-hidpp, mms114 and tsc200x seem to be
> > affected by the same issue.
>
> I agree with this analysis overall. At least in hid-nvidia-shield, I can
> not use devm for allocating the input name string and explicitly free it
> after calling input_unregister_device. In this scenario, the name string
> would have been freed explicitly after input_put_device was called
> (since the input device is not devres managed). input_put_device would
> drop the reference count to zero and the device would be cleaned up at
> that point triggering KOBJ_REMOVE and firing off that final
> input_dev_uevent.
>
> I think this can be done for a number of the drivers as a workaround
> till this issue is properly resolved. If this seems appropriate, I can
> send out a series later in the day. This is just a workaround till the
> discussion below converges (which I am interested in).
I'm sorry, I don't know the input framework well enough to understand
what you had in mind exactly. Could you send a patch with your
suggestion for the hid-nvidia-shield so we can discuss this further?
That being said, I think that the current design around name, phys and
uniq is fairly treacherous to drivers and we should aim for a solution
that prevents that issue from being possible at all.
I was inclined to go for a char array for each to get rid of the pointer
entirely, but Benjamin raised some concerns over the structure size so
it's probably not a great solution.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox