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 <[email protected]>
---
drivers/firewire/core-device.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index 0547253d16fe..47d6cb3dc916 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -323,7 +323,7 @@ static ssize_t show_immediate(struct device *dev,
if (value < 0)
return -ENOENT;
- return snprintf(buf, buf ? PAGE_SIZE : 0, "0x%06x\n", value);
+ return snprintf(buf, PAGE_SIZE, "0x%06x\n", value);
}
#define IMMEDIATE_ATTR(name, key) \
@@ -335,8 +335,6 @@ static ssize_t show_text_leaf(struct device *dev,
struct config_rom_attribute *attr =
container_of(dattr, struct config_rom_attribute, attr);
const u32 *directories[] = {NULL, NULL};
- size_t bufsize;
- char dummy_buf[2];
int i, ret = -ENOENT;
down_read(&fw_device_rwsem);
@@ -358,15 +356,9 @@ static ssize_t show_text_leaf(struct device *dev,
}
}
- if (buf) {
- bufsize = PAGE_SIZE - 1;
- } else {
- buf = dummy_buf;
- bufsize = 1;
- }
-
for (i = 0; i < ARRAY_SIZE(directories) && !!directories[i]; ++i) {
- int result = fw_csr_string(directories[i], attr->key, buf, bufsize);
+ int result = fw_csr_string(directories[i], attr->key, buf,
+ PAGE_SIZE - 1);
// Detected.
if (result >= 0)
ret = result;
--
2.29.2
Per filesystems/sysfs.rst, show() should only use sysfs_emit()
or sysfs_emit_at() when formatting the value to be returned to user space.
coccinelle complains that there are still a couple of functions that use
snprintf(). Convert them to sysfs_emit().
> drivers/firewire/core-device.c:326:8-16: WARNING: please use sysfs_emit or sysfs_emit_at
No functional change intended
Signed-off-by: Li Zhijian <[email protected]>
---
"[PATCH 00/42] Fix coccicheck warnings"[1] is trying to fix all warnings
reported by coccicheck. firewire introduce newly abused snprintf case.
[1] https://lore.kernel.org/lkml/[email protected]/
---
drivers/firewire/core-device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index 47d6cb3dc916..790985479ff3 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -323,7 +323,7 @@ static ssize_t show_immediate(struct device *dev,
if (value < 0)
return -ENOENT;
- return snprintf(buf, PAGE_SIZE, "0x%06x\n", value);
+ return sysfs_emit(buf, "0x%06x\n", value);
}
#define IMMEDIATE_ATTR(name, key) \
@@ -474,7 +474,7 @@ static ssize_t is_local_show(struct device *dev,
{
struct fw_device *device = fw_device(dev);
- return sprintf(buf, "%u\n", device->is_local);
+ return sysfs_emit(buf, "%u\n", device->is_local);
}
static int units_sprintf(char *buf, const u32 *directory)
--
2.29.2
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
On 18/03/2024 12:46, Takashi Sakamoto wrote:
> 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 <[email protected]>
>>> ---
>>> 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...
>
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.)
Thanks
Zhijian
>
> Regards
>
> Takashi Sakamoto
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 <[email protected]>
---
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
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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Li Zhijian <[email protected]>
> ---
> 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