[ovs-dev] [PATCH v5 0/2] flow dumps containing igmp match cannot be restored

[Salvatore Daniele]

On Mon, Nov 29, 2021 at 8:01 AM Ilya Maximets <i.maximets at ovn.org> wrote:
>
> On 11/4/21 20:54, Salvatore Daniele wrote:
> > Hi Ilya,
> >
> > Per your feedback on v4, I accounted for igmp_type/code, both in the
> > work-around in 1/2, and removing it from match.c entirely in 2/2.
> >
> > I was not sure the best way to proceed regarding the issue I had
> > mentioned on v4 where ofputil_normalize_match__() fails to recognize
> > 'igmp/ip,nw_proto=2' as an L3 protocol and normalizes igmp flows,
> > removing igmp_type/code.
> >
> > However, I feel removing the fields as you suggested should solve the
> > problem regardless.
> >
> > Curious to hear your thoughts at your convenience!
>
> Hi.  Thanks for v5 and sorry for it taking so long for me to reply.
>
> Patches LGTM at the first glance.  I'll run a couple of tests with
> them and apply if everything will look fine.

Great, thank you Ilya!

>
> For the matching issue.  tp_src field in the OpenFlow structure is
> defined as "TCP/UDP/SCTP source port/ICMP type", so it's not intended
> to be used for IGMP packets.  Matching on IGMP fields also not supported
> by the OpenFlow specification.  So, it seems correct that OVS rejects
> this kind of matching requests.  Same for tp_dst.
>
> Best regards, Ilya Maximets.
>
> >
> > Best,
> >
> > Salvatore
> >
> > On Thu, Nov 4, 2021 at 3:39 PM Salvatore Daniele <sdaniele at redhat.com> wrote:
> >>
> >> match_format() prints the keyword "igmp" for flows with the field
> >> "ip,nw_proto=2". ofp_parse_protocol does not accept this value, or
> >> the values igmp_type or igmp_code.
> >>
> >> This results in flow dump restoration failing when the ovs-save script
> >> is used by "ovs-ctl restart" on a dump of flows containing this match.
> >> However, removing the "igmp" keyword entirely could break existing
> >> scripts in stable branches.
> >>
> >> The first patch addresses this issue by providing a workaround within
> >> ovs-save to preserve the 'igmp' keywords while allowing flows to be
> >> restored. This change would be backported to all stable branches.
> >>
> >> The second patch removes the 'igmp' outputs entirely, replacing it with
> >> 'ip,nw_proto=2'. This has been added to NEWS, and would be applied
> >> in master branch only.
> >>
> >> While it might make sense to eventually remove the ovs-save workaround,
> >> it remains in this second patch to ensure flows can be restored when
> >> upgrading ovs-vswitchd from older versions.
> >>
> >> v5
> >> - Handle igmp_type/code in workaround and remove them from match.c in
> >>   2/2
> >> v4
> >> - Include the ovs-save workaround in both patch 1/2 and 2/2 to address
> >>   upgrade case discussed in v3
> >> v3
> >> - Fixed typos to be inline with DCO policy
> >> - Rebased ontop of latest master
> >> v2
> >> - Address comments made on v1 with regard to a work around for stable
> >>   branches
> >>
> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1982743
> >>
> >> Signed-off-by: Salvatore Daniele <sdaniele at redhat.com>
> >> Signed-off-by: Adrian Moreno <amorenoz at redhat.com>
> >> Co-authored-by: Adrian Moreno <amorenoz at redhat.com>
> >>
> >> Adrian Moreno (1):
> >>   Match: Do not print "igmp" match keyword
> >>
> >> Salvatore Daniele (1):
> >>   ovs-save: Save igmp flows in ofp_parse syntax
> >>
> >>  NEWS               | 2 ++
> >>  lib/match.c        | 6 ------
> >>  tests/ovs-ofctl.at | 6 ++++++
> >>  utilities/ovs-save | 5 ++++-
> >>  4 files changed, 12 insertions(+), 7 deletions(-)
> >>
> >> --
> >> 2.31.1
> >>
> >
>