[ovs-dev] ofp-parse: Support "igmp" keyword in flows.

match_format() prints out "igmp" for IGMP flows, but
ofp_parse_protocol() didn't accept it, which meant that OVS would print
out a flow that it wouldn't re-parse.  This fixes the problem and adds
a test.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/ofp-parse.c    | 1 +
 tests/ovs-ofctl.at | 2 ++
 2 files changed, 3 insertions(+)

On 11/3/20 12:28 AM, Ben Pfaff wrote:
> match_format() prints out "igmp" for IGMP flows, but
> ofp_parse_protocol() didn't accept it, which meant that OVS would print
> out a flow that it wouldn't re-parse.  This fixes the problem and adds
> a test.

I'm a bit confused.  IIUC, matching on igmp is not supported by any
OF version or by any extensions, i.e. we could match by 'ip,nw_proto=2',
but there is no special OF header for "igmp" or "igmp_type" or "igmp_code".
While it seems easy to add parsing of "igmp" itself, its fields ("igmp_type"
and "igmp_code") could appear in the output too and parsing of these fields
will, I guess, require a new OF extension.  Is it correct?  If so, maybe
it's better to remove "igmp*" printing code from the match_format() instead?

Best regards, Ilya Maximets.

> 
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
>  lib/ofp-parse.c    | 1 +
>  tests/ovs-ofctl.at | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> index a90b926efb54..395a519f555f 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -184,6 +184,7 @@ ofp_parse_protocol(const char *name, const struct ofp_protocol **p_out)
>          { "ip4", ETH_TYPE_IP, 0 },
>          { "arp", ETH_TYPE_ARP, 0 },
>          { "icmp", ETH_TYPE_IP, IPPROTO_ICMP },
> +        { "igmp", ETH_TYPE_IP, IPPROTO_IGMP },
>          { "tcp", ETH_TYPE_IP, IPPROTO_TCP },
>          { "udp", ETH_TYPE_IP, IPPROTO_UDP },
>          { "sctp", ETH_TYPE_IP, IPPROTO_SCTP },
> diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
> index b6951f404c45..05ba943d84d6 100644
> --- a/tests/ovs-ofctl.at
> +++ b/tests/ovs-ofctl.at
> @@ -191,6 +191,7 @@ actions=note:41.42.43,note:00.01.02.03.04.05.06.07,note
>  ip,actions=set_field:10.4.3.77->ip_src,mod_nw_ecn:2
>  sctp actions=drop
>  sctp actions=drop
> +igmp actions=drop
>  in_port=0 actions=resubmit:0
>  actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678)
>  actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678,ingress)
> @@ -225,6 +226,7 @@ OFPT_FLOW_MOD: ADD actions=note:41.42.43.00.00.00,note:00.01.02.03.04.05.06.07.0
>  OFPT_FLOW_MOD: ADD ip actions=mod_nw_src:10.4.3.77,load:0x2->NXM_NX_IP_ECN[]
>  OFPT_FLOW_MOD: ADD sctp actions=drop
>  OFPT_FLOW_MOD: ADD sctp actions=drop
> +OFPT_FLOW_MOD: ADD igmp actions=drop
>  OFPT_FLOW_MOD: ADD in_port=0 actions=resubmit:0
>  OFPT_FLOW_MOD: ADD actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678)
>  OFPT_FLOW_MOD: ADD actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678,ingress)
>

On Tue, Nov 03, 2020 at 03:53:51AM +0100, Ilya Maximets wrote:
> On 11/3/20 12:28 AM, Ben Pfaff wrote:
> > match_format() prints out "igmp" for IGMP flows, but
> > ofp_parse_protocol() didn't accept it, which meant that OVS would print
> > out a flow that it wouldn't re-parse.  This fixes the problem and adds
> > a test.
> 
> I'm a bit confused.  IIUC, matching on igmp is not supported by any
> OF version or by any extensions, i.e. we could match by 'ip,nw_proto=2',
> but there is no special OF header for "igmp" or "igmp_type" or "igmp_code".
> While it seems easy to add parsing of "igmp" itself, its fields ("igmp_type"
> and "igmp_code") could appear in the output too and parsing of these fields
> will, I guess, require a new OF extension.  Is it correct?  If so, maybe
> it's better to remove "igmp*" printing code from the match_format() instead?

I hadn't thought about that.  I was just thinking of this as a short
form of "ip,nw_proto=2".

It looks like OVS's support for IGMP matching is inconsistent.
flow_extract() pulls the igmp_type and igmp_code values into tp_src and
tp_dst.  The kernel datapath doesn't do that, though, and there's no
support for the fields at the OpenFlow level.  I guess it has never been
important enough.

OK, I won't complain either way then.

