* [PATCH net-next 0/2] veth: a couple of fixes
@ 2022-11-17 23:33 Paolo Abeni
2022-11-17 23:33 ` [PATCH net-next 1/2] veth: fix uninitialized napi disable Paolo Abeni
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Paolo Abeni @ 2022-11-17 23:33 UTC (permalink / raw)
To: netdev; +Cc: Heng Qi, Xuan Zhuo, bpf
Recent changes in the veth driver caused a few regressions
this series addresses a couple of them, causing oops.
Paolo Abeni (2):
veth: fix uninitialized napi disable
veth: fix double napi enable
drivers/net/veth.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
--
2.38.1
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH net-next 1/2] veth: fix uninitialized napi disable 2022-11-17 23:33 [PATCH net-next 0/2] veth: a couple of fixes Paolo Abeni @ 2022-11-17 23:33 ` Paolo Abeni 2022-11-17 23:33 ` [PATCH net-next 2/2] veth: fix double napi enable Paolo Abeni 2022-11-18 8:41 ` [PATCH net-next 0/2] veth: a couple of fixes Paolo Abeni 2 siblings, 0 replies; 14+ messages in thread From: Paolo Abeni @ 2022-11-17 23:33 UTC (permalink / raw) To: netdev; +Cc: Heng Qi, Xuan Zhuo, bpf syzkaller reports a list corruption at veth close time: kernel BUG at lib/list_debug.c:56! invalid opcode: 0000 [#1] PREEMPT SMP KASAN CPU: 1 PID: 4247 Comm: syz-executor.4 Not tainted 6.1.0-rc4-syzkaller-01115-g68d268d08931 #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022 RIP: 0010:__list_del_entry_valid.cold+0x37/0x72 lib/list_debug.c:56 Code: e8 d2 62 f0 ff 0f 0b 48 89 ee 48 c7 c7 80 10 a8 8a e8 c1 62 f0 ff 0f 0b 4c 89 e2 48 89 ee 48 c7 c7 40 11 a8 8a e8 ad 62 f0 ff <0f> 0b 48 89 ee 48 c7 c7 20 10 a8 8a e8 9c 62 f0 ff 0f 0b 4c 89 ea RSP: 0018:ffffc9000a116ed0 EFLAGS: 00010282 RAX: 000000000000004e RBX: 0000000000000000 RCX: 0000000000000000 RDX: 0000000000040000 RSI: ffffffff8165772c RDI: fffff52001422dcc RBP: ffff88804eace160 R08: 000000000000004e R09: 0000000000000000 R10: 0000000080000000 R11: 0000000000000000 R12: dead000000000122 R13: ffff88804eab6050 R14: ffff88804eace000 R15: ffff88804eace000 FS: 00007fa8ab74e700(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 000000002040f030 CR3: 000000007efb8000 CR4: 00000000003506e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> __list_del_entry include/linux/list.h:134 [inline] list_del_rcu include/linux/rculist.h:157 [inline] __netif_napi_del.part.0+0x118/0x530 net/core/dev.c:6458 __netif_napi_del+0x40/0x50 net/core/dev.c:6454 veth_napi_del_range+0xcd/0x560 drivers/net/veth.c:1041 veth_napi_del drivers/net/veth.c:1055 [inline] veth_close+0x164/0x500 drivers/net/veth.c:1385 __dev_close_many+0x1b6/0x2e0 net/core/dev.c:1501 __dev_close net/core/dev.c:1513 [inline] __dev_change_flags+0x2ce/0x750 net/core/dev.c:8528 dev_change_flags+0x97/0x170 net/core/dev.c:8602 do_setlink+0x9f1/0x3bb0 net/core/rtnetlink.c:2827 rtnl_group_changelink net/core/rtnetlink.c:3344 [inline] __rtnl_newlink+0xb90/0x1840 net/core/rtnetlink.c:3600 rtnl_newlink+0x68/0xa0 net/core/rtnetlink.c:3637 rtnetlink_rcv_msg+0x43e/0xca0 net/core/rtnetlink.c:6141 netlink_rcv_skb+0x165/0x440 net/netlink/af_netlink.c:2564 netlink_unicast_kernel net/netlink/af_netlink.c:1330 [inline] netlink_unicast+0x547/0x7f0 net/netlink/af_netlink.c:1356 netlink_sendmsg+0x91b/0xe10 net/netlink/af_netlink.c:1932 sock_sendmsg_nosec net/socket.c:714 [inline] sock_sendmsg+0xd3/0x120 net/socket.c:734 ____sys_sendmsg+0x712/0x8c0 net/socket.c:2476 ___sys_sendmsg+0x110/0x1b0 net/socket.c:2530 __sys_sendmsg+0xf7/0x1c0 net/socket.c:2559 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x63/0xcd RIP: 0033:0x7fa8aaa8b639 Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48 RSP: 002b:00007fa8ab74e168 EFLAGS: 00000246 ORIG_RAX: 000000000000002e RAX: ffffffffffffffda RBX: 00007fa8aababf80 RCX: 00007fa8aaa8b639 RDX: 0000000000000000 RSI: 0000000020000140 RDI: 0000000000000003 RBP: 00007fa8aaae6ae9 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000 R13: 00007ffe720d839f R14: 00007fa8ab74e300 R15: 0000000000022000 </TASK> The issue can actually reproduced with: ip link add type veth ip link set dev veth0 up ip link set dev veth1 up ip link set dev veth0 xdp object <obj> ip link set dev veth0 down ip link set dev veth1 down The veth1 napi instances are deleted first when veth0 goes down, and when veth1 is shut down, veth_close() tries again to delete them, causing the oops. This patch addresses the issue explicitly checking that the napi instances are enabled before deleting them. Fixes: 2e0de6366ac1 ("veth: Avoid drop packets when xdp_redirect performs") Reported-by: syzbot+0cffe54ffe58ab9c059b@syzkaller.appspotmail.com Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- drivers/net/veth.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/net/veth.c b/drivers/net/veth.c index 2a4592780141..1384134f7100 100644 --- a/drivers/net/veth.c +++ b/drivers/net/veth.c @@ -1381,7 +1381,8 @@ static int veth_close(struct net_device *dev) if (!veth_gro_requested(peer) && !peer_priv->_xdp_prog) veth_napi_del(peer); } - } else if (veth_gro_requested(dev) || (peer && peer_priv->_xdp_prog)) { + } else if ((veth_gro_requested(dev) || (peer && peer_priv->_xdp_prog)) && + rtnl_dereference(priv->rq[0].napi)) { veth_napi_del(dev); } -- 2.38.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH net-next 2/2] veth: fix double napi enable 2022-11-17 23:33 [PATCH net-next 0/2] veth: a couple of fixes Paolo Abeni 2022-11-17 23:33 ` [PATCH net-next 1/2] veth: fix uninitialized napi disable Paolo Abeni @ 2022-11-17 23:33 ` Paolo Abeni 2022-11-18 14:24 ` Heng Qi 2022-11-18 8:41 ` [PATCH net-next 0/2] veth: a couple of fixes Paolo Abeni 2 siblings, 1 reply; 14+ messages in thread From: Paolo Abeni @ 2022-11-17 23:33 UTC (permalink / raw) To: netdev; +Cc: Heng Qi, Xuan Zhuo, bpf While investigating a related issue I stumbled upon another oops, reproducible as the follow: ip link add type veth ip link set dev veth0 xdp object <obj> ip link set dev veth0 up ip link set dev veth1 up The first link up command will enable the napi instances on veth1 and the second link up common will try again the same operation, causing the oops. This change addresses the issue explicitly checking the peer is up before enabling its napi instances. Fixes: 2e0de6366ac1 ("veth: Avoid drop packets when xdp_redirect performs") Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- drivers/net/veth.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/net/veth.c b/drivers/net/veth.c index 1384134f7100..d541183e0c66 100644 --- a/drivers/net/veth.c +++ b/drivers/net/veth.c @@ -1343,7 +1343,8 @@ static int veth_open(struct net_device *dev) if (err) return err; /* refer to the logic in veth_xdp_set() */ - if (!rtnl_dereference(peer_rq->napi)) { + if (!rtnl_dereference(peer_rq->napi) && + (peer->flags & IFF_UP)) { err = veth_napi_enable(peer); if (err) return err; -- 2.38.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 2/2] veth: fix double napi enable 2022-11-17 23:33 ` [PATCH net-next 2/2] veth: fix double napi enable Paolo Abeni @ 2022-11-18 14:24 ` Heng Qi 0 siblings, 0 replies; 14+ messages in thread From: Heng Qi @ 2022-11-18 14:24 UTC (permalink / raw) To: Paolo Abeni; +Cc: netdev, Xuan Zhuo, bpf > While investigating a related issue I stumbled upon another > oops, reproducible as the follow: > > ip link add type veth > ip link set dev veth0 xdp object <obj> > ip link set dev veth0 up > ip link set dev veth1 up > > The first link up command will enable the napi instances on > veth1 and the second link up common will try again the same > operation, causing the oops. > > This change addresses the issue explicitly checking the peer > is up before enabling its napi instances. > > Fixes: 2e0de6366ac1 ("veth: Avoid drop packets when xdp_redirect performs") > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > drivers/net/veth.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/veth.c b/drivers/net/veth.c > index 1384134f7100..d541183e0c66 100644 > --- a/drivers/net/veth.c > +++ b/drivers/net/veth.c > @@ -1343,7 +1343,8 @@ static int veth_open(struct net_device *dev) > if (err) > return err; > /* refer to the logic in veth_xdp_set() */ > - if (!rtnl_dereference(peer_rq->napi)) { > + if (!rtnl_dereference(peer_rq->napi) && > + (peer->flags & IFF_UP)) { > err = veth_napi_enable(peer); > if (err) > return err; I have checked the conditions related to enabling and disabling napi for veth pair. In general, we should check whether napi is disabled before enabling it, and check whether napi is enabled before disabling it. I am sorry that my previous test cases didn't do better, and we can work completely with your patchset. As the your patch in link below does https://lore.kernel.org/all/c59f4adcdd1296ee37cc6bca4d927b8c79158909.1668727939.git.pabeni@redhat.com/ Is this patch more uniform like the following: --- a/drivers/net/veth.c +++ b/drivers/net/veth.c @@ -1348,7 +1348,8 @@ static int veth_open(struct net_device *dev) if (err) return err; } - } else if (veth_gro_requested(dev) || peer_priv->_xdp_prog) { + } else if ((veth_gro_requested(dev) || peer_priv->_xdp_prog) && + !rtnl_dereference(priv->rq[0].napi)) { err = veth_napi_enable(dev); if (err) return err; Thanks. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 0/2] veth: a couple of fixes 2022-11-17 23:33 [PATCH net-next 0/2] veth: a couple of fixes Paolo Abeni 2022-11-17 23:33 ` [PATCH net-next 1/2] veth: fix uninitialized napi disable Paolo Abeni 2022-11-17 23:33 ` [PATCH net-next 2/2] veth: fix double napi enable Paolo Abeni @ 2022-11-18 8:41 ` Paolo Abeni 2022-11-18 11:05 ` Toke Høiland-Jørgensen 2022-11-21 3:33 ` Xuan Zhuo 2 siblings, 2 replies; 14+ messages in thread From: Paolo Abeni @ 2022-11-18 8:41 UTC (permalink / raw) To: netdev; +Cc: Heng Qi, Xuan Zhuo, bpf On Fri, 2022-11-18 at 00:33 +0100, Paolo Abeni wrote: > Recent changes in the veth driver caused a few regressions > this series addresses a couple of them, causing oops. > > Paolo Abeni (2): > veth: fix uninitialized napi disable > veth: fix double napi enable > > drivers/net/veth.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) @Xuan Zhuo: another option would be reverting 2e0de6366ac1 ("veth: Avoid drop packets when xdp_redirect performs") and its follow-up 5e5dc33d5dac ("bpf: veth driver panics when xdp prog attached before veth_open"). That option would be possibly safer, because I feel there are other issues with 2e0de6366ac1, and would offer the opportunity to refactor its logic a bit: the napi enable/disable condition is quite complex and not used consistently mixing and alternating the gro/xdp/peer xdp check with the napi ptr dereference. Ideally it would be better to have an helper alike napi_should_be_enabled(), use it everywhere, and pair the new code with some selftests, extending the existing ones. WDYT? Side notes: - Heng Qi address is bouncing - the existing veth self-tests would have caught the bug addressed here, if commit afef88e65554 ("selftests/bpf: Store BPF object files with .bpf.o extension") would not have broken them meanwhile :( /P ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 0/2] veth: a couple of fixes 2022-11-18 8:41 ` [PATCH net-next 0/2] veth: a couple of fixes Paolo Abeni @ 2022-11-18 11:05 ` Toke Høiland-Jørgensen 2022-11-19 1:06 ` Jakub Kicinski 2022-11-21 3:33 ` Xuan Zhuo 1 sibling, 1 reply; 14+ messages in thread From: Toke Høiland-Jørgensen @ 2022-11-18 11:05 UTC (permalink / raw) To: Paolo Abeni, netdev; +Cc: Heng Qi, Xuan Zhuo, bpf Paolo Abeni <pabeni@redhat.com> writes: > On Fri, 2022-11-18 at 00:33 +0100, Paolo Abeni wrote: >> Recent changes in the veth driver caused a few regressions >> this series addresses a couple of them, causing oops. >> >> Paolo Abeni (2): >> veth: fix uninitialized napi disable >> veth: fix double napi enable >> >> drivers/net/veth.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) > > @Xuan Zhuo: another option would be reverting 2e0de6366ac1 ("veth: > Avoid drop packets when xdp_redirect performs") and its follow-up > 5e5dc33d5dac ("bpf: veth driver panics when xdp prog attached before > veth_open"). > > That option would be possibly safer, because I feel there are other > issues with 2e0de6366ac1, and would offer the opportunity to refactor > its logic a bit: the napi enable/disable condition is quite complex and > not used consistently mixing and alternating the gro/xdp/peer xdp check > with the napi ptr dereference. > > Ideally it would be better to have an helper alike > napi_should_be_enabled(), use it everywhere, and pair the new code with > some selftests, extending the existing ones. > > WDYT? FWIW, the original commit 2e0de6366ac1 was merged very quickly without much review; so I'm not terribly surprised it breaks. I would personally be OK with just reverting it... -Toke ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 0/2] veth: a couple of fixes 2022-11-18 11:05 ` Toke Høiland-Jørgensen @ 2022-11-19 1:06 ` Jakub Kicinski 2022-11-21 6:15 ` John Fastabend 0 siblings, 1 reply; 14+ messages in thread From: Jakub Kicinski @ 2022-11-19 1:06 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Paolo Abeni, netdev, Heng Qi, Xuan Zhuo, bpf On Fri, 18 Nov 2022 12:05:39 +0100 Toke Høiland-Jørgensen wrote: > FWIW, the original commit 2e0de6366ac1 was merged very quickly without > much review; so I'm not terribly surprised it breaks. I would personally > be OK with just reverting it... +1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 0/2] veth: a couple of fixes 2022-11-19 1:06 ` Jakub Kicinski @ 2022-11-21 6:15 ` John Fastabend 0 siblings, 0 replies; 14+ messages in thread From: John Fastabend @ 2022-11-21 6:15 UTC (permalink / raw) To: Jakub Kicinski, Toke Høiland-Jørgensen Cc: Paolo Abeni, netdev, Heng Qi, Xuan Zhuo, bpf Jakub Kicinski wrote: > On Fri, 18 Nov 2022 12:05:39 +0100 Toke Høiland-Jørgensen wrote: > > FWIW, the original commit 2e0de6366ac1 was merged very quickly without > > much review; so I'm not terribly surprised it breaks. I would personally > > be OK with just reverting it... > > +1 Also fine with reverting my fix was mainly there to fix a CI test that was failing. Apparently my quick fix wasn't nearly good enough. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 0/2] veth: a couple of fixes 2022-11-18 8:41 ` [PATCH net-next 0/2] veth: a couple of fixes Paolo Abeni 2022-11-18 11:05 ` Toke Høiland-Jørgensen @ 2022-11-21 3:33 ` Xuan Zhuo 2022-11-21 3:55 ` Heng Qi 1 sibling, 1 reply; 14+ messages in thread From: Xuan Zhuo @ 2022-11-21 3:33 UTC (permalink / raw) To: Paolo Abeni; +Cc: Heng Qi, bpf, netdev On Fri, 18 Nov 2022 09:41:05 +0100, Paolo Abeni <pabeni@redhat.com> wrote: > On Fri, 2022-11-18 at 00:33 +0100, Paolo Abeni wrote: > > Recent changes in the veth driver caused a few regressions > > this series addresses a couple of them, causing oops. > > > > Paolo Abeni (2): > > veth: fix uninitialized napi disable > > veth: fix double napi enable > > > > drivers/net/veth.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > @Xuan Zhuo: another option would be reverting 2e0de6366ac1 ("veth: > Avoid drop packets when xdp_redirect performs") and its follow-up > 5e5dc33d5dac ("bpf: veth driver panics when xdp prog attached before > veth_open"). > > That option would be possibly safer, because I feel there are other > issues with 2e0de6366ac1, and would offer the opportunity to refactor > its logic a bit: the napi enable/disable condition is quite complex and > not used consistently mixing and alternating the gro/xdp/peer xdp check > with the napi ptr dereference. > > Ideally it would be better to have an helper alike > napi_should_be_enabled(), use it everywhere, and pair the new code with > some selftests, extending the existing ones. > > WDYT? I take your point. Thanks. > > Side notes: > - Heng Qi address is bouncing > - the existing veth self-tests would have caught the bug addressed > here, if commit afef88e65554 ("selftests/bpf: Store BPF object files > with .bpf.o extension") would not have broken them meanwhile :( > > /P > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 0/2] veth: a couple of fixes 2022-11-21 3:33 ` Xuan Zhuo @ 2022-11-21 3:55 ` Heng Qi 2022-11-21 10:11 ` Paolo Abeni 0 siblings, 1 reply; 14+ messages in thread From: Heng Qi @ 2022-11-21 3:55 UTC (permalink / raw) To: Xuan Zhuo, Paolo Abeni; +Cc: bpf, netdev, Jakub Kicinski, John Fastabend 在 2022/11/21 上午11:33, Xuan Zhuo 写道: > On Fri, 18 Nov 2022 09:41:05 +0100, Paolo Abeni <pabeni@redhat.com> wrote: >> On Fri, 2022-11-18 at 00:33 +0100, Paolo Abeni wrote: >>> Recent changes in the veth driver caused a few regressions >>> this series addresses a couple of them, causing oops. >>> >>> Paolo Abeni (2): >>> veth: fix uninitialized napi disable >>> veth: fix double napi enable >>> >>> drivers/net/veth.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >> @Xuan Zhuo: another option would be reverting 2e0de6366ac1 ("veth: >> Avoid drop packets when xdp_redirect performs") and its follow-up >> 5e5dc33d5dac ("bpf: veth driver panics when xdp prog attached before >> veth_open"). >> >> That option would be possibly safer, because I feel there are other >> issues with 2e0de6366ac1, and would offer the opportunity to refactor >> its logic a bit: the napi enable/disable condition is quite complex and >> not used consistently mixing and alternating the gro/xdp/peer xdp check >> with the napi ptr dereference. >> >> Ideally it would be better to have an helper alike >> napi_should_be_enabled(), use it everywhere, and pair the new code with >> some selftests, extending the existing ones. >> >> WDYT? > I take your point. I'll rewrite a patch as soon as possible and resubmit it. Thanks. > > Thanks. > > >> Side notes: >> - Heng Qi address is bouncing >> - the existing veth self-tests would have caught the bug addressed >> here, if commit afef88e65554 ("selftests/bpf: Store BPF object files >> with .bpf.o extension") would not have broken them meanwhile :( >> >> /P >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 0/2] veth: a couple of fixes 2022-11-21 3:55 ` Heng Qi @ 2022-11-21 10:11 ` Paolo Abeni 2022-11-21 11:28 ` [PATCH 0/2] Revert "veth: Avoid drop packets when xdp_redirect performs" and its fix Heng Qi 0 siblings, 1 reply; 14+ messages in thread From: Paolo Abeni @ 2022-11-21 10:11 UTC (permalink / raw) To: Heng Qi, Xuan Zhuo; +Cc: bpf, netdev, Jakub Kicinski, John Fastabend On Mon, 2022-11-21 at 11:55 +0800, Heng Qi wrote: > 在 2022/11/21 上午11:33, Xuan Zhuo 写道: > > On Fri, 18 Nov 2022 09:41:05 +0100, Paolo Abeni <pabeni@redhat.com> wrote: > > > On Fri, 2022-11-18 at 00:33 +0100, Paolo Abeni wrote: > > > > Recent changes in the veth driver caused a few regressions > > > > this series addresses a couple of them, causing oops. > > > > > > > > Paolo Abeni (2): > > > > veth: fix uninitialized napi disable > > > > veth: fix double napi enable > > > > > > > > drivers/net/veth.c | 6 ++++-- > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > @Xuan Zhuo: another option would be reverting 2e0de6366ac1 ("veth: > > > Avoid drop packets when xdp_redirect performs") and its follow-up > > > 5e5dc33d5dac ("bpf: veth driver panics when xdp prog attached before > > > veth_open"). > > > option would be possibly safer, because I feel there are other > > > issues with 2e0de6366ac1, and would offer the opportunity to refactor > > > its logic a bit: the napi enable/disable condition is quite complex and > > > not used consistently mixing and alternating the gro/xdp/peer xdp check > > > with the napi ptr dereference. > > > > > > Ideally it would be better to have an helper alike > > > napi_should_be_enabled(), use it everywhere, and pair the new code with > > > some selftests, extending the existing ones. > > > > > > WDYT? > > I take your point. > > I'll rewrite a patch as soon as possible and resubmit it. Could you please first send the revert? Thanks! Paolo ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 0/2] Revert "veth: Avoid drop packets when xdp_redirect performs" and its fix 2022-11-21 10:11 ` Paolo Abeni @ 2022-11-21 11:28 ` Heng Qi 2022-11-21 11:28 ` [PATCH 1/2] Revert "bpf: veth driver panics when xdp prog attached before veth_open" Heng Qi 2022-11-21 11:28 ` [PATCH 2/2] Revert "veth: Avoid drop packets when xdp_redirect performs" Heng Qi 0 siblings, 2 replies; 14+ messages in thread From: Heng Qi @ 2022-11-21 11:28 UTC (permalink / raw) To: netdev, bpf; +Cc: Paolo Abeni, Jakub Kicinski, Xuan Zhuo This patch 2e0de6366ac16 enables napi of the peer veth automatically when the veth loads the xdp, but it breaks down as reported by Paolo and John. So reverting it and its fix, we will rework the patch and make it more robust based on comments. Heng Qi (2): Revert "bpf: veth driver panics when xdp prog attached before veth_open" Revert "veth: Avoid drop packets when xdp_redirect performs" drivers/net/veth.c | 88 +++++++--------------------------------------- 1 file changed, 12 insertions(+), 76 deletions(-) -- 2.19.1.6.gb485710b ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] Revert "bpf: veth driver panics when xdp prog attached before veth_open" 2022-11-21 11:28 ` [PATCH 0/2] Revert "veth: Avoid drop packets when xdp_redirect performs" and its fix Heng Qi @ 2022-11-21 11:28 ` Heng Qi 2022-11-21 11:28 ` [PATCH 2/2] Revert "veth: Avoid drop packets when xdp_redirect performs" Heng Qi 1 sibling, 0 replies; 14+ messages in thread From: Heng Qi @ 2022-11-21 11:28 UTC (permalink / raw) To: netdev, bpf; +Cc: Paolo Abeni, Jakub Kicinski, Xuan Zhuo This reverts commit 5e5dc33d5dacb34b0165061bc5a10efd2fd3b66f. This patch fixes the panic maked by 2e0de6366ac16. Now Paolo and Toke suggest reverting the patch 2e0de6366ac16 and making it stronger, so do this first. Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> --- drivers/net/veth.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/veth.c b/drivers/net/veth.c index 2a4592780141..b1ed5a93b6c5 100644 --- a/drivers/net/veth.c +++ b/drivers/net/veth.c @@ -1125,7 +1125,7 @@ static int veth_enable_xdp(struct net_device *dev) int err, i; rq = &priv->rq[0]; - napi_already_on = rcu_access_pointer(rq->napi); + napi_already_on = (dev->flags & IFF_UP) && rcu_access_pointer(rq->napi); if (!xdp_rxq_info_is_reg(&priv->rq[0].xdp_rxq)) { err = veth_enable_xdp_range(dev, 0, dev->real_num_rx_queues, napi_already_on); -- 2.19.1.6.gb485710b ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] Revert "veth: Avoid drop packets when xdp_redirect performs" 2022-11-21 11:28 ` [PATCH 0/2] Revert "veth: Avoid drop packets when xdp_redirect performs" and its fix Heng Qi 2022-11-21 11:28 ` [PATCH 1/2] Revert "bpf: veth driver panics when xdp prog attached before veth_open" Heng Qi @ 2022-11-21 11:28 ` Heng Qi 1 sibling, 0 replies; 14+ messages in thread From: Heng Qi @ 2022-11-21 11:28 UTC (permalink / raw) To: netdev, bpf; +Cc: Paolo Abeni, Jakub Kicinski, Xuan Zhuo This reverts commit 2e0de6366ac16ab4d0abb2aaddbc8a1eba216d11. Based on the issues reported by John and Paolo and their comments, this patch and the corresponding fix 5e5dc33d5da are reverted, and we'll remake it. Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> --- drivers/net/veth.c | 88 +++++++--------------------------------------- 1 file changed, 12 insertions(+), 76 deletions(-) diff --git a/drivers/net/veth.c b/drivers/net/veth.c index b1ed5a93b6c5..ac7c0653695f 100644 --- a/drivers/net/veth.c +++ b/drivers/net/veth.c @@ -1119,14 +1119,10 @@ static void veth_disable_xdp_range(struct net_device *dev, int start, int end, static int veth_enable_xdp(struct net_device *dev) { + bool napi_already_on = veth_gro_requested(dev) && (dev->flags & IFF_UP); struct veth_priv *priv = netdev_priv(dev); - bool napi_already_on; - struct veth_rq *rq; int err, i; - rq = &priv->rq[0]; - napi_already_on = (dev->flags & IFF_UP) && rcu_access_pointer(rq->napi); - if (!xdp_rxq_info_is_reg(&priv->rq[0].xdp_rxq)) { err = veth_enable_xdp_range(dev, 0, dev->real_num_rx_queues, napi_already_on); if (err) @@ -1327,28 +1323,18 @@ static int veth_set_channels(struct net_device *dev, static int veth_open(struct net_device *dev) { - struct veth_priv *peer_priv, *priv = netdev_priv(dev); + struct veth_priv *priv = netdev_priv(dev); struct net_device *peer = rtnl_dereference(priv->peer); - struct veth_rq *peer_rq; int err; if (!peer) return -ENOTCONN; - peer_priv = netdev_priv(peer); - peer_rq = &peer_priv->rq[0]; - if (priv->_xdp_prog) { err = veth_enable_xdp(dev); if (err) return err; - /* refer to the logic in veth_xdp_set() */ - if (!rtnl_dereference(peer_rq->napi)) { - err = veth_napi_enable(peer); - if (err) - return err; - } - } else if (veth_gro_requested(dev) || peer_priv->_xdp_prog) { + } else if (veth_gro_requested(dev)) { err = veth_napi_enable(dev); if (err) return err; @@ -1364,29 +1350,17 @@ static int veth_open(struct net_device *dev) static int veth_close(struct net_device *dev) { - struct veth_priv *peer_priv, *priv = netdev_priv(dev); + struct veth_priv *priv = netdev_priv(dev); struct net_device *peer = rtnl_dereference(priv->peer); - struct veth_rq *peer_rq; netif_carrier_off(dev); - if (peer) { - peer_priv = netdev_priv(peer); - peer_rq = &peer_priv->rq[0]; - } + if (peer) + netif_carrier_off(peer); - if (priv->_xdp_prog) { + if (priv->_xdp_prog) veth_disable_xdp(dev); - /* refer to the logic in veth_xdp_set */ - if (peer && rtnl_dereference(peer_rq->napi)) { - if (!veth_gro_requested(peer) && !peer_priv->_xdp_prog) - veth_napi_del(peer); - } - } else if (veth_gro_requested(dev) || (peer && peer_priv->_xdp_prog)) { + else if (veth_gro_requested(dev)) veth_napi_del(dev); - } - - if (peer) - netif_carrier_off(peer); return 0; } @@ -1496,21 +1470,17 @@ static int veth_set_features(struct net_device *dev, { netdev_features_t changed = features ^ dev->features; struct veth_priv *priv = netdev_priv(dev); - struct veth_rq *rq = &priv->rq[0]; int err; if (!(changed & NETIF_F_GRO) || !(dev->flags & IFF_UP) || priv->_xdp_prog) return 0; if (features & NETIF_F_GRO) { - if (!rtnl_dereference(rq->napi)) { - err = veth_napi_enable(dev); - if (err) - return err; - } + err = veth_napi_enable(dev); + if (err) + return err; } else { - if (rtnl_dereference(rq->napi)) - veth_napi_del(dev); + veth_napi_del(dev); } return 0; } @@ -1542,19 +1512,14 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog, struct netlink_ext_ack *extack) { struct veth_priv *priv = netdev_priv(dev); - struct veth_priv *peer_priv; struct bpf_prog *old_prog; - struct veth_rq *peer_rq; struct net_device *peer; - bool napi_already_off; unsigned int max_mtu; - bool noreq_napi; int err; old_prog = priv->_xdp_prog; priv->_xdp_prog = prog; peer = rtnl_dereference(priv->peer); - peer_priv = netdev_priv(peer); if (prog) { if (!peer) { @@ -1591,24 +1556,6 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog, } } - if (peer && (peer->flags & IFF_UP)) { - peer_rq = &peer_priv->rq[0]; - - /* If the peer hasn't enabled GRO and loaded xdp, - * then we enable napi automatically if its napi - * is not ready. - */ - napi_already_off = !rtnl_dereference(peer_rq->napi); - if (napi_already_off) { - err = veth_napi_enable(peer); - if (err) { - NL_SET_ERR_MSG_MOD(extack, - "Failed to automatically enable napi of peer"); - goto err; - } - } - } - if (!old_prog) { peer->hw_features &= ~NETIF_F_GSO_SOFTWARE; peer->max_mtu = max_mtu; @@ -1623,17 +1570,6 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog, if (peer) { peer->hw_features |= NETIF_F_GSO_SOFTWARE; peer->max_mtu = ETH_MAX_MTU; - peer_rq = &peer_priv->rq[0]; - - /* If the peer doesn't has its xdp and enabled - * GRO, then we disable napi if its napi is ready; - */ - if (rtnl_dereference(peer_rq->napi)) { - noreq_napi = !veth_gro_requested(peer) && - !peer_priv->_xdp_prog; - if (noreq_napi && (peer->flags & IFF_UP)) - veth_napi_del(peer); - } } } bpf_prog_put(old_prog); -- 2.19.1.6.gb485710b ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-11-21 11:34 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-17 23:33 [PATCH net-next 0/2] veth: a couple of fixes Paolo Abeni 2022-11-17 23:33 ` [PATCH net-next 1/2] veth: fix uninitialized napi disable Paolo Abeni 2022-11-17 23:33 ` [PATCH net-next 2/2] veth: fix double napi enable Paolo Abeni 2022-11-18 14:24 ` Heng Qi 2022-11-18 8:41 ` [PATCH net-next 0/2] veth: a couple of fixes Paolo Abeni 2022-11-18 11:05 ` Toke Høiland-Jørgensen 2022-11-19 1:06 ` Jakub Kicinski 2022-11-21 6:15 ` John Fastabend 2022-11-21 3:33 ` Xuan Zhuo 2022-11-21 3:55 ` Heng Qi 2022-11-21 10:11 ` Paolo Abeni 2022-11-21 11:28 ` [PATCH 0/2] Revert "veth: Avoid drop packets when xdp_redirect performs" and its fix Heng Qi 2022-11-21 11:28 ` [PATCH 1/2] Revert "bpf: veth driver panics when xdp prog attached before veth_open" Heng Qi 2022-11-21 11:28 ` [PATCH 2/2] Revert "veth: Avoid drop packets when xdp_redirect performs" Heng Qi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).