| Message ID | CAOE=1Z2JitAPf-_53MNAFEO6dybp1n2u-tymbJMn5pCsaGTMAA@mail.gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] ovs-tcpdump: Bugfix-of-ovs-tcpdump | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On Mon, Jul 10, 2023 at 11:13 PM Simon Jones <batmanustc@gmail.com> wrote: > > From: simon <batmanustc@gmail.com> > > Fix bug of ovs-tcpdump, which will cause megaflow action wrong. > > As use ovs-tcpdump will add mipxxx NIC, and this NIC has IPv6 address by > default. > For vxlan topology, mipxxx will be treated as tunnel port, and will got > error actions. > > For detail discuss, refer: > https://github.com/batmancn/github-work/blob/main/ovs-bugfix-ovs-tcpdump.md > > Signed-off-by: simon <batmanustc@gmail.com> I was able to confirm a variety of strange behaviours with these autoconf IPv6 addresses in a vxlan setup. And this patch fixed those issues. Acked-by: Mike Pattrick <mkp@redhat.com> > --- > utilities/ovs-tcpdump.in | 4 ++++ > 1 file changed, 4 insertions(+) > diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in > index 420c11eb8..4cbd9a5d3 100755 > --- a/utilities/ovs-tcpdump.in > +++ b/utilities/ovs-tcpdump.in > @@ -96,6 +96,10 @@ def _install_dst_if_linux(tap_name, mtu_value=None): > *(['ip', 'link', 'set', 'dev', str(tap_name), 'up'])) > pipe.wait() > > + pipe = _doexec( > + *(['ip', '-6', 'addr', 'flush', 'dev', str(tap_name)])) > + pipe.wait() > + > > def _remove_dst_if_linux(tap_name): > _doexec( > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Hi Simon, Thanks for the contribution! Simon Jones <batmanustc@gmail.com> writes: > From: simon <batmanustc@gmail.com> > > Fix bug of ovs-tcpdump, which will cause megaflow action wrong. > > As use ovs-tcpdump will add mipxxx NIC, and this NIC has IPv6 address by > default. This is true only on systems where the ipv6 autoconf sysctl is set to true, I think. I think it is probably worth including that detail. > For vxlan topology, mipxxx will be treated as tunnel port, and will got > error actions. > > For detail discuss, refer: > https://github.com/batmancn/github-work/blob/main/ovs-bugfix-ovs-tcpdump.md Why not include some of these details in the commit message? At least, we can say that ipv6 packets going down the mirror port will cause erroneous behavior in most scenarios, and we should just stop ipv6 address there. Actually, I wonder if there should be any other steps we might need to take incase zeroconf is setup for ipv4 on the system, but hopefully that is managed separately. > Signed-off-by: simon <batmanustc@gmail.com> > --- Acked-by: Aaron Conole <aconole@redhat.com>
On Thu, Jul 20, 2023 at 11:38:41AM -0400, Aaron Conole wrote: > Hi Simon, > > Thanks for the contribution! > > Simon Jones <batmanustc@gmail.com> writes: > > > From: simon <batmanustc@gmail.com> > > > > Fix bug of ovs-tcpdump, which will cause megaflow action wrong. > > > > As use ovs-tcpdump will add mipxxx NIC, and this NIC has IPv6 address by > > default. > > This is true only on systems where the ipv6 autoconf sysctl is set to > true, I think. I think it is probably worth including that detail. > > > For vxlan topology, mipxxx will be treated as tunnel port, and will got > > error actions. > > > > For detail discuss, refer: > > https://github.com/batmancn/github-work/blob/main/ovs-bugfix-ovs-tcpdump.md > > Why not include some of these details in the commit message? At least, > we can say that ipv6 packets going down the mirror port will cause > erroneous behavior in most scenarios, and we should just stop ipv6 > address there. > > Actually, I wonder if there should be any other steps we might need to > take incase zeroconf is setup for ipv4 on the system, but hopefully that > is managed separately. Hi Simon, could I ask you to prepare a v3 taking into account Arron's suggestion. > > Signed-off-by: simon <batmanustc@gmail.com> And updating the above to include your first and last name, as appears in the from address of your emails. > > --- > > Acked-by: Aaron Conole <aconole@redhat.com> You can include this in v3, immediately after your Signed-off-by line. Thanks!
ok ~ Many thanks for suggestion, I will fix these ~ ---- Simon Jones Simon Horman <simon.horman@corigine.com> 于2023年7月27日周四 01:09写道： > On Thu, Jul 20, 2023 at 11:38:41AM -0400, Aaron Conole wrote: > > Hi Simon, > > > > Thanks for the contribution! > > > > Simon Jones <batmanustc@gmail.com> writes: > > > > > From: simon <batmanustc@gmail.com> > > > > > > Fix bug of ovs-tcpdump, which will cause megaflow action wrong. > > > > > > As use ovs-tcpdump will add mipxxx NIC, and this NIC has IPv6 address > by > > > default. > > > > This is true only on systems where the ipv6 autoconf sysctl is set to > > true, I think. I think it is probably worth including that detail. > > > > > For vxlan topology, mipxxx will be treated as tunnel port, and will got > > > error actions. > > > > > > For detail discuss, refer: > > > > https://github.com/batmancn/github-work/blob/main/ovs-bugfix-ovs-tcpdump.md > > > > Why not include some of these details in the commit message? At least, > > we can say that ipv6 packets going down the mirror port will cause > > erroneous behavior in most scenarios, and we should just stop ipv6 > > address there. > > > > Actually, I wonder if there should be any other steps we might need to > > take incase zeroconf is setup for ipv4 on the system, but hopefully that > > is managed separately. > > Hi Simon, > > could I ask you to prepare a v3 taking into account Arron's suggestion. > > > > Signed-off-by: simon <batmanustc@gmail.com> > > And updating the above to include your first and last name, > as appears in the from address of your emails. > > > > --- > > > > Acked-by: Aaron Conole <aconole@redhat.com> > > You can include this in v3, immediately after your Signed-off-by line. > > Thanks! >
Hi Aaron, Thanks for your suggestion. This is my opinion as below. ---- Simon Jones Aaron Conole <aconole@redhat.com> 于2023年7月20日周四 23:41写道： > Hi Simon, > > Thanks for the contribution! > > Simon Jones <batmanustc@gmail.com> writes: > > > From: simon <batmanustc@gmail.com> > > > > Fix bug of ovs-tcpdump, which will cause megaflow action wrong. > > > > As use ovs-tcpdump will add mipxxx NIC, and this NIC has IPv6 address by > > default. > > This is true only on systems where the ipv6 autoconf sysctl is set to > true, I think. I think it is probably worth including that detail. > > > For vxlan topology, mipxxx will be treated as tunnel port, and will got > > error actions. > > > > For detail discuss, refer: > > > https://github.com/batmancn/github-work/blob/main/ovs-bugfix-ovs-tcpdump.md > > Why not include some of these details in the commit message? At least, > we can say that ipv6 packets going down the mirror port will cause > erroneous behavior in most scenarios, and we should just stop ipv6 > address there. > Ok, how about change like this: Fix bug of ovs-tcpdump, which will cause megaflow action wrong. Details: As use ovs-tcpdump will add mipxxx NIC, and this NIC has IPv6 address by default. For vxlan topology, mipxxx, which has IPv6 address, will be treated as tunnel port, and will got error actions. So we should just stop ipv6 address there. For more detail discuss, refer: https://github.com/batmancn/github-work/blob/main/ovs-bugfix-ovs-tcpdump.md What patch do: So we need to clear all IP address on mipxxx NIC. As there is no IPv4 address, but has IPv6 by default, which is tested on ubuntu and centos. So my patch is to clear IPv6 address on mipxxx NIC. > > Actually, I wonder if there should be any other steps we might need to > take incase zeroconf is setup for ipv4 on the system, but hopefully that > is managed separately. > I have tested on ubuntu20.04 and centos stream 8, there is no IPv4 address by default. I think for now this patch works. > > > Signed-off-by: simon <batmanustc@gmail.com> > > --- > > Acked-by: Aaron Conole <aconole@redhat.com> > >
Simon Jones <batmanustc@gmail.com> writes: > Hi Aaron, > > Thanks for your suggestion. This is my opinion as below. > > ---- > Simon Jones > > Aaron Conole <aconole@redhat.com> 于2023年7月20日周四 23:41写道： > > Hi Simon, > > Thanks for the contribution! > > Simon Jones <batmanustc@gmail.com> writes: > > > From: simon <batmanustc@gmail.com> > > > > Fix bug of ovs-tcpdump, which will cause megaflow action wrong. > > > > As use ovs-tcpdump will add mipxxx NIC, and this NIC has IPv6 address by > > default. > > This is true only on systems where the ipv6 autoconf sysctl is set to > true, I think. I think it is probably worth including that detail. > > > For vxlan topology, mipxxx will be treated as tunnel port, and will got > > error actions. > > > > For detail discuss, refer: > > https://github.com/batmancn/github-work/blob/main/ovs-bugfix-ovs-tcpdump.md > > Why not include some of these details in the commit message? At least, > we can say that ipv6 packets going down the mirror port will cause > erroneous behavior in most scenarios, and we should just stop ipv6 > address there. > > Ok, how about change like this: I don't think we need the pointer to the markdown file after including the details. > Fix bug of ovs-tcpdump, which will cause megaflow action wrong. maybe: ovs-tcpdump: clear auto-assigned ipv6 address of mirror port ovs-tcpdump will add mipxxx NIC, and on some systems this NIC has IPv6 address by default. For vxlan topology, mipxxx, which has IPv6 address, will be treated as tunnel port, and will got error actions. Prevent this by clearing the auto-assigned IPv6 address. This can also be controlled on some systems with ipv6 sysctls. Tested on centos stream 8, and ubunto20.04 > As there is no IPv4 address, but has IPv6 by default, which is tested on ubuntu and centos. > So my patch is to clear IPv6 address on mipxxx NIC. > > > Actually, I wonder if there should be any other steps we might need to > take incase zeroconf is setup for ipv4 on the system, but hopefully that > is managed separately. > > I have tested on ubuntu20.04 and centos stream 8, there is no IPv4 address by default. > I think for now this patch works. > > > > Signed-off-by: simon <batmanustc@gmail.com> > > --- > > Acked-by: Aaron Conole <aconole@redhat.com>
ok～ ---- Simon Jones Aaron Conole <aconole@redhat.com> 于2023年7月27日周四 22:32写道： > Simon Jones <batmanustc@gmail.com> writes: > > > Hi Aaron, > > > > Thanks for your suggestion. This is my opinion as below. > > > > ---- > > Simon Jones > > > > Aaron Conole <aconole@redhat.com> 于2023年7月20日周四 23:41写道： > > > > Hi Simon, > > > > Thanks for the contribution! > > > > Simon Jones <batmanustc@gmail.com> writes: > > > > > From: simon <batmanustc@gmail.com> > > > > > > Fix bug of ovs-tcpdump, which will cause megaflow action wrong. > > > > > > As use ovs-tcpdump will add mipxxx NIC, and this NIC has IPv6 address > by > > > default. > > > > This is true only on systems where the ipv6 autoconf sysctl is set to > > true, I think. I think it is probably worth including that detail. > > > > > For vxlan topology, mipxxx will be treated as tunnel port, and will > got > > > error actions. > > > > > > For detail discuss, refer: > > > > https://github.com/batmancn/github-work/blob/main/ovs-bugfix-ovs-tcpdump.md > > > > Why not include some of these details in the commit message? At least, > > we can say that ipv6 packets going down the mirror port will cause > > erroneous behavior in most scenarios, and we should just stop ipv6 > > address there. > > > > Ok, how about change like this: > > I don't think we need the pointer to the markdown file after including > the details. > > > Fix bug of ovs-tcpdump, which will cause megaflow action wrong. > > maybe: > > ovs-tcpdump: clear auto-assigned ipv6 address of mirror port > > ovs-tcpdump will add mipxxx NIC, and on some systems this NIC has IPv6 > address by default. For vxlan topology, mipxxx, which has IPv6 > address, will be treated as tunnel port, and will got error actions. > > Prevent this by clearing the auto-assigned IPv6 address. This can also > be controlled on some systems with ipv6 sysctls. > > Tested on centos stream 8, and ubunto20.04 > > > As there is no IPv4 address, but has IPv6 by default, which is tested on > ubuntu and centos. > > So my patch is to clear IPv6 address on mipxxx NIC. > > > > > > Actually, I wonder if there should be any other steps we might need to > > take incase zeroconf is setup for ipv4 on the system, but hopefully that > > is managed separately. > > > > I have tested on ubuntu20.04 and centos stream 8, there is no IPv4 > address by default. > > I think for now this patch works. > > > > > > > Signed-off-by: simon <batmanustc@gmail.com> > > > --- > > > > Acked-by: Aaron Conole <aconole@redhat.com> > >
diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in index 420c11eb8..4cbd9a5d3 100755 --- a/utilities/ovs-tcpdump.in +++ b/utilities/ovs-tcpdump.in @@ -96,6 +96,10 @@ def _install_dst_if_linux(tap_name, mtu_value=None): *(['ip', 'link', 'set', 'dev', str(tap_name), 'up'])) pipe.wait() + pipe = _doexec( + *(['ip', '-6', 'addr', 'flush', 'dev', str(tap_name)])) + pipe.wait() + def _remove_dst_if_linux(tap_name): _doexec(