diff mbox series

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

Message ID 20201102232805.1960103-1-blp@ovn.org
State New
Headers show
Series [ovs-dev] ofp-parse: Support "igmp" keyword in flows. | expand

Commit Message

Ben Pfaff Nov. 2, 2020, 11:28 p.m. UTC
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(+)

Comments

Ilya Maximets Nov. 3, 2020, 2:53 a.m. UTC | #1
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)
>
Ben Pfaff Nov. 3, 2020, 4:46 p.m. UTC | #2
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.
Ilya Maximets Nov. 4, 2020, 4:33 p.m. UTC | #3
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.
diff mbox series

Patch

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)