Message ID | 20221022044847.61995-2-shaozhengchao@huawei.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | fix some issues in netdevsim driver | expand |
Context | Check | Description |
---|---|---|
netdev/tree_selection | success | Clearly marked for net |
netdev/fixes_present | success | Fixes tag present in non-next series |
netdev/subject_prefix | success | Link |
netdev/cover_letter | success | Series has a cover letter |
netdev/patch_count | success | Link |
netdev/header_inline | success | No static functions without inline keyword in header files |
netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
netdev/module_param | success | Was 0 now: 0 |
netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
netdev/check_selftest | success | No net selftest shell script |
netdev/verify_fixes | success | Fixes tag looks correct |
netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 63 lines checked |
netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
netdev/source_inline | success | Was 0 now: 0 |
On Sat, 2022-10-22 at 12:48 +0800, Zhengchao Shao wrote: > If some items in nsim_dev_resources_register() fail, memory leak will > occur. The following is the memory leak information. > > unreferenced object 0xffff888074c02600 (size 128): > comm "echo", pid 8159, jiffies 4294945184 (age 493.530s) > hex dump (first 32 bytes): > 40 47 ea 89 ff ff ff ff 01 00 00 00 00 00 00 00 @G.............. > ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................ > backtrace: > [<0000000011a31c98>] kmalloc_trace+0x22/0x60 > [<0000000027384c69>] devl_resource_register+0x144/0x4e0 > [<00000000a16db248>] nsim_drv_probe+0x37a/0x1260 > [<000000007d1f448c>] really_probe+0x20b/0xb10 > [<00000000c416848a>] __driver_probe_device+0x1b3/0x4a0 > [<00000000077e0351>] driver_probe_device+0x49/0x140 > [<0000000054f2465a>] __device_attach_driver+0x18c/0x2a0 > [<000000008538f359>] bus_for_each_drv+0x151/0x1d0 > [<0000000038e09747>] __device_attach+0x1c9/0x4e0 > [<00000000dd86e533>] bus_probe_device+0x1d5/0x280 > [<00000000839bea35>] device_add+0xae0/0x1cb0 > [<000000009c2abf46>] new_device_store+0x3b6/0x5f0 > [<00000000fb823d7f>] bus_attr_store+0x72/0xa0 > [<000000007acc4295>] sysfs_kf_write+0x106/0x160 > [<000000005f50cb4d>] kernfs_fop_write_iter+0x3a8/0x5a0 > [<0000000075eb41bf>] vfs_write+0x8f0/0xc80 > > Fixes: 37923ed6b8ce ("netdevsim: Add simple FIB resource controller via devlink") > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > --- > drivers/net/netdevsim/dev.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c > index 794fc0cc73b8..07b1a3b3afaf 100644 > --- a/drivers/net/netdevsim/dev.c > +++ b/drivers/net/netdevsim/dev.c > @@ -442,7 +442,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) > ¶ms); > if (err) { > pr_err("Failed to register IPv4 top resource\n"); > - goto out; > + goto err_out; > } > > err = devl_resource_register(devlink, "fib", (u64)-1, > @@ -450,7 +450,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) > NSIM_RESOURCE_IPV4, ¶ms); > if (err) { > pr_err("Failed to register IPv4 FIB resource\n"); > - return err; > + goto err_out; > } > > err = devl_resource_register(devlink, "fib-rules", (u64)-1, > @@ -458,7 +458,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) > NSIM_RESOURCE_IPV4, ¶ms); > if (err) { > pr_err("Failed to register IPv4 FIB rules resource\n"); > - return err; > + goto err_out; > } > > /* Resources for IPv6 */ > @@ -468,7 +468,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) > ¶ms); > if (err) { > pr_err("Failed to register IPv6 top resource\n"); > - goto out; > + goto err_out; > } > > err = devl_resource_register(devlink, "fib", (u64)-1, > @@ -476,7 +476,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) > NSIM_RESOURCE_IPV6, ¶ms); > if (err) { > pr_err("Failed to register IPv6 FIB resource\n"); > - return err; > + goto err_out; > } > > err = devl_resource_register(devlink, "fib-rules", (u64)-1, > @@ -484,7 +484,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) > NSIM_RESOURCE_IPV6, ¶ms); > if (err) { > pr_err("Failed to register IPv6 FIB rules resource\n"); > - return err; > + goto err_out; > } > > /* Resources for nexthops */ > @@ -492,8 +492,14 @@ static int nsim_dev_resources_register(struct devlink *devlink) > NSIM_RESOURCE_NEXTHOPS, > DEVLINK_RESOURCE_ID_PARENT_TOP, > ¶ms); > + if (err) { > + pr_err("Failed to register NEXTHOPS resource\n"); > + goto err_out; > + } > + return err; Nit pick: I think it would be more clear if here you do: return 0; The above is actually almost an excuse to ask you to repost with a better/more describing cover letter, as the current one is practically empty. Thanks! Paolo
On 2022/10/25 19:24, Paolo Abeni wrote: > On Sat, 2022-10-22 at 12:48 +0800, Zhengchao Shao wrote: >> If some items in nsim_dev_resources_register() fail, memory leak will >> occur. The following is the memory leak information. >> >> unreferenced object 0xffff888074c02600 (size 128): >> comm "echo", pid 8159, jiffies 4294945184 (age 493.530s) >> hex dump (first 32 bytes): >> 40 47 ea 89 ff ff ff ff 01 00 00 00 00 00 00 00 @G.............. >> ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................ >> backtrace: >> [<0000000011a31c98>] kmalloc_trace+0x22/0x60 >> [<0000000027384c69>] devl_resource_register+0x144/0x4e0 >> [<00000000a16db248>] nsim_drv_probe+0x37a/0x1260 >> [<000000007d1f448c>] really_probe+0x20b/0xb10 >> [<00000000c416848a>] __driver_probe_device+0x1b3/0x4a0 >> [<00000000077e0351>] driver_probe_device+0x49/0x140 >> [<0000000054f2465a>] __device_attach_driver+0x18c/0x2a0 >> [<000000008538f359>] bus_for_each_drv+0x151/0x1d0 >> [<0000000038e09747>] __device_attach+0x1c9/0x4e0 >> [<00000000dd86e533>] bus_probe_device+0x1d5/0x280 >> [<00000000839bea35>] device_add+0xae0/0x1cb0 >> [<000000009c2abf46>] new_device_store+0x3b6/0x5f0 >> [<00000000fb823d7f>] bus_attr_store+0x72/0xa0 >> [<000000007acc4295>] sysfs_kf_write+0x106/0x160 >> [<000000005f50cb4d>] kernfs_fop_write_iter+0x3a8/0x5a0 >> [<0000000075eb41bf>] vfs_write+0x8f0/0xc80 >> >> Fixes: 37923ed6b8ce ("netdevsim: Add simple FIB resource controller via devlink") >> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> >> --- >> drivers/net/netdevsim/dev.c | 20 +++++++++++++------- >> 1 file changed, 13 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c >> index 794fc0cc73b8..07b1a3b3afaf 100644 >> --- a/drivers/net/netdevsim/dev.c >> +++ b/drivers/net/netdevsim/dev.c >> @@ -442,7 +442,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) >> ¶ms); >> if (err) { >> pr_err("Failed to register IPv4 top resource\n"); >> - goto out; >> + goto err_out; >> } >> >> err = devl_resource_register(devlink, "fib", (u64)-1, >> @@ -450,7 +450,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) >> NSIM_RESOURCE_IPV4, ¶ms); >> if (err) { >> pr_err("Failed to register IPv4 FIB resource\n"); >> - return err; >> + goto err_out; >> } >> >> err = devl_resource_register(devlink, "fib-rules", (u64)-1, >> @@ -458,7 +458,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) >> NSIM_RESOURCE_IPV4, ¶ms); >> if (err) { >> pr_err("Failed to register IPv4 FIB rules resource\n"); >> - return err; >> + goto err_out; >> } >> >> /* Resources for IPv6 */ >> @@ -468,7 +468,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) >> ¶ms); >> if (err) { >> pr_err("Failed to register IPv6 top resource\n"); >> - goto out; >> + goto err_out; >> } >> >> err = devl_resource_register(devlink, "fib", (u64)-1, >> @@ -476,7 +476,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) >> NSIM_RESOURCE_IPV6, ¶ms); >> if (err) { >> pr_err("Failed to register IPv6 FIB resource\n"); >> - return err; >> + goto err_out; >> } >> >> err = devl_resource_register(devlink, "fib-rules", (u64)-1, >> @@ -484,7 +484,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) >> NSIM_RESOURCE_IPV6, ¶ms); >> if (err) { >> pr_err("Failed to register IPv6 FIB rules resource\n"); >> - return err; >> + goto err_out; >> } >> >> /* Resources for nexthops */ >> @@ -492,8 +492,14 @@ static int nsim_dev_resources_register(struct devlink *devlink) >> NSIM_RESOURCE_NEXTHOPS, >> DEVLINK_RESOURCE_ID_PARENT_TOP, >> ¶ms); >> + if (err) { >> + pr_err("Failed to register NEXTHOPS resource\n"); >> + goto err_out; >> + } >> + return err; > > Nit pick: I think it would be more clear if here you do: > > return 0; > > The above is actually almost an excuse to ask you to repost with a > better/more describing cover letter, as the current one is practically > empty. > > Thanks! > > Paolo > Hi Paolo: Thank you for your reveiw. I will change it in v3. Zhengchao Shao
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c index 794fc0cc73b8..07b1a3b3afaf 100644 --- a/drivers/net/netdevsim/dev.c +++ b/drivers/net/netdevsim/dev.c @@ -442,7 +442,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) ¶ms); if (err) { pr_err("Failed to register IPv4 top resource\n"); - goto out; + goto err_out; } err = devl_resource_register(devlink, "fib", (u64)-1, @@ -450,7 +450,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) NSIM_RESOURCE_IPV4, ¶ms); if (err) { pr_err("Failed to register IPv4 FIB resource\n"); - return err; + goto err_out; } err = devl_resource_register(devlink, "fib-rules", (u64)-1, @@ -458,7 +458,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) NSIM_RESOURCE_IPV4, ¶ms); if (err) { pr_err("Failed to register IPv4 FIB rules resource\n"); - return err; + goto err_out; } /* Resources for IPv6 */ @@ -468,7 +468,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) ¶ms); if (err) { pr_err("Failed to register IPv6 top resource\n"); - goto out; + goto err_out; } err = devl_resource_register(devlink, "fib", (u64)-1, @@ -476,7 +476,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) NSIM_RESOURCE_IPV6, ¶ms); if (err) { pr_err("Failed to register IPv6 FIB resource\n"); - return err; + goto err_out; } err = devl_resource_register(devlink, "fib-rules", (u64)-1, @@ -484,7 +484,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) NSIM_RESOURCE_IPV6, ¶ms); if (err) { pr_err("Failed to register IPv6 FIB rules resource\n"); - return err; + goto err_out; } /* Resources for nexthops */ @@ -492,8 +492,14 @@ static int nsim_dev_resources_register(struct devlink *devlink) NSIM_RESOURCE_NEXTHOPS, DEVLINK_RESOURCE_ID_PARENT_TOP, ¶ms); + if (err) { + pr_err("Failed to register NEXTHOPS resource\n"); + goto err_out; + } + return err; -out: +err_out: + devl_resources_unregister(devlink); return err; }
If some items in nsim_dev_resources_register() fail, memory leak will occur. The following is the memory leak information. unreferenced object 0xffff888074c02600 (size 128): comm "echo", pid 8159, jiffies 4294945184 (age 493.530s) hex dump (first 32 bytes): 40 47 ea 89 ff ff ff ff 01 00 00 00 00 00 00 00 @G.............. ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................ backtrace: [<0000000011a31c98>] kmalloc_trace+0x22/0x60 [<0000000027384c69>] devl_resource_register+0x144/0x4e0 [<00000000a16db248>] nsim_drv_probe+0x37a/0x1260 [<000000007d1f448c>] really_probe+0x20b/0xb10 [<00000000c416848a>] __driver_probe_device+0x1b3/0x4a0 [<00000000077e0351>] driver_probe_device+0x49/0x140 [<0000000054f2465a>] __device_attach_driver+0x18c/0x2a0 [<000000008538f359>] bus_for_each_drv+0x151/0x1d0 [<0000000038e09747>] __device_attach+0x1c9/0x4e0 [<00000000dd86e533>] bus_probe_device+0x1d5/0x280 [<00000000839bea35>] device_add+0xae0/0x1cb0 [<000000009c2abf46>] new_device_store+0x3b6/0x5f0 [<00000000fb823d7f>] bus_attr_store+0x72/0xa0 [<000000007acc4295>] sysfs_kf_write+0x106/0x160 [<000000005f50cb4d>] kernfs_fop_write_iter+0x3a8/0x5a0 [<0000000075eb41bf>] vfs_write+0x8f0/0xc80 Fixes: 37923ed6b8ce ("netdevsim: Add simple FIB resource controller via devlink") Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> --- drivers/net/netdevsim/dev.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)