On 11/3/20 5:46 PM, Ben Pfaff wrote:
> On Tue, Nov 03, 2020 at 03:53:51AM +0100, Ilya Maximets wrote:
>> On 11/3/20 12:28 AM, Ben Pfaff wrote:
>>> match_format() prints out "igmp" for IGMP flows, but
>>> ofp_parse_protocol() didn't accept it, which meant that OVS would print
>>> out a flow that it wouldn't re-parse.  This fixes the problem and adds
>>> a test.
>>
>> I'm a bit confused.  IIUC, matching on igmp is not supported by any
>> OF version or by any extensions, i.e. we could match by 'ip,nw_proto=2',
>> but there is no special OF header for "igmp" or "igmp_type" or "igmp_code".
>> While it seems easy to add parsing of "igmp" itself, its fields ("igmp_type"
>> and "igmp_code") could appear in the output too and parsing of these fields
>> will, I guess, require a new OF extension.  Is it correct?  If so, maybe
>> it's better to remove "igmp*" printing code from the match_format() instead?
> 
> I hadn't thought about that.  I was just thinking of this as a short
> form of "ip,nw_proto=2".
> 
> It looks like OVS's support for IGMP matching is inconsistent.
> flow_extract() pulls the igmp_type and igmp_code values into tp_src and
> tp_dst.  The kernel datapath doesn't do that, though, and there's no
> support for the fields at the OpenFlow level.  I guess it has never been
> important enough.

From my understanding, for now we only need support inside OVS userspace
for multicast snooping.  So, OVS just instructs datapath that all igmp
(ip,nw_proto=2) packets should go to userspace, so they could be specially
processed.  And this is just a configuration of the bridge, so no need
to have support in OpenFlow.  I think, the same we have for LLDP:  OVS
instructs datapath that all 0x88cc packets should go to userspace and
userspace handles these packets adjusting the configuration of ports and
bridges.  In this case we don't really need any special support from the
datapath and this also works without involving the OF level as these
packets are consumed by OVS itself and not passed through OF pipelines.

> 
> OK, I won't complain either way then.

I think, it's better to just stop printing word 'igmp' in flow dumps to
avoid confusion since OF doesn't support igmp.  We're not printing anything
like this for lldp or some other types of packets anyway.

Flavio, you were the original author of this mcast snooping feature, what
do you think?

Best regards, Ilya Maximets.

On Wed, Nov 04, 2020 at 05:33:27PM +0100, Ilya Maximets wrote:
> On 11/3/20 5:46 PM, Ben Pfaff wrote:
> > On Tue, Nov 03, 2020 at 03:53:51AM +0100, Ilya Maximets wrote:
> >> On 11/3/20 12:28 AM, Ben Pfaff wrote:
> >>> match_format() prints out "igmp" for IGMP flows, but
> >>> ofp_parse_protocol() didn't accept it, which meant that OVS would print
> >>> out a flow that it wouldn't re-parse.  This fixes the problem and adds
> >>> a test.
> >>
> >> I'm a bit confused.  IIUC, matching on igmp is not supported by any
> >> OF version or by any extensions, i.e. we could match by 'ip,nw_proto=2',
> >> but there is no special OF header for "igmp" or "igmp_type" or "igmp_code".
> >> While it seems easy to add parsing of "igmp" itself, its fields ("igmp_type"
> >> and "igmp_code") could appear in the output too and parsing of these fields
> >> will, I guess, require a new OF extension.  Is it correct?  If so, maybe
> >> it's better to remove "igmp*" printing code from the match_format() instead?
> > 
> > I hadn't thought about that.  I was just thinking of this as a short
> > form of "ip,nw_proto=2".
> > 
> > It looks like OVS's support for IGMP matching is inconsistent.
> > flow_extract() pulls the igmp_type and igmp_code values into tp_src and
> > tp_dst.  The kernel datapath doesn't do that, though, and there's no
> > support for the fields at the OpenFlow level.  I guess it has never been
> > important enough.
> 
> From my understanding, for now we only need support inside OVS userspace
> for multicast snooping.  So, OVS just instructs datapath that all igmp
> (ip,nw_proto=2) packets should go to userspace, so they could be specially
> processed.  And this is just a configuration of the bridge, so no need
> to have support in OpenFlow.  I think, the same we have for LLDP:  OVS
> instructs datapath that all 0x88cc packets should go to userspace and
> userspace handles these packets adjusting the configuration of ports and
> bridges.  In this case we don't really need any special support from the
> datapath and this also works without involving the OF level as these
> packets are consumed by OVS itself and not passed through OF pipelines.
> 
> > 
> > OK, I won't complain either way then.
> 
> I think, it's better to just stop printing word 'igmp' in flow dumps to
> avoid confusion since OF doesn't support igmp.  We're not printing anything
> like this for lldp or some other types of packets anyway.
> 
> Flavio, you were the original author of this mcast snooping feature, what
> do you think?

Sorry the delay.

Your understanding is correct about the membership packets going
to userspace for special processing. There was no intention of
exposing the protocol itself.

I don't like changing outputs because doing so can break scripts
but maybe in this case that's better in order to keep consistency
with LLDP and OF.