| Message ID | cover.1668727939.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | veth: a couple of fixes | expand |
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
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
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
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 >
在 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 >>
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.
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
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(-)