| Message ID | b21d35da33357b20ece39c7892f57084b94c017a.1645214686.git.thomas.lendacky@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: amd-xgbe: Replace kasprintf() with snprintf() for debugfs name | expand |
From: Tom Lendacky > Sent: 18 February 2022 20:05 > > It was reported that using kasprintf() produced a kernel warning as the > network interface name was being changed by udev rules at the same time > that the debugfs entry for the device was being created. What was the error? I'm guessing the length changed and that made kvasprintf() unhappy?? ... > - buf = kasprintf(GFP_KERNEL, "amd-xgbe-%s", pdata->netdev->name); > - if (!buf) > + ret = snprintf(buf, sizeof(buf), "%s%s", XGBE_DIR_PREFIX, > + pdata->netdev->name); You can do: snprintf(buf, sizeof buf, XGBE_DIR_PREFIX "%s", pdata->netdev->name) > + if (ret >= sizeof(buf)) > return; Unlike kasnprintf() where kmalloc() can fail, the simple snprintf() can't really overrun unless pdata->netdev->name isn't '\0' terminated. Even if it being changed while you look at it that shouldn't happen. Don't you need to synchronise this anyway? If the debugfs create and rename can happen at the same time then the rename can be requested before the create and you get the wrong name. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 2/18/22 15:01, David Laight wrote: > From: Tom Lendacky >> Sent: 18 February 2022 20:05 >> >> It was reported that using kasprintf() produced a kernel warning as the >> network interface name was being changed by udev rules at the same time >> that the debugfs entry for the device was being created. > > What was the error? > I'm guessing the length changed and that made kvasprintf() unhappy?? > > ... >> - buf = kasprintf(GFP_KERNEL, "amd-xgbe-%s", pdata->netdev->name); >> - if (!buf) >> + ret = snprintf(buf, sizeof(buf), "%s%s", XGBE_DIR_PREFIX, >> + pdata->netdev->name); > > You can do: > snprintf(buf, sizeof buf, XGBE_DIR_PREFIX "%s", pdata->netdev->name) > Thought about that, just decided on the double %s, though. >> + if (ret >= sizeof(buf)) >> return; > > Unlike kasnprintf() where kmalloc() can fail, the simple snprintf() > can't really overrun unless pdata->netdev->name isn't '\0' terminated. > Even if it being changed while you look at it that shouldn't happen. It's a safety check, it doesn't hurt anything. > > > Don't you need to synchronise this anyway? > > If the debugfs create and rename can happen at the same time then > the rename can be requested before the create and you get the wrong > name. I thought about introducing a mutex or semaphore, but thought it was overkill just for debugfs support and this small window. But if folks think it's really needed, it can be added. Thanks, Tom > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) >
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c b/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c index b0a6c96b6ef4..a6537f24dd79 100644 --- a/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c +++ b/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c @@ -121,6 +121,8 @@ #include "xgbe.h" #include "xgbe-common.h" +#define XGBE_DIR_PREFIX "amd-xgbe-" + static ssize_t xgbe_common_read(char __user *buffer, size_t count, loff_t *ppos, unsigned int value) { @@ -438,15 +440,17 @@ static const struct file_operations xi2c_reg_value_fops = { void xgbe_debugfs_init(struct xgbe_prv_data *pdata) { - char *buf; + char buf[sizeof(XGBE_DIR_PREFIX) + sizeof(pdata->netdev->name)]; + int ret; /* Set defaults */ pdata->debugfs_xgmac_reg = 0; pdata->debugfs_xpcs_mmd = 1; pdata->debugfs_xpcs_reg = 0; - buf = kasprintf(GFP_KERNEL, "amd-xgbe-%s", pdata->netdev->name); - if (!buf) + ret = snprintf(buf, sizeof(buf), "%s%s", XGBE_DIR_PREFIX, + pdata->netdev->name); + if (ret >= sizeof(buf)) return; pdata->xgbe_debugfs = debugfs_create_dir(buf, NULL); @@ -493,8 +497,6 @@ void xgbe_debugfs_init(struct xgbe_prv_data *pdata) pdata->xgbe_debugfs, &pdata->debugfs_an_cdr_track_early); } - - kfree(buf); } void xgbe_debugfs_exit(struct xgbe_prv_data *pdata) @@ -505,21 +507,20 @@ void xgbe_debugfs_exit(struct xgbe_prv_data *pdata) void xgbe_debugfs_rename(struct xgbe_prv_data *pdata) { - char *buf; + char buf[sizeof(XGBE_DIR_PREFIX) + sizeof(pdata->netdev->name)]; + int ret; if (!pdata->xgbe_debugfs) return; - buf = kasprintf(GFP_KERNEL, "amd-xgbe-%s", pdata->netdev->name); - if (!buf) + ret = snprintf(buf, sizeof(buf), "%s%s", XGBE_DIR_PREFIX, + pdata->netdev->name); + if (ret >= sizeof(buf)) return; if (!strcmp(pdata->xgbe_debugfs->d_name.name, buf)) - goto out; + return; debugfs_rename(pdata->xgbe_debugfs->d_parent, pdata->xgbe_debugfs, pdata->xgbe_debugfs->d_parent, buf); - -out: - kfree(buf); }
It was reported that using kasprintf() produced a kernel warning as the network interface name was being changed by udev rules at the same time that the debugfs entry for the device was being created. Kernel panic - not syncing: panic_on_warn set ... CPU: 1 PID: 2219 Comm: qemu-event Tainted: G O 5.4.134 #1 Hardware name: <redacted> Call Trace: dump_stack+0x50/0x63 panic+0x102/0x2d2 ? kvasprintf+0xb5/0xc0 __warn.cold+0x20/0x20 ? kvasprintf+0xb5/0xc0 report_bug+0xcc/0x100 do_error_trap+0xa3/0xc0 ? kvasprintf+0xb5/0xc0 do_invalid_op+0x37/0x40 ? kvasprintf+0xb5/0xc0 invalid_op+0x28/0x30 RIP: 0010:kvasprintf+0xb5/0xc0 Code: 28 00 00 00 75 28 48 83 c4 20 4c 89 e8 5d 41 5c 41 5d 41 5e 41 5f c3 4c 89 f1 89 c2 89 ee 48 c7 c7 d8 1e 0c a8 e8 b0 a5 3a 00 <0f> 0b eb c8 e8 92 cc cd ff 66 90 41 55 41 89 fd 41 54 49 89 d4 55 RSP: 0018:ffffa79f80e37c40 EFLAGS: 00010286 RAX: 0000000000000000 RBX: ffff9b71b633c7c0 RCX: 0000000000000000 RDX: 0000000000000001 RSI: ffffffffa8986566 RDI: 00000000ffffffff RBP: 000000000000000d R08: 0000004aafbb5f98 R09: 0000000000000046 R10: ffffffffa8986900 R11: 00000000a8986553 R12: ffffa79f80e37c90 R13: ffff9b71f0dcdba0 R14: ffffffffc03c0e1a R15: 000000000000000e kasprintf+0x4e/0x70 ? timecounter_init+0x20/0x50 xgbe_debugfs_init+0x39/0x200 [amd_xgbe] xgbe_config_netdev+0x390/0x450 [amd_xgbe] xgbe_pci_probe+0x374/0x620 [amd_xgbe] local_pci_probe+0x26/0x50 pci_device_probe+0x107/0x1a0 really_probe+0x147/0x3b0 ? driver_allows_async_probing+0x50/0x50 bus_for_each_drv+0x7e/0xc0 __device_attach+0xd6/0x130 bus_rescan_devices_helper+0x35/0x80 drivers_probe_store+0x31/0x60 kernfs_fop_write+0xce/0x1b0 vfs_write+0xb6/0x1a0 ksys_write+0x5f/0xe0 do_syscall_64+0x55/0x1c0 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x7fa72e73bd7f Code: 89 54 24 18 48 89 74 24 10 89 7c 24 08 e8 b9 7b f9 ff 48 8b 54 24 18 48 8b 74 24 10 41 89 c0 8b 7c 24 08 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 2d 44 89 c7 48 89 44 24 08 e8 ec 7b f9 ff 48 RSP: 002b:00007fa6de7fba10 EFLAGS: 00000293 ORIG_RAX: 0000000000000001 RAX: ffffffffffffffda RBX: 000000000000000c RCX: 00007fa72e73bd7f RDX: 000000000000000c RSI: 00007fa72803cf60 RDI: 000000000000001c RBP: 00007fa72803cf60 R08: 0000000000000000 R09: 0000000000000003 R10: 0000000000000000 R11: 0000000000000293 R12: 000000000000001c R13: 000000000000001c R14: 0000000000000000 R15: 00007fa72ef0a9e8 Kernel Offset: 0x26200000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff) Replace the use of kasprintf() with snprintf() using a local buffer to prevent this situation. It is still possible for the device name to be changed while the debugfs entry is being created, but that will be handled by xgbe_debugfs_rename() function. Fixes: c5aa9e3b8156 ("amd-xgbe: Initial AMD 10GbE platform driver") Reported-by: Anthony Pighin <anthony.pighin@nokia.com> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> --- Please queue to stable: - As the warning is only produced at v4.5 and above, no need to go back further than that. - This patch will generate conflicts prior to the v5.4 stable tree that should be easy to resolve. But, if not, I'll take care of it when I see the emails. --- drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c | 25 ++++++++++---------- 1 file changed, 13 insertions(+), 12 deletions(-)