Re: KASAN: invalid-free in p9_client_create (2)
在 2021/1/28 3:31, Linus Torvalds 写道:
[ Participants list changed - syzbot thought this was networking and
p9, but it really looks entirely like a slub internal bug. I left the
innocent people on the list just to let them know they are innocent ]
On Wed, Jan 27, 2021 at 6:27 AM syzbot
wrote:
The issue was bisected to:
commit dde3c6b72a16c2db826f54b2d49bdea26c3534a2
Author: Wang Hai
Date: Wed Jun 3 22:56:21 2020 +
mm/slub: fix a memory leak in sysfs_slab_add()
BUG: KASAN: double-free or invalid-free in slab_free mm/slub.c:3142 [inline]
BUG: KASAN: double-free or invalid-free in kmem_cache_free+0x82/0x350
mm/slub.c:3158
The p9 part of this bug report seems to be a red herring.
The problem looks like it's simply the kmem_cache failure case, ie:
- mm/slab_common.c: create_cache(): if the __kmem_cache_create()
fails, it does:
out_free_cache:
kmem_cache_free(kmem_cache, s);
- but __kmem_cache_create() - at least for slub() - will have done
sysfs_slab_add(s) .. fails ..
-> kobject_del(>kobj); .. which frees s ...
so the networking and p9 are fine, and the only reason p9 shows up in
the trace is that apparently it causes that failure in
kobject_init_and_add() for whatever reason, and that then exposes the
problem.
So the added kobject_put() really looks buggy in this situation, and
the memory leak that that commit dde3c6b72a16 ("mm/slub: fix a memory
leak in sysfs_slab_add()") fixes is now a double free.
And no, I don't think you can just remove the kmem_cache_free() in
create_cache(), because _other_ error cases of __kmem_cache_create()
do not free this.
Wang Hai - comments? I'm inclined to revert that commit for now unless
somebody can come up with a proper fix..
Linus
.
Hi, Linus.
I'm sorry for introducing this bug, and I agree to revert it.
I've just sent the revert patch.
https://lore.kernel.org/patchwork/patch/1372475/
Thanks,
Wang Hai
Re: [PATCH] staging: greybus: audio: Add missing unlock in gbaudio_dapm_free_controls()
在 2020/12/5 2:02, Vaibhav Agarwal 写道:
On Fri, Dec 4, 2020 at 2:10 PM Johan Hovold wrote:
On Fri, Dec 04, 2020 at 10:13:50AM +0800, Wang Hai wrote:
Add the missing unlock before return from function
gbaudio_dapm_free_controls() in the error handling case.
Fixes: 510e340efe0c ("staging: greybus: audio: Add helper APIs for dynamic audio
module")
Reported-by: Hulk Robot
Signed-off-by: Wang Hai
---
drivers/staging/greybus/audio_helper.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/staging/greybus/audio_helper.c
b/drivers/staging/greybus/audio_helper.c
index 237531ba60f3..293675dbea10 100644
--- a/drivers/staging/greybus/audio_helper.c
+++ b/drivers/staging/greybus/audio_helper.c
@@ -135,6 +135,7 @@ int gbaudio_dapm_free_controls(struct snd_soc_dapm_context
*dapm,
if (!w) {
dev_err(dapm->dev, "%s: widget not found\n",
widget->name);
+ mutex_unlock(>card->dapm_mutex);
return -EINVAL;
}
widget++;
This superficially looks correct, but there seems to be another bug in
this function. It can be used free an array of widgets, but if one of
them isn't found we just leak the rest. Perhaps that return should
rather be "widget++; continue;".
Vaibhav?
Thanks Wang for sharing the patch. As already pointed by Johan, this
function indeed has another bug as well.
Pls feel free to share the patch as suggested above.
I just sent another patch
"[PATCH] staging: greybus: audio: Fix possible leak free widgets in
gbaudio_dapm_free_controls"
Johan
.
Re: [PATCH] staging: greybus: audio: Add missing unlock in gbaudio_dapm_free_controls()
在 2020/12/4 16:40, Johan Hovold 写道:
On Fri, Dec 04, 2020 at 10:13:50AM +0800, Wang Hai wrote:
Add the missing unlock before return from function
gbaudio_dapm_free_controls() in the error handling case.
Fixes: 510e340efe0c ("staging: greybus: audio: Add helper APIs for dynamic audio
module")
Reported-by: Hulk Robot
Signed-off-by: Wang Hai
---
drivers/staging/greybus/audio_helper.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/staging/greybus/audio_helper.c
b/drivers/staging/greybus/audio_helper.c
index 237531ba60f3..293675dbea10 100644
--- a/drivers/staging/greybus/audio_helper.c
+++ b/drivers/staging/greybus/audio_helper.c
@@ -135,6 +135,7 @@ int gbaudio_dapm_free_controls(struct snd_soc_dapm_context
*dapm,
if (!w) {
dev_err(dapm->dev, "%s: widget not found\n",
widget->name);
+ mutex_unlock(>card->dapm_mutex);
return -EINVAL;
}
widget++;
This superficially looks correct, but there seems to be another bug in
this function. It can be used free an array of widgets, but if one of
them isn't found we just leak the rest. Perhaps that return should
rather be "widget++; continue;".
I think this is a good idea, should I send a v2 patch?
Re: [PATCH net] net: bridge: Fix a warning when del bridge sysfs
在 2020/12/3 18:34, Nikolay Aleksandrov 写道:
On 03/12/2020 03:03, Jakub Kicinski wrote:
On Tue, 1 Dec 2020 22:01:14 +0800 Wang Hai wrote:
If adding bridge sysfs fails, br->ifobj will be NULL, there is no
need to delete its non-existent sysfs when deleting the bridge device,
otherwise, it will cause a warning. So, when br->ifobj == NULL,
directly return can fix this bug.
br_sysfs_addbr: can't create group bridge4/bridge
[ cut here ]
sysfs group 'bridge' not found for kobject 'bridge4'
WARNING: CPU: 2 PID: 9004 at fs/sysfs/group.c:279 sysfs_remove_group
fs/sysfs/group.c:279 [inline]
WARNING: CPU: 2 PID: 9004 at fs/sysfs/group.c:279
sysfs_remove_group+0x153/0x1b0 fs/sysfs/group.c:270
Modules linked in: iptable_nat
...
Call Trace:
br_dev_delete+0x112/0x190 net/bridge/br_if.c:384
br_dev_newlink net/bridge/br_netlink.c:1381 [inline]
br_dev_newlink+0xdb/0x100 net/bridge/br_netlink.c:1362
__rtnl_newlink+0xe11/0x13f0 net/core/rtnetlink.c:3441
rtnl_newlink+0x64/0xa0 net/core/rtnetlink.c:3500
rtnetlink_rcv_msg+0x385/0x980 net/core/rtnetlink.c:5562
netlink_rcv_skb+0x134/0x3d0 net/netlink/af_netlink.c:2494
netlink_unicast_kernel net/netlink/af_netlink.c:1304 [inline]
netlink_unicast+0x4a0/0x6a0 net/netlink/af_netlink.c:1330
netlink_sendmsg+0x793/0xc80 net/netlink/af_netlink.c:1919
sock_sendmsg_nosec net/socket.c:651 [inline]
sock_sendmsg+0x139/0x170 net/socket.c:671
sys_sendmsg+0x658/0x7d0 net/socket.c:2353
___sys_sendmsg+0xf8/0x170 net/socket.c:2407
__sys_sendmsg+0xd3/0x190 net/socket.c:2440
do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x44/0xa9
Reported-by: Hulk Robot
Signed-off-by: Wang Hai
Nik, is this the way you want to handle this?
Should the notifier not fail if sysfs files cannot be created?
Currently br_sysfs_addbr() returns an int but the only caller
ignores it.
Hi,
The fix is wrong because this is not the only user of ifobj. The bridge
port sysfs code also uses it and br_sysfs_addif() will create the new
symlink in sysfs_root_kn due to NULL kobj passed which basically means
only one port will be enslaved, the others will fail in creating their
sysfs entries and thus fail to be added as ports.
I'd prefer to just fail from the notifier based on the return value.
The only catch would be to test it with br_vlan_bridge_event() which
is called on bridge master device events, it should be fine as
br_vlan_find() deals with NULL vlan groups but at least a comment
above it in br.c's notifier would be good so if anyone decides to add
any NETDEVICE_UNREGISTER handling would be warned about it.
Thanks for your advice, I will perfect my patch
Also please add proper fixes tag, the bug seems to be since:
bb900b27a2f4 ("bridge: allow creating bridge devices with netlink")
It actually changed the behaviour, before that the return value of
br_sysfs_addbr()
was checked and the device got unregistered on failure.
Thanks,
Nik
.
Re: [PATCH net] ipv6: addrlabel: fix possible memory leak in ip6addrlbl_net_init
在 2020/11/24 9:22, Jakub Kicinski 写道: On Sun, 22 Nov 2020 10:34:56 +0800 Wang Hai wrote: kmemleak report a memory leak as follows: unreferenced object 0x8880059c6a00 (size 64): comm "ip", pid 23696, jiffies 4296590183 (age 1755.384s) hex dump (first 32 bytes): 20 01 00 10 00 00 00 00 00 00 00 00 00 00 00 00 ... 1c 00 00 00 00 00 00 00 00 00 00 00 07 00 00 00 backtrace: [] ip6addrlbl_add+0x90/0xbb0 [<70b8d7f1>] ip6addrlbl_net_init+0x109/0x170 [<6a9ca9d4>] ops_init+0xa8/0x3c0 [<2da57bf2>] setup_net+0x2de/0x7e0 [<4e52d573>] copy_net_ns+0x27d/0x530 [ ] create_new_namespaces+0x382/0xa30 [<3b76d36f>] unshare_nsproxy_namespaces+0xa1/0x1d0 [<30653721>] ksys_unshare+0x3a4/0x780 [<07e82e40>] __x64_sys_unshare+0x2d/0x40 [<31a10c08>] do_syscall_64+0x33/0x40 [<99df30e7>] entry_SYSCALL_64_after_hwframe+0x44/0xa9 We should free all rules when we catch an error in ip6addrlbl_net_init(). otherwise a memory leak will occur. Fixes: 2a8cc6c89039 ("[IPV6] ADDRCONF: Support RFC3484 configurable address selection policy table.") Reported-by: Hulk Robot Signed-off-by: Wang Hai We can simplify this function. diff --git a/net/ipv6/addrlabel.c b/net/ipv6/addrlabel.c index 642fc6ac13d2..637e323a0224 100644 --- a/net/ipv6/addrlabel.c +++ b/net/ipv6/addrlabel.c @@ -306,6 +306,8 @@ static int ip6addrlbl_del(struct net *net, /* add default label */ static int __net_init ip6addrlbl_net_init(struct net *net) { + struct ip6addrlbl_entry *p = NULL; + struct hlist_node *n; int err = 0; err does not need init int i; @@ -320,9 +322,17 @@ static int __net_init ip6addrlbl_net_init(struct net *net) instead of the temporary ret variable we can assign directly to err ip6addrlbl_init_table[i].prefixlen, 0, ip6addrlbl_init_table[i].label, 0); - /* XXX: should we free all rules when we catch an error? */ - if (ret && (!err || err != -ENOMEM)) + if (ret && (!err || err != -ENOMEM)) { this will become if (err) err = ret; + goto err_ip6addrlbl_add; + } + } + return err; return 0; +err_ip6addrlbl_add: + hlist_for_each_entry_safe(p, n, >ipv6.ip6addrlbl_table.head, list) { + hlist_del_rcu(>list); + kfree_rcu(p, rcu); } return err; } Thanks for advice. I just sent a v2 “[PATCH net v2] ipv6: addrlabel: fix possible memory leak in ip6addrlbl_net_init” .
Re: [PATCH net] ipvs: fix possible memory leak in ip_vs_control_net_init
在 2020/11/20 2:22, Julian Anastasov 写道:
Hello,
On Thu, 19 Nov 2020, Wang Hai wrote:
kmemleak report a memory leak as follows:
BUG: memory leak
unreferenced object 0x8880759ea000 (size 256):
comm "syz-executor.3", pid 6484, jiffies 4297476946 (age 48.546s)
hex dump (first 32 bytes):
00 00 00 00 01 00 00 00 08 a0 9e 75 80 88 ff ff ...u
[...]
Reported-by: Hulk Robot
Signed-off-by: Wang Hai
---
net/netfilter/ipvs/ip_vs_ctl.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
index e279ded4e306..d99bb89e7c25 100644
--- a/net/netfilter/ipvs/ip_vs_ctl.c
+++ b/net/netfilter/ipvs/ip_vs_ctl.c
@@ -4180,6 +4180,9 @@ int __net_init ip_vs_control_net_init(struct netns_ipvs
*ipvs)
return 0;
May be we should add some #ifdef CONFIG_PROC_FS because
proc_create_net* return NULL when PROC is not used. For example:
#ifdef CONFIG_PROC_FS
if (!proc_create_net...
goto err_vs;
if (!proc_create_net...
goto err_stats;
...
#endif
...
err:
#ifdef CONFIG_PROC_FS
+ remove_proc_entry("ip_vs_stats_percpu", ipvs->net->proc_net);
err_percpu:
+ remove_proc_entry("ip_vs_stats", ipvs->net->proc_net);
err_stats:
+ remove_proc_entry("ip_vs", ipvs->net->proc_net);
err_vs:
#endif
free_percpu(ipvs->tot_stats.cpustats);
return -ENOMEM;
}
--
Regards
--
Julian Anastasov
.
Thanks for your advice, I just sent v2
“[PATCH net v2] ipvs: fix possible memory leak in ip_vs_control_net_init”
Re: [PATCH] android/ion: fix error return code in opensocket()
There is a syntax problem with this patch, please ignore it!
在 2020/11/18 18:39, Wang Hai 写道:
Fix to return a negative error code from the error handling
case instead of 0, as done elsewhere in this function.
Fixes: 47a18c42d992 ("android/ion: userspace test utility for ion buffer
sharing")
Reported-by: Hulk Robot
Signed-off-by: Wang Hai
---
tools/testing/selftests/android/ion/ipcsocket.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/android/ion/ipcsocket.c
b/tools/testing/selftests/android/ion/ipcsocket.c
index 7dc521002095..268e6b610357 100644
--- a/tools/testing/selftests/android/ion/ipcsocket.c
+++ b/tools/testing/selftests/android/ion/ipcsocket.c
@@ -28,8 +28,9 @@ int opensocket(int *sockfd, const char *name, int connecttype)
}
*sockfd = ret;
- if (setsockopt(*sockfd, SOL_SOCKET, SO_REUSEADDR,
- (char *), sizeof(int)) < 0) {
+ ret = setsockopt(*sockfd, SOL_SOCKET, SO_REUSEADDR, (char *),
+sizeof(int))
+ if (ret < 0) {
fprintf(stderr, "<%s>: Failed setsockopt: <%s>\n",
__func__, strerror(errno));
goto err;
Re: [PATCH] PCI: dwc: fix error return code in dw_pcie_host_init()
在 2020/11/17 9:49, Jisheng Zhang 写道:
On Mon, 16 Nov 2020 21:50:23 +0800 Wang Hai wrote:
Fix to return a negative error code from the error handling
case instead of 0, as done elsewhere in this function.
good catch.
Fixes: 07940c369a6b ("PCI: dwc: Fix MSI page leakage in suspend/resume")
Reported-by: Hulk Robot
Signed-off-by: Wang Hai
if (dma_mapping_error(pci->dev, pp->msi_data)) {
dev_err(pci->dev, "Failed to map MSI data\n");
pp->msi_data = 0;
+ ret = -ENOMEM;
what about use the return value of dma_maping_error()? I.E
ret = dma_mapping_error()
if (ret) {
}
Thanks for your review, I just sent v2
"[PATCH v2] PCI: dwc: fix error return code in dw_pcie_host_init()"
.
Re: [PATCH net] devlink: Add missing genlmsg_cancel() in devlink_nl_sb_port_pool_fill()
在 2020/11/13 1:51, Jakub Kicinski 写道: On Wed, 11 Nov 2020 21:58:53 +0800 Wang Hai wrote: If sb_occ_port_pool_get() failed in devlink_nl_sb_port_pool_fill(), msg should be canceled by genlmsg_cancel(). +++ b/net/core/devlink.c @@ -1447,8 +1447,10 @@ static int devlink_nl_sb_port_pool_fill(struct sk_buff *msg, [...] return err; I guess the driver would have to return -EMSGSIZE for this to matter, which is quite unlikely but we should indeed fix. Still, returning in the middle of the function with an epilogue is what got use here in the first place, so please use a goto. E.g. like this: [...] static int devlink_nl_cmd_sb_port_pool_get_doit(struct sk_buff *skb, . Thanks for your review, I just sent v2 [PATCH v2 net] devlink: Add missing genlmsg_cancel() in devlink_nl_sb_port_pool_fill()
Re: [PATCH v2 bpf] tools: bpftool: Add missing close before bpftool net attach exit
在 2020/11/10 12:33, John Fastabend 写道:
Wang Hai wrote:
progfd is created by prog_parse_fd(), before 'bpftool net attach' exit,
it should be closed.
Fixes: 04949ccc273e ("tools: bpftool: add net attach command to attach XDP on
interface")
Signed-off-by: Wang Hai
---
v1->v2: use cleanup tag instead of repeated closes
tools/bpf/bpftool/net.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
index 910e7bac6e9e..1ac7228167e6 100644
--- a/tools/bpf/bpftool/net.c
+++ b/tools/bpf/bpftool/net.c
@@ -578,8 +578,8 @@ static int do_attach(int argc, char **argv)
ifindex = net_parse_dev(, );
if (ifindex < 1) {
- close(progfd);
- return -EINVAL;
+ err = -EINVAL;
+ goto cleanup;
}
if (argc) {
@@ -587,8 +587,8 @@ static int do_attach(int argc, char **argv)
overwrite = true;
} else {
p_err("expected 'overwrite', got: '%s'?", *argv);
- close(progfd);
- return -EINVAL;
+ err = -EINVAL;
+ goto cleanup;
}
}
@@ -600,13 +600,15 @@ static int do_attach(int argc, char **argv)
I think now that return value depends on this err it should be 'if (err)'
otherwise we risk retunring non-zero error code from do_attach which
will cause programs to fail.
I agree with you. Thanks.
if (err < 0) {
if (err) {
p_err("interface %s attach failed: %s",
attach_type_strings[attach_type], strerror(-err));
- return err;
+ goto cleanup;
}
if (json_output)
jsonw_null(json_wtr);
- return 0;
Alternatively we could add an 'err = 0' here, but above should never
return a value >0 as far as I can see.
It's true that 'err > 0' doesn't exist currently , but adding 'err = 0'
would make the code clearer. Thanks for your advice.
+cleanup:
+ close(progfd);
+ return err;
}
static int do_detach(int argc, char **argv)
--
2.17.1
Can it be fixed like this?
--- a/tools/bpf/bpftool/net.c
+++ b/tools/bpf/bpftool/net.c
@@ -578,8 +578,8 @@ static int do_attach(int argc, char **argv)
ifindex = net_parse_dev(, );
if (ifindex < 1) {
- close(progfd);
- return -EINVAL;
+ err = -EINVAL;
+ goto cleanup;
}
if (argc) {
@@ -587,8 +587,8 @@ static int do_attach(int argc, char **argv)
overwrite = true;
} else {
p_err("expected 'overwrite', got: '%s'?", *argv);
- close(progfd);
- return -EINVAL;
+ err = -EINVAL;
+ goto cleanup;
}
}
@@ -597,16 +597,19 @@ static int do_attach(int argc, char **argv)
err = do_attach_detach_xdp(progfd, attach_type, ifindex,
overwrite);
- if (err < 0) {
+ if (err) {
p_err("interface %s attach failed: %s",
attach_type_strings[attach_type], strerror(-err));
- return err;
+ goto cleanup;
}
if (json_output)
jsonw_null(json_wtr);
- return 0;
+ ret = 0;
+cleanup:
+ close(progfd);
+ return err;
}
.
Re: [PATCH bpf] tools: bpftool: Add missing close before bpftool net attach exit
在 2020/11/9 18:51, Michal Rostecki 写道:
On 11/9/20 8:04 AM, Wang Hai wrote:
progfd is created by prog_parse_fd(), before 'bpftool net attach' exit,
it should be closed.
Fixes: 04949ccc273e ("tools: bpftool: add net attach command to
attach XDP on interface")
Signed-off-by: Wang Hai
---
tools/bpf/bpftool/net.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
index 910e7bac6e9e..3e9b40e64fb0 100644
--- a/tools/bpf/bpftool/net.c
+++ b/tools/bpf/bpftool/net.c
@@ -600,12 +600,14 @@ static int do_attach(int argc, char **argv)
if (err < 0) {
p_err("interface %s attach failed: %s",
attach_type_strings[attach_type], strerror(-err));
+ close(progfd);
return err;
}
if (json_output)
jsonw_null(json_wtr);
+ close(progfd);
return 0;
}
Nit - wouldn't it be better to create a `cleanup`/`out` section before
return and use goto, to avoid copying the `close` call?
.
Thanks for review. I just sent v2 patch
"[PATCH v2 bpf] tools: bpftool: Add missing close before bpftool net
attach exit"
Re: [PATCH net-next 0/3] Fix some kernel-doc warnings for e1000/e1000e
在 2020/9/11 3:38, Jakub Kicinski 写道: On Thu, 10 Sep 2020 12:38:00 -0700 Jakub Kicinski wrote: On Thu, 10 Sep 2020 23:04:26 +0800 Wang Hai wrote: Wang Hai (3): e1000e: Fix some kernel-doc warnings in ich8lan.c e1000e: Fix some kernel-doc warnings in netdev.c e1000: Fix a bunch of kerneldoc parameter issues in e1000_hw.c You should put some text here but I can confirm this set removes 17 warnings. Reviewed-by: Jakub Kicinski . Thans for your review, I'll add some description next time
Re: [PATCH net-next] net/packet: Remove unused macro BLOCK_PRIV
在 2020/9/4 21:26, Willem de Bruijn 写道: On Fri, Sep 4, 2020 at 3:09 PM Wang Hai wrote: BPDU_TYPE_TCN is never used after it was introduced. So better to remove it. This comment does not cover the patch contents. Otherwise the patch looks good to me. Thanks for your review, I will revise this comment. Reported-by: Hulk Robot Signed-off-by: Wang Hai --- net/packet/af_packet.c | 1 - 1 file changed, 1 deletion(-) diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index da8254e680f9..c430672c6a67 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -177,7 +177,6 @@ static int packet_set_ring(struct sock *sk, union tpacket_req_u *req_u, #define BLOCK_LEN(x) ((x)->hdr.bh1.blk_len) #define BLOCK_SNUM(x) ((x)->hdr.bh1.seq_num) #define BLOCK_O2PRIV(x)((x)->offset_to_priv) -#define BLOCK_PRIV(x) ((void *)((char *)(x) + BLOCK_O2PRIV(x))) struct packet_sock; static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, -- 2.17.1 .
Re: [PATCH net] net: gemini: Fix missing free_netdev() in error path of gemini_ethernet_port_probe()
在 2020/8/19 3:54, David Miller 写道:
From: Wang Hai
Date: Tue, 18 Aug 2020 21:44:04 +0800
Fix the missing free_netdev() before return from
gemini_ethernet_port_probe() in the error handling case.
Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Reported-by: Hulk Robot
Signed-off-by: Wang Hai
You can just convert this function to use 'devm_alloc_etherdev_mqs', which
is a one line fix.
.
Thanks for your advice, I just sent a v2 patch.
"[PATCH net v2] net: gemini: Fix missing free_netdev() in error path of
gemini_ethernet_port_probe()"
Re: [PATCH net] net: qcom/emac: Fix missing clk_disable_unprepare() in error path of emac_probe
在 2020/8/7 21:38, Timur Tabi 写道: On 8/6/20 8:54 PM, wanghai (M) wrote: Thanks for your suggestion. May I fix it like this? Yes, this is what I had in mind. Thanks. Acked-by: Timur Tabi . Thanks for your ack. I just sent a new patch. "[PATCH net] net: qcom/emac: add missed clk_disable_unprepare in error path of emac_clks_phase1_init"
Re: [PATCH net] net: qcom/emac: Fix missing clk_disable_unprepare() in error path of emac_probe
在 2020/8/6 22:23, Timur Tabi 写道: On 8/6/20 9:06 AM, Wang Hai wrote: In emac_clks_phase1_init() of emac_probe(), there may be a situation in which some clk_prepare_enable() succeed and others fail. If emac_clks_phase1_init() fails, goto err_undo_clocks to clean up the clk that was successfully clk_prepare_enable(). Good catch, however, I think the proper fix is to fix this in emac_clks_phase1_init(), so that if some clocks fail, the other clocks are cleaned up and then an error is returned. . Thanks for your suggestion. May I fix it like this? diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c index 7520c02eec12..7977ad02a7c6 100644 --- a/drivers/net/ethernet/qualcomm/emac/emac.c +++ b/drivers/net/ethernet/qualcomm/emac/emac.c @@ -474,13 +474,25 @@ static int emac_clks_phase1_init(struct platform_device *pdev, ret = clk_prepare_enable(adpt->clk[EMAC_CLK_CFG_AHB]); if (ret) - return ret; + goto disable_clk_axi; ret = clk_set_rate(adpt->clk[EMAC_CLK_HIGH_SPEED], 1920); if (ret) - return ret; + goto disable_clk_cfg_ahb; - return clk_prepare_enable(adpt->clk[EMAC_CLK_HIGH_SPEED]); + ret = clk_prepare_enable(adpt->clk[EMAC_CLK_HIGH_SPEED]); + if (ret) + goto disable_clk_cfg_ahb; + + return 0; + +disable_clk_cfg_ahb: + clk_disable_unprepare(adpt->clk[EMAC_CLK_CFG_AHB]); +disable_clk_axi: + clk_disable_unprepare(adpt->clk[EMAC_CLK_AXI]); + + return ret; }
Re: [PATCH net-next] liquidio: Remove unneeded cast from memory allocation
在 2020/7/28 23:54, Joe Perches 写道:
On Tue, 2020-07-28 at 21:38 +0800, wanghai (M) wrote:
Thanks for your explanation. I got it.
Can it be modified like this?
[]
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
@@ -1152,11 +1152,8 @@ octeon_register_dispatch_fn(struct octeon_device
*oct,
dev_dbg(>pci_dev->dev,
"Adding opcode to dispatch list linked list\n");
- dispatch = (struct octeon_dispatch *)
- vmalloc(sizeof(struct octeon_dispatch));
+ dispatch = kmalloc(sizeof(struct octeon_dispatch),
GFP_KERNEL);
if (!dispatch) {
- dev_err(>pci_dev->dev,
- "No memory to add dispatch function\n");
return 1;
}
dispatch->opcode = combined_opcode;
Yes, but the free also needs to be changed.
I think it's:
---
drivers/net/ethernet/cavium/liquidio/octeon_device.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.c
b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
index 934115d18488..4ee4cb946e1d 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_device.c
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
@@ -1056,7 +1056,7 @@ void octeon_delete_dispatch_list(struct octeon_device
*oct)
list_for_each_safe(temp, tmp2, ) {
list_del(temp);
- vfree(temp);
+ kfree(temp);
}
}
@@ -1152,13 +1152,10 @@ octeon_register_dispatch_fn(struct octeon_device *oct,
dev_dbg(>pci_dev->dev,
"Adding opcode to dispatch list linked list\n");
- dispatch = (struct octeon_dispatch *)
- vmalloc(sizeof(struct octeon_dispatch));
- if (!dispatch) {
- dev_err(>pci_dev->dev,
- "No memory to add dispatch function\n");
+ dispatch = kmalloc(sizeof(struct octeon_dispatch), GFP_KERNEL);
+ if (!dispatch)
return 1;
- }
+
dispatch->opcode = combined_opcode;
dispatch->dispatch_fn = fn;
dispatch->arg = fn_arg;
.
Thanks for your suggestion. I just sent another patch for this.
"[PATCH net-next] liquidio: Replace vmalloc with kmalloc in
octeon_register_dispatch_fn()"
Re: [PATCH net-next] liquidio: Remove unneeded cast from memory allocation
在 2020/7/28 17:11, Joe Perches 写道:
On Tue, 2020-07-28 at 16:42 +0800, wanghai (M) wrote:
在 2020/7/25 5:29, Joe Perches 写道:
On Fri, 2020-07-24 at 21:00 +0800, Wang Hai wrote:
Remove casting the values returned by memory allocation function.
Coccinelle emits WARNING:
./drivers/net/ethernet/cavium/liquidio/octeon_device.c:1155:14-36: WARNING:
casting value returned by memory allocation function to (struct
octeon_dispatch *) is useless.
[]
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.c
b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
[]
@@ -1152,8 +1152,7 @@ octeon_register_dispatch_fn(struct octeon_device *oct,
dev_dbg(>pci_dev->dev,
"Adding opcode to dispatch list linked list\n");
- dispatch = (struct octeon_dispatch *)
- vmalloc(sizeof(struct octeon_dispatch));
+ dispatch = vmalloc(sizeof(struct octeon_dispatch));
More the question is why this is vmalloc at all
as the structure size is very small.
Likely this should just be kmalloc.
Thanks for your advice. It is indeed best to use kmalloc here.
if (!dispatch) {
dev_err(>pci_dev->dev,
"No memory to add dispatch function\n");
And this dev_err is unnecessary.
I don't understand why dev_err is not needed here. We can easily know
that an error has occurred here through dev_err
Memory allocation failures without __GFP_NOWARN. already
do a dump_stack to show the location of the code that
could not successfully allocate memory.
Thanks for your explanation. I got it.
Can it be modified like this?
--- a/drivers/net/ethernet/cavium/liquidio/octeon_device.c
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
@@ -1152,11 +1152,8 @@ octeon_register_dispatch_fn(struct octeon_device
*oct,
dev_dbg(>pci_dev->dev,
"Adding opcode to dispatch list linked list\n");
- dispatch = (struct octeon_dispatch *)
- vmalloc(sizeof(struct octeon_dispatch));
+ dispatch = kmalloc(sizeof(struct octeon_dispatch),
GFP_KERNEL);
if (!dispatch) {
- dev_err(>pci_dev->dev,
- "No memory to add dispatch function\n");
return 1;
}
dispatch->opcode = combined_opcode;
.
Re: [PATCH net-next] liquidio: Remove unneeded cast from memory allocation
在 2020/7/25 5:29, Joe Perches 写道:
On Fri, 2020-07-24 at 21:00 +0800, Wang Hai wrote:
Remove casting the values returned by memory allocation function.
Coccinelle emits WARNING:
./drivers/net/ethernet/cavium/liquidio/octeon_device.c:1155:14-36: WARNING:
casting value returned by memory allocation function to (struct
octeon_dispatch *) is useless.
[]
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.c
b/drivers/net/ethernet/cavium/liquidio/octeon_device.c
[]
@@ -1152,8 +1152,7 @@ octeon_register_dispatch_fn(struct octeon_device *oct,
dev_dbg(>pci_dev->dev,
"Adding opcode to dispatch list linked list\n");
- dispatch = (struct octeon_dispatch *)
- vmalloc(sizeof(struct octeon_dispatch));
+ dispatch = vmalloc(sizeof(struct octeon_dispatch));
More the question is why this is vmalloc at all
as the structure size is very small.
Likely this should just be kmalloc.
Thanks for your advice. It is indeed best to use kmalloc here.
if (!dispatch) {
dev_err(>pci_dev->dev,
"No memory to add dispatch function\n");
And this dev_err is unnecessary.
I don't understand why dev_err is not needed here. We can easily know
that an error has occurred here through dev_err
.
Re: [PATCH net-next v2] net: ena: Fix using plain integer as NULL pointer in ena_init_napi_in_range
在 2020/7/20 11:15, Joe Perches 写道:
On Mon, 2020-07-20 at 10:53 +0800, Wang Hai wrote:
Fix sparse build warning:
drivers/net/ethernet/amazon/ena/ena_netdev.c:2193:34: warning:
Using plain integer as NULL pointer
[]
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
b/drivers/net/ethernet/amazon/ena/ena_netdev.c
[]
@@ -2190,11 +2190,10 @@ static void ena_del_napi_in_range(struct ena_adapter
*adapter,
static void ena_init_napi_in_range(struct ena_adapter *adapter,
int first_index, int count)
{
- struct ena_napi *napi = {0};
int i;
for (i = first_index; i < first_index + count; i++) {
- napi = >ena_napi[i];
+ struct ena_napi *napi = >ena_napi[i];
netif_napi_add(adapter->netdev,
>ena_napi[i].napi,
Another possible change is to this statement:
netif_napi_add(adapter->netdev,
>napi,
etc...);
Good catch. I'll add this change, Thanks.
.
Re: [PATCH -next] net: ena: use NULL instead of zero
在 2020/7/18 23:06, Joe Perches 写道: On Sat, 2020-07-18 at 19:56 +0800, Wang Hai wrote: Fix sparse build warning: drivers/net/ethernet/amazon/ena/ena_netdev.c:2193:34: warning: Using plain integer as NULL pointer Better to remove the initialization altogether and move the declaration into the loop. Thanks for your advice. I'll send a v2 patch.
Re: [PATCH] net: dsa: felix: Make some symbols static
Thanks for reminding me, I'll do it.
在 2020/7/18 18:40, Vladimir Oltean 写道:
On Sat, Jul 18, 2020 at 06:01:58PM +0800, Wang Hai wrote:
Fix sparse build warning:
drivers/net/dsa/ocelot/felix_vsc9959.c:560:19: warning:
symbol 'vsc9959_vcap_is2_keys' was not declared. Should it be static?
drivers/net/dsa/ocelot/felix_vsc9959.c:640:19: warning:
symbol 'vsc9959_vcap_is2_actions' was not declared. Should it be static?
Reported-by: Hulk Robot
Signed-off-by: Wang Hai
---
Please update your tree.
commit 3ab4ceb6e9639e4e42d473e46ae7976c24187876
Author: Vladimir Oltean
Date: Sat Jun 20 18:43:36 2020 +0300
net: dsa: felix: make vcap is2 keys and actions static
Get rid of some sparse warnings.
Signed-off-by: Vladimir Oltean
Signed-off-by: David S. Miller
drivers/net/dsa/ocelot/felix_vsc9959.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c
b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 1dd9e348152d..2067776773f7 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -557,7 +557,7 @@ static const struct ocelot_stat_layout
vsc9959_stats_layout[] = {
{ .offset = 0x111, .name = "drop_green_prio_7", },
};
-struct vcap_field vsc9959_vcap_is2_keys[] = {
+static struct vcap_field vsc9959_vcap_is2_keys[] = {
/* Common: 41 bits */
[VCAP_IS2_TYPE] = { 0, 4},
[VCAP_IS2_HK_FIRST] = { 4, 1},
@@ -637,7 +637,7 @@ struct vcap_field vsc9959_vcap_is2_keys[] = {
[VCAP_IS2_HK_OAM_IS_Y1731] = {182, 1},
};
-struct vcap_field vsc9959_vcap_is2_actions[] = {
+static struct vcap_field vsc9959_vcap_is2_actions[] = {
[VCAP_IS2_ACT_HIT_ME_ONCE] = { 0, 1},
[VCAP_IS2_ACT_CPU_COPY_ENA] = { 1, 1},
[VCAP_IS2_ACT_CPU_QU_NUM] = { 2, 3},
--
2.17.1
Thanks,
-Vladimir
.
Re: [PATCH] net: cxgb3: add missed destroy_workqueue in cxgb3 probe failure
在 2020/7/18 9:39, David Miller 写道:
From: Wang Hai
Date: Fri, 17 Jul 2020 14:21:17 +0800
The driver forgets to call destroy_workqueue when cxgb3 probe fails.
Add the missed calls to fix it.
Fixes: 4d22de3e6cc4 ("Add support for the latest 1G/10G Chelsio adapter, T3.")
Reported-by: Hulk Robot
Signed-off-by: Wang Hai
You have to handle the case that the probing of one or more devices
fails yet one or more others succeed.
And you can't know in advance how this will play out.
This is why the workqueue is unconditionally created, and only destroyed
on module remove.
.
Thanks for your explanation. I got it.
Re: [PATCH] net: cxgb3: add missed destroy_workqueue in nci_register_device
subject msg was wrong. "net: cxgb3:" should be "nfc: nci:". v2 patch
has been sent.
("[PATCH v2] nfc: nci: add missed destroy_workqueue in nci_register_device")
在 2020/7/17 14:18, Wang Hai 写道:
When nfc_register_device fails in nci_register_device,
destroy_workqueue() shouled be called to destroy ndev->tx_wq.
Fixes: 3c1c0f5dc80b ("NFC: NCI: Fix nci_register_device init sequence")
Reported-by: Hulk Robot
Signed-off-by: Wang Hai
---
net/nfc/nci/core.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
index 7cd524884304..78ea8c94dcba 100644
--- a/net/nfc/nci/core.c
+++ b/net/nfc/nci/core.c
@@ -1228,10 +1228,13 @@ int nci_register_device(struct nci_dev *ndev)
rc = nfc_register_device(ndev->nfc_dev);
if (rc)
- goto destroy_rx_wq_exit;
+ goto destroy_tx_wq_exit;
goto exit;
+destroy_tx_wq_exit:
+ destroy_workqueue(ndev->tx_wq);
+
destroy_rx_wq_exit:
destroy_workqueue(ndev->rx_wq);
Re: [PATCH v2] 9p/trans_fd: Fix concurrency del of req_list in p9_fd_cancelled/p9_read_work
在 2020/6/12 17:10, Dominique Martinet 写道:
Wang Hai wrote on Fri, Jun 12, 2020:
p9_read_work and p9_fd_cancelled may be called concurrently.
In some cases, req->req_list may be deleted by both p9_read_work
and p9_fd_cancelled.
We can fix it by ignoring replies associated with a cancelled
request and ignoring cancelled request if message has been received
before lock.
Fixes: 60ff779c4abb ("9p: client: remove unused code and any reference to
"cancelled" function")
Reported-by: syzbot+77a25acfa0382e06a...@syzkaller.appspotmail.com
Signed-off-by: Wang Hai
Thanks! looks good to me, I'll queue for 5.9 as well unless you're in a
hurry.
Ok, thanks.
Re: [PATCH] cxl: Fix kobject memleak
在 2020/6/3 19:33, Andrew Donnellan 写道:
On 2/6/20 10:07 pm, Wang Hai wrote:
Currently the error return path from kobject_init_and_add() is not
followed by a call to kobject_put() - which means we are leaking
the kobject.
Fix it by adding a call to kobject_put() in the error path of
kobject_init_and_add().
Fixes: b087e6190ddc ("cxl: Export optional AFU configuration record
in sysfs")
Reported-by: Hulk Robot
Signed-off-by: Wang Hai
Thanks for the fix!
I note that the err1 label returns without calling kfree(cr) and I
can't see a reason why we do that - so perhaps we should remove the
return statement in err1: so it falls through?
kfree(cr) can be called when
kobject_put()-->kobject_release()-->kobject_cleanup()-->kobj_type->release()
is called. The kobj_type here is afu_config_record_type
Thanks,
Wang Hai
Re: [PATCH] cxl: Fix kobject memory leak in cxl_sysfs_afu_new_cr()
在 2020/6/3 14:50, Greg Kroah-Hartman 写道: On Wed, Jun 03, 2020 at 02:34:07PM +0800, wanghai (M) wrote: 在 2020/6/3 14:14, Greg Kroah-Hartman 写道: On Wed, Jun 03, 2020 at 09:42:41AM +0800, wanghai (M) wrote: 在 2020/6/3 1:20, Markus Elfring 写道: Fix it by adding a call to kobject_put() in the error path of kobject_init_and_add(). Thanks for another completion of the exception handling. Would an other patch subject be a bit nicer? Thanks for the guidance, I will perfect this description and send a v2 Please note that you are responding to someone that a lot of kernel developers and maintainers have blacklisted as being very annoying and not helpful at all. Please do not feel that you need to respond to, or change any patch in response to their emails at all. I strongly recommend you just add them to your filters to not have to see their messages. That's what I have done. thanks, greg k-h . Okay, so I don’t have to send the v2 patch. No, all should be fine, I'll review the patch when after 5.8-rc1 is out, and if I find any problems with it, will let you know then. Got it. Thanks. thanks, Wang Hai
Re: [PATCH] cxl: Fix kobject memory leak in cxl_sysfs_afu_new_cr()
在 2020/6/3 14:14, Greg Kroah-Hartman 写道: On Wed, Jun 03, 2020 at 09:42:41AM +0800, wanghai (M) wrote: 在 2020/6/3 1:20, Markus Elfring 写道: Fix it by adding a call to kobject_put() in the error path of kobject_init_and_add(). Thanks for another completion of the exception handling. Would an other patch subject be a bit nicer? Thanks for the guidance, I will perfect this description and send a v2 Please note that you are responding to someone that a lot of kernel developers and maintainers have blacklisted as being very annoying and not helpful at all. Please do not feel that you need to respond to, or change any patch in response to their emails at all. I strongly recommend you just add them to your filters to not have to see their messages. That's what I have done. thanks, greg k-h . Okay, so I don’t have to send the v2 patch. -- thanks, Wang Hai
Re: [PATCH] cxl: Fix kobject memory leak in cxl_sysfs_afu_new_cr()
在 2020/6/3 1:20, Markus Elfring 写道: Fix it by adding a call to kobject_put() in the error path of kobject_init_and_add(). Thanks for another completion of the exception handling. Would an other patch subject be a bit nicer? Thanks for the guidance, I will perfect this description and send a v2 … +++ b/drivers/misc/cxl/sysfs.c @@ -624,7 +624,7 @@ static struct afu_config_record *cxl_sysfs_afu_new_cr(struct cxl_afu *afu, int c rc = kobject_init_and_add(>kobj, _config_record_type, >dev.kobj, "cr%i", cr->cr); if (rc) - goto err; + goto err1; … Can an other label be more reasonable here? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=f359287765c04711ff54fbd11645271d8e5ff763#n465 I just used the original author's label, should I replace all his labels like'err','err1' with reasonable one.
