Thread: Re: [PATCH 1/2] firewire: Kill unnecessary buf check in device_attribute.show
Brought to you by:
aeb,
bencollins
From: Takashi S. <o-t...@sa...> - 2024-01-22 08:56:20
|
Hi, On Mon, Jan 22, 2024 at 01:39:41PM +0800, Li Zhijian wrote: > Per Documentation/filesystems/sysfs.rst: > > sysfs allocates a buffer of size (PAGE_SIZE) and passes it to the > > method. > > So we can kill the unnecessary buf check safely. > > Signed-off-by: Li Zhijian <liz...@fu...> > --- > drivers/firewire/core-device.c | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) Applied both patches to linux-next branch, since they are not so-urgent fixes. Thanks Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-03-18 09:23:31
|
Hi, On Mon, Mar 18, 2024 at 06:24:52AM +0000, Zhijian Li (Fujitsu) wrote: > ... > Sorry for the mistake. I haven't considered callers from other than sysfs. > I'm fine to reverting both *two*. > > If we are interesting in the sysfs_emit conversion one, i cooked(see the attachment) > a patch to revert "firewire: Kill unnecessary buf check in device_attribute.show" only. > > (Feel free to ignore it if you have had a local fix.) > ... > From 96ad3e15b86e2504f3c17fd6a10be48e5ff81cb1 Mon Sep 17 00:00:00 2001 > From: Li Zhijian <liz...@fu...> > Date: Mon, 18 Mar 2024 14:05:32 +0800 > Subject: [PATCH] Revert "firewire: Kill unnecessary buf check in > device_attribute.show" > > This reverts commit 4a2b06ca33763b363038d333274e212db6ff0de1. > > The previous fix didn't consider callers from other than sysfs. Revert > it to fix the NULL dereference > > kernel: ? sysfs_emit+0xb5/0xc0 > kernel: show_immediate+0x13f/0x1d0 [firewire_core] > kernel: init_fw_attribute_group+0x81/0x150 [firewire_core] > kernel: create_units+0x119/0x160 [firewire_core] > kernel: fw_device_init+0x1a9/0x330 [firewire_core] > kernel: fw_device_workfn+0x12/0x20 [firewire_core] > kernel: process_one_work+0x16f/0x350 > kernel: worker_thread+0x306/0x440 > kernel: ? __pfx_worker_thread+0x10/0x10 > kernel: kthread+0xf2/0x120 > kernel: ? __pfx_kthread+0x10/0x10 > kernel: ret_from_fork+0x47/0x70 > kernel: ? __pfx_kthread+0x10/0x10 > kernel: ret_from_fork_asm+0x1b/0x30 > kernel: </TASK> > kernel: ---[ end trace 0000000000000000 ]--- > kernel: ------------[ cut here ]------------ > > Fixes: 4a2b06ca3376 ("firewire: Kill unnecessary buf check in device_attribute.show") > Reported-by: Takashi Sakamoto <o-t...@sa...> > Signed-off-by: Li Zhijian <liz...@fu...> > --- > drivers/firewire/core-device.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) Thanks for your immediate action. I applied it to for-linus branch, and will send it in this week with my additional patch for notes. Let's back to test, hehe. Thanks Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-03-18 04:46:30
|
Hi, On Mon, Jan 22, 2024 at 05:56:04PM +0900, Takashi Sakamoto wrote: > Hi, > > On Mon, Jan 22, 2024 at 01:39:41PM +0800, Li Zhijian wrote: > > Per Documentation/filesystems/sysfs.rst: > > > sysfs allocates a buffer of size (PAGE_SIZE) and passes it to the > > > method. > > > > So we can kill the unnecessary buf check safely. > > > > Signed-off-by: Li Zhijian <liz...@fu...> > > --- > > drivers/firewire/core-device.c | 14 +++----------- > > 1 file changed, 3 insertions(+), 11 deletions(-) > > Applied both patches to linux-next branch, since they are not so-urgent > fixes. I realized that it causes an issue at the path to initialize device structure for node in IEEE 1394 bus. (drivers/firewire/core-device.c) fw_device_init() / fw_device_refresh() ->create_units() ->init_fw_attribute_group() ->attr->show(dev, attr, NULL) kernel: ------------[ cut here ]------------ kernel: invalid sysfs_emit: buf:0000000000000000 kernel: WARNING: CPU: 5 PID: 647501 at fs/sysfs/file.c:747 sysfs_emit+0xb5/0xc0 kernel: Modules linked in: snd_fireworks(OE) snd_firewire_lib(OE) firewire_ohci(OE) firewire_core(OE) cfg80211 veth nft_masq crc_itu_t vfio_pci vfio_pci_core vfio_iommu_type1 vfio iommufd rpcsec_gss_krb5 nfsv4 nfs netfs snd_seq_dummy sn> kernel: crypto_simd asus_wmi snd_timer ledtrig_audio cryptd cec sparse_keymap nls_iso8859_1 rapl platform_profile wmi_bmof k10temp i2c_piix4 snd rc_core i2c_algo_bit ccp soundcore zfs(PO) spl(O) input_leds joydev cdc_mbim cdc_wdm mac_h> kernel: CPU: 5 PID: 647501 Comm: kworker/5:0 Tainted: P W OE 6.8.0-11-generic #11-Ubuntu kernel: Hardware name: System manufacturer System Product Name/TUF GAMING X570-PLUS, BIOS 5003 10/07/2023 kernel: Workqueue: firewire fw_device_workfn [firewire_core] kernel: RIP: 0010:sysfs_emit+0xb5/0xc0 kernel: Code: 25 28 00 00 00 75 29 c9 31 d2 31 c9 31 f6 31 ff 45 31 c0 45 31 c9 e9 5a 89 c7 00 48 89 fe 48 c7 c7 64 06 3f bd e8 1b 80 b4 ff <0f> 0b 31 c0 eb c7 e8 a0 ea c5 00 90 90 90 90 90 90 90 90 90 90 90 kernel: RSP: 0018:ffffacd857103cd0 EFLAGS: 00010246 kernel: RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 kernel: RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000 kernel: RBP: ffffacd857103d20 R08: 0000000000000000 R09: 0000000000000000 kernel: R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000010000 kernel: R13: ffffffffc28e2ff8 R14: 0000000000000000 R15: 0000000000000001 kernel: FS: 0000000000000000(0000) GS:ffff918190080000(0000) knlGS:0000000000000000 kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 kernel: CR2: 00007835dca0b048 CR3: 00000003cb898000 CR4: 00000000003506f0 kernel: Call Trace: kernel: <TASK> kernel: ? show_regs+0x6d/0x80 kernel: ? __warn+0x89/0x160 kernel: ? sysfs_emit+0xb5/0xc0 kernel: ? report_bug+0x17e/0x1b0 kernel: ? handle_bug+0x51/0xa0 kernel: ? exc_invalid_op+0x18/0x80 kernel: ? asm_exc_invalid_op+0x1b/0x20 kernel: ? sysfs_emit+0xb5/0xc0 kernel: show_immediate+0x13f/0x1d0 [firewire_core] kernel: init_fw_attribute_group+0x81/0x150 [firewire_core] kernel: create_units+0x119/0x160 [firewire_core] kernel: fw_device_init+0x1a9/0x330 [firewire_core] kernel: fw_device_workfn+0x12/0x20 [firewire_core] kernel: process_one_work+0x16f/0x350 kernel: worker_thread+0x306/0x440 kernel: ? __pfx_worker_thread+0x10/0x10 kernel: kthread+0xf2/0x120 kernel: ? __pfx_kthread+0x10/0x10 kernel: ret_from_fork+0x47/0x70 kernel: ? __pfx_kthread+0x10/0x10 kernel: ret_from_fork_asm+0x1b/0x30 kernel: </TASK> kernel: ---[ end trace 0000000000000000 ]--- kernel: ------------[ cut here ]------------ Furthermore, 'show_text_leaf()' returns negative value when the NULL pointer is passed. It results in the lack of vendor/model names from sysfs. I absolutely overlooked them. I would like to fix them within this merge window, or revert them as a last resort... Regards Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-03-18 09:18:18
|
In the case of firewire core function, the caller of show functions for device attributes is not only sysfs user, but also device initialization. This commit adds memo about it against the typical assumption that the functions are just dedicated to sysfs user. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/core-device.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c index f208a02d0ebf..a8172a6c2caa 100644 --- a/drivers/firewire/core-device.c +++ b/drivers/firewire/core-device.c @@ -322,6 +322,7 @@ static ssize_t show_immediate(struct device *dev, if (value < 0) return -ENOENT; + // Note that this function is called by init_fw_attribute_group() with NULL pointer. return buf ? sysfs_emit(buf, "0x%06x\n", value) : 0; } @@ -357,6 +358,7 @@ static ssize_t show_text_leaf(struct device *dev, } } + // Note that this function is called by init_fw_attribute_group() with NULL pointer. if (buf) { bufsize = PAGE_SIZE - 1; } else { -- 2.43.0 |