Re: [ovs-dev] [PATCH] ovn-northd: Skip icmp4 packets destined for router ports from conntrack

[Numan Siddique]

Adding Joe and Jarno to CC.



On Tue, Mar 14, 2017 at 9:01 PM, Lance Richardson <lrich...@redhat.com>
wrote:

>
>
> - Original Message -
> > From: "Numan Siddique" <nusid...@redhat.com>
> > To: "Russell Bryant" <russ...@ovn.org>
> > Cc: "ovs dev" <d...@openvswitch.org>
> > Sent: Tuesday, March 14, 2017 11:21:33 AM
> > Subject: Re: [ovs-dev] [PATCH] ovn-northd: Skip icmp4 packets destined
> for router ports from conntrack
> >
> > On Tue, Mar 14, 2017 at 12:57 AM, Russell Bryant <russ...@ovn.org>
> wrote:
> >
> > > On Fri, Mar 10, 2017 at 4:48 PM, Russell Bryant <russ...@ovn.org>
> wrote:
> > > > On Fri, Mar 10, 2017 at 2:35 PM, Russell Bryant <russ...@ovn.org>
> wrote:
> > > >> On Thu, Mar 9, 2017 at 11:52 PM, Numan Siddique <
> nusid...@redhat.com>
> > > wrote:
> > > >> I don't think it's a Neutron issue.
> > > >>
> > > >> I see the conntrack entry remaining in the UNREPLIED state, even in
> > > >> the working case where there's not an ACL dropping the reply.
> > > >>
> > > >> icmp 1 29 src=10.0.0.10 dst=10.0.0.1 type=8 code=0 id=25857
> > > >> [UNREPLIED] src=10.0.0.1 dst=10.0.0.10 type=0 code=0 id=25857 mark=0
> > > >> zone=8 use=1
> > > >>
> > > >> If I ping a different address (something past the logical router), I
> > > >> see a proper conntrack entry that's not in the UNREPLIED state.
> > > >>
> > > >> I wonder if there's something about how we are generating the ICMP
> > > >> response from the logical router that's making conntrack not
> properly
> > > >> associate it with the request?
> > > >
> > > > I checked into this and there's no meaningful difference in how we
> > > > form the ICMP reply.
> > > >
> > > > I'm guessing this has to do with the request and reply both going
> > > > through conntrack as a part of processing the same packet in the OVS
> > > > data path.  That's not behaving how we would expect.  I'll keep
> > > > looking next week to try to identify the root cause here, but I would
> > > > appreciate any insight others may have about the behavior we should
> > > > expect in this scenario.
> > >
> > > I'm able to reproduce this outside of OpenStack.
> > >
> > > https://gist.github.com/russellb/4ab0a9641f12f8ac66fdd6822ee7789e
> > >
> > > This creates an OVN setup with two logical switches connected by a
> > > logical router.
> > >
> > > switch d47412f9-e64a-4734-be26-80ee71ded805 (sw1)
> > > port sw1-port1
> > > addresses: ["50:54:00:00:00:03 11.0.0.2"]
> > > switch a1730d73-ccab-4f00-b748-3cafb683e9b8 (sw0)
> > > port sw0-port2
> > > addresses: ["50:54:00:00:00:02 192.168.0.3"]
> > > port sw0-port1
> > > addresses: ["50:54:00:00:00:01 192.168.0.2"]
> > > router 193f68cd-4a93-4a04-ad3b-3ddf7b5c66f3 (lr0)
> > >
> > > The ping commands at the end demonstrate the problem.  My expectation
> > > is that the very last ping command should be successful, but is not
> > > due to the issue we're seeing here.
> > >
> > >
> >
> > This issue seems to be fixed with the below code diff. From my
> observation,
> > I could see that when the router datapath generates the icmp response and
> > when the reply packet hits the ovs_ct_execute function in datapath
> >  - the "sw_flow_key - key" param has old conntrack values
> >  - in the function "__ovs_ct_lookup",  skb_nfct_cached(net, key, info,
> skb)
> > returns false.
> >  - and the state is not set with the flag- OVS_CS_F_ESTABLISHED.
> >
> >
> > I want to check, if this is the right fix and get your feedback before
> > sending the upstream patch to net-next.
> >
>
> Hi Numan,
>
> Hopefully folks more familiar with this code than me will weigh in, but
> your patch seems to make sense.
>
> Since this is a bug fix, if you do wind up submitting it upstream it
> should go to "net" instead of "net-next" (with a "Fixes:" tag).
>
> Regards,
>
>Lance
>
>
​Thanks Lance for your comments.

​


> >
> > ><
> > diff --git a/datapath/flow.c b/datapath/flow.c
> > index 2bc1ad0..2c59acd 100644
> > --- a/datapath/flow.c
> > +++ b/datapath/flow.c
> > @@ -845,6 +845,8 @@ int ovs_flow_key_extract_userspace(struct net *net,
> > const struct nlattr *attr,
> > if (err)
> > return err;
> >
> > +   ovs_ct_fill_key(skb, key);
> > +
> > /* Check that we have conntrack original direction tuple metadata
> > only
> >  * for packets for which it makes sense.  Otherwise the key may
> be
> >  * corrupted due to overlapping key fields.
> >
> > ><
> >
> > Thanks
> > Numan
> >
> >
> > ​
> >
> >
> > > --
> > > Russell Bryant
> > >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn-northd: Skip icmp4 packets destined for router ports from conntrack

[Lance Richardson]

- Original Message -
> From: "Numan Siddique" <nusid...@redhat.com>
> To: "Russell Bryant" <russ...@ovn.org>
> Cc: "ovs dev" <d...@openvswitch.org>
> Sent: Tuesday, March 14, 2017 11:21:33 AM
> Subject: Re: [ovs-dev] [PATCH] ovn-northd: Skip icmp4 packets destined for 
> router ports from conntrack
> 
> On Tue, Mar 14, 2017 at 12:57 AM, Russell Bryant <russ...@ovn.org> wrote:
> 
> > On Fri, Mar 10, 2017 at 4:48 PM, Russell Bryant <russ...@ovn.org> wrote:
> > > On Fri, Mar 10, 2017 at 2:35 PM, Russell Bryant <russ...@ovn.org> wrote:
> > >> On Thu, Mar 9, 2017 at 11:52 PM, Numan Siddique <nusid...@redhat.com>
> > wrote:
> > >> I don't think it's a Neutron issue.
> > >>
> > >> I see the conntrack entry remaining in the UNREPLIED state, even in
> > >> the working case where there's not an ACL dropping the reply.
> > >>
> > >> icmp 1 29 src=10.0.0.10 dst=10.0.0.1 type=8 code=0 id=25857
> > >> [UNREPLIED] src=10.0.0.1 dst=10.0.0.10 type=0 code=0 id=25857 mark=0
> > >> zone=8 use=1
> > >>
> > >> If I ping a different address (something past the logical router), I
> > >> see a proper conntrack entry that's not in the UNREPLIED state.
> > >>
> > >> I wonder if there's something about how we are generating the ICMP
> > >> response from the logical router that's making conntrack not properly
> > >> associate it with the request?
> > >
> > > I checked into this and there's no meaningful difference in how we
> > > form the ICMP reply.
> > >
> > > I'm guessing this has to do with the request and reply both going
> > > through conntrack as a part of processing the same packet in the OVS
> > > data path.  That's not behaving how we would expect.  I'll keep
> > > looking next week to try to identify the root cause here, but I would
> > > appreciate any insight others may have about the behavior we should
> > > expect in this scenario.
> >
> > I'm able to reproduce this outside of OpenStack.
> >
> > https://gist.github.com/russellb/4ab0a9641f12f8ac66fdd6822ee7789e
> >
> > This creates an OVN setup with two logical switches connected by a
> > logical router.
> >
> > switch d47412f9-e64a-4734-be26-80ee71ded805 (sw1)
> > port sw1-port1
> > addresses: ["50:54:00:00:00:03 11.0.0.2"]
> > switch a1730d73-ccab-4f00-b748-3cafb683e9b8 (sw0)
> > port sw0-port2
> > addresses: ["50:54:00:00:00:02 192.168.0.3"]
> > port sw0-port1
> > addresses: ["50:54:00:00:00:01 192.168.0.2"]
> > router 193f68cd-4a93-4a04-ad3b-3ddf7b5c66f3 (lr0)
> >
> > The ping commands at the end demonstrate the problem.  My expectation
> > is that the very last ping command should be successful, but is not
> > due to the issue we're seeing here.
> >
> >
> 
> This issue seems to be fixed with the below code diff. From my observation,
> I could see that when the router datapath generates the icmp response and
> when the reply packet hits the ovs_ct_execute function in datapath
>  - the "sw_flow_key - key" param has old conntrack values
>  - in the function "__ovs_ct_lookup",  skb_nfct_cached(net, key, info, skb)
> returns false.
>  - and the state is not set with the flag- OVS_CS_F_ESTABLISHED.
> 
> 
> I want to check, if this is the right fix and get your feedback before
> sending the upstream patch to net-next.
> 

Hi Numan,

Hopefully folks more familiar with this code than me will weigh in, but
your patch seems to make sense.

Since this is a bug fix, if you do wind up submitting it upstream it
should go to "net" instead of "net-next" (with a "Fixes:" tag).

Regards,

   Lance

> 
> ><
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 2bc1ad0..2c59acd 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -845,6 +845,8 @@ int ovs_flow_key_extract_userspace(struct net *net,
> const struct nlattr *attr,
> if (err)
> return err;
> 
> +   ovs_ct_fill_key(skb, key);
> +
> /* Check that we have conntrack original direction tuple metadata
> only
>  * for packets for which it makes sense.  Otherwise the key may be
>  * corrupted due to overlapping key fields.
> 
> ><
> 
> Thanks
> Numan
> 
> 
> ​
> 
> 
> > --
> > Russell Bryant
> >
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn-northd: Skip icmp4 packets destined for router ports from conntrack

[Numan Siddique]

On Tue, Mar 14, 2017 at 12:57 AM, Russell Bryant  wrote:

> On Fri, Mar 10, 2017 at 4:48 PM, Russell Bryant  wrote:
> > On Fri, Mar 10, 2017 at 2:35 PM, Russell Bryant  wrote:
> >> On Thu, Mar 9, 2017 at 11:52 PM, Numan Siddique 
> wrote:
> >> I don't think it's a Neutron issue.
> >>
> >> I see the conntrack entry remaining in the UNREPLIED state, even in
> >> the working case where there's not an ACL dropping the reply.
> >>
> >> icmp 1 29 src=10.0.0.10 dst=10.0.0.1 type=8 code=0 id=25857
> >> [UNREPLIED] src=10.0.0.1 dst=10.0.0.10 type=0 code=0 id=25857 mark=0
> >> zone=8 use=1
> >>
> >> If I ping a different address (something past the logical router), I
> >> see a proper conntrack entry that's not in the UNREPLIED state.
> >>
> >> I wonder if there's something about how we are generating the ICMP
> >> response from the logical router that's making conntrack not properly
> >> associate it with the request?
> >
> > I checked into this and there's no meaningful difference in how we
> > form the ICMP reply.
> >
> > I'm guessing this has to do with the request and reply both going
> > through conntrack as a part of processing the same packet in the OVS
> > data path.  That's not behaving how we would expect.  I'll keep
> > looking next week to try to identify the root cause here, but I would
> > appreciate any insight others may have about the behavior we should
> > expect in this scenario.
>
> I'm able to reproduce this outside of OpenStack.
>
> https://gist.github.com/russellb/4ab0a9641f12f8ac66fdd6822ee7789e
>
> This creates an OVN setup with two logical switches connected by a
> logical router.
>
> switch d47412f9-e64a-4734-be26-80ee71ded805 (sw1)
> port sw1-port1
> addresses: ["50:54:00:00:00:03 11.0.0.2"]
> switch a1730d73-ccab-4f00-b748-3cafb683e9b8 (sw0)
> port sw0-port2
> addresses: ["50:54:00:00:00:02 192.168.0.3"]
> port sw0-port1
> addresses: ["50:54:00:00:00:01 192.168.0.2"]
> router 193f68cd-4a93-4a04-ad3b-3ddf7b5c66f3 (lr0)
>
> The ping commands at the end demonstrate the problem.  My expectation
> is that the very last ping command should be successful, but is not
> due to the issue we're seeing here.
>
>

This issue seems to be fixed with the below code diff. From my observation,
I could see that when the router datapath generates the icmp response and
when the reply packet hits the ovs_ct_execute function in datapath
 - the "sw_flow_key - key" param has old conntrack values
 - in the function "__ovs_ct_lookup",  skb_nfct_cached(net, key, info, skb)
returns false.
 - and the state is not set with the flag- OVS_CS_F_ESTABLISHED.


I want to check, if this is the right fix and get your feedback before
sending the upstream patch to net-next.


><
diff --git a/datapath/flow.c b/datapath/flow.c
index 2bc1ad0..2c59acd 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -845,6 +845,8 @@ int ovs_flow_key_extract_userspace(struct net *net,
const struct nlattr *attr,
if (err)
return err;

+   ovs_ct_fill_key(skb, key);
+
/* Check that we have conntrack original direction tuple metadata
only
 * for packets for which it makes sense.  Otherwise the key may be
 * corrupted due to overlapping key fields.

><

Thanks
Numan


​


> --
> Russell Bryant
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn-northd: Skip icmp4 packets destined for router ports from conntrack

[Russell Bryant]

On Fri, Mar 10, 2017 at 4:48 PM, Russell Bryant  wrote:
> On Fri, Mar 10, 2017 at 2:35 PM, Russell Bryant  wrote:
>> On Thu, Mar 9, 2017 at 11:52 PM, Numan Siddique  wrote:
>> I don't think it's a Neutron issue.
>>
>> I see the conntrack entry remaining in the UNREPLIED state, even in
>> the working case where there's not an ACL dropping the reply.
>>
>> icmp 1 29 src=10.0.0.10 dst=10.0.0.1 type=8 code=0 id=25857
>> [UNREPLIED] src=10.0.0.1 dst=10.0.0.10 type=0 code=0 id=25857 mark=0
>> zone=8 use=1
>>
>> If I ping a different address (something past the logical router), I
>> see a proper conntrack entry that's not in the UNREPLIED state.
>>
>> I wonder if there's something about how we are generating the ICMP
>> response from the logical router that's making conntrack not properly
>> associate it with the request?
>
> I checked into this and there's no meaningful difference in how we
> form the ICMP reply.
>
> I'm guessing this has to do with the request and reply both going
> through conntrack as a part of processing the same packet in the OVS
> data path.  That's not behaving how we would expect.  I'll keep
> looking next week to try to identify the root cause here, but I would
> appreciate any insight others may have about the behavior we should
> expect in this scenario.

I'm able to reproduce this outside of OpenStack.

https://gist.github.com/russellb/4ab0a9641f12f8ac66fdd6822ee7789e

This creates an OVN setup with two logical switches connected by a
logical router.

switch d47412f9-e64a-4734-be26-80ee71ded805 (sw1)
port sw1-port1
addresses: ["50:54:00:00:00:03 11.0.0.2"]
switch a1730d73-ccab-4f00-b748-3cafb683e9b8 (sw0)
port sw0-port2
addresses: ["50:54:00:00:00:02 192.168.0.3"]
port sw0-port1
addresses: ["50:54:00:00:00:01 192.168.0.2"]
router 193f68cd-4a93-4a04-ad3b-3ddf7b5c66f3 (lr0)

The ping commands at the end demonstrate the problem.  My expectation
is that the very last ping command should be successful, but is not
due to the issue we're seeing here.

-- 
Russell Bryant
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn-northd: Skip icmp4 packets destined for router ports from conntrack

[Russell Bryant]

On Fri, Mar 10, 2017 at 2:35 PM, Russell Bryant  wrote:
> On Thu, Mar 9, 2017 at 11:52 PM, Numan Siddique  wrote:
>> Thanks for the review. Please see inline.
>>
>>
>> On Fri, Mar 10, 2017 at 1:44 AM, Russell Bryant  wrote:
>>>
>>> On Mon, Feb 27, 2017 at 12:59 AM,   wrote:
>>> > From: Numan Siddique 
>>> >
>>> > Presently, the icmp4 requests to the router gateway ip are sent to the
>>> > connectiont tracker, but the icmp4 reply packets responded by
>>> > 'lr_in_ip_input' stage are not sent to the connection tracker.
>>> > Also no zone ids are assigned for the router ports. Because of which
>>> > the icmp4 request packets in the connection tracker will be in the
>>> > UNREPLIED state. If the CMS has added ACLs to drop packets which
>>> > are not in ESTABLISHED state, the icmp4 replies will be dropped.
>>> >
>>> > To fix this issue, this patch adds a priority-110 flow in
>>> > 'ls_in_pre_acl'
>>> > stage which doesn't set reg0[0] = 1 for icmp4 request packets destined
>>> > to the router gateway ips.
>>> >
>>> > Alternate solution would be to assign zone ids for the router ports
>>> > and send the packets from the router ports to the connection tracker.
>>> >
>>> > The approach used in this patch seems to be simpler.
>>> >
>>> > Signed-off-by: Numan Siddique 
>>> > ---
>>> >  ovn/northd/ovn-northd.8.xml | 29 +++---
>>> >  ovn/northd/ovn-northd.c | 92
>>> > +++--
>>> >  2 files changed, 79 insertions(+), 42 deletions(-)
>>>
>>>
>>> Can you clarify where the packet gets dropped?  It seems like we have
>>> flows trying to handle this already.  We skip conntrack for the router
>>> interface ports.  Roughly, I would expect:
>>>
>>
>> The packets are getting dropped because of the flow
>>
>> table=52, n_packets=40, n_bytes=3920,
>> priority=2001,ct_state=-est+trk,ip,reg15=0x3,metadata=0x3 ac
>> tions=drop
>>
>> This flow corresponds to logical flow -
>> table=4 (ls_out_acl ), priority=2001 , match=((!ct.est || (ct.est &&
>> ct_label.blocked == 1)) && (outport ==
>> "ce575934-f308-45b8-b9cd-457646da213d" && ip)), action=(drop;)
>>
>>
>> This logical flow is added by neutron OVN plugin when the port is configured
>> with security groups. When I clear the security groups for the port or add a
>> specific security group rule to allow icmp, it works fine.
>
> The above flow could be hit at two different points (#2 and #5 below).
> In my local testing, it looks like it's happening in #5 so at least we
> aren't hitting conntrack related flows in a part of the pipeline where
> we don't expect it.
>
>>
>>>
>>> 1) inport == lport1, logical switch ingress pipeline.  packet sent
>>> through conntrack.
>>>
>>> 2) outport == router interface port, logical switch egress pipeline.
>>> packet *skips* conntrack since it's a router interface.
>>>
>>> 3) router datapath, icmp respose generated, sent back to logical switch
>>> ...
>>>
>>> 4) inport == router interface, logical switch ingress pipeline, packet
>>> *skips* conntrack since it's a router interface
>>>
>>> 5) outport == lport1, logical switch egress pipeline, packet sent
>>> through conntrack, which should find an existing conntrack entry
>>> established in step 1.  packet delivered to lport1.
>>>
>>
>> The connection tracking entry has this
>>
>> $ sudo conntrack -L | grep 10.0.0.1
>> conntrack v1.4.3 (conntrack-tools): 54 flow entries have been shown.
>> icmp 1 29 src=10.0.0.6 dst=10.0.0.1 type=8 code=0 id=7486 [UNREPLIED]
>> src=10.0.0.1 dst=10.0.0.6 type=0 code=0 id=7486 mark=0
>> secctx=system_u:object_r:unlabeled_t:s0 zone=1 use=1
>>
>> You think this should be addressed by neutron OVN plugin ?
>
> I don't think it's a Neutron issue.
>
> I see the conntrack entry remaining in the UNREPLIED state, even in
> the working case where there's not an ACL dropping the reply.
>
> icmp 1 29 src=10.0.0.10 dst=10.0.0.1 type=8 code=0 id=25857
> [UNREPLIED] src=10.0.0.1 dst=10.0.0.10 type=0 code=0 id=25857 mark=0
> zone=8 use=1
>
> If I ping a different address (something past the logical router), I
> see a proper conntrack entry that's not in the UNREPLIED state.
>
> I wonder if there's something about how we are generating the ICMP
> response from the logical router that's making conntrack not properly
> associate it with the request?

I checked into this and there's no meaningful difference in how we
form the ICMP reply.

I'm guessing this has to do with the request and reply both going
through conntrack as a part of processing the same packet in the OVS
data path.  That's not behaving how we would expect.  I'll keep
looking next week to try to identify the root cause here, but I would
appreciate any insight others may have about the behavior we should
expect in this scenario.

-- 
Russell Bryant
___
dev mailing list
d...@openvswitch.org

Re: [ovs-dev] [PATCH] ovn-northd: Skip icmp4 packets destined for router ports from conntrack

[Russell Bryant]

On Thu, Mar 9, 2017 at 11:52 PM, Numan Siddique  wrote:
> Thanks for the review. Please see inline.
>
>
> On Fri, Mar 10, 2017 at 1:44 AM, Russell Bryant  wrote:
>>
>> On Mon, Feb 27, 2017 at 12:59 AM,   wrote:
>> > From: Numan Siddique 
>> >
>> > Presently, the icmp4 requests to the router gateway ip are sent to the
>> > connectiont tracker, but the icmp4 reply packets responded by
>> > 'lr_in_ip_input' stage are not sent to the connection tracker.
>> > Also no zone ids are assigned for the router ports. Because of which
>> > the icmp4 request packets in the connection tracker will be in the
>> > UNREPLIED state. If the CMS has added ACLs to drop packets which
>> > are not in ESTABLISHED state, the icmp4 replies will be dropped.
>> >
>> > To fix this issue, this patch adds a priority-110 flow in
>> > 'ls_in_pre_acl'
>> > stage which doesn't set reg0[0] = 1 for icmp4 request packets destined
>> > to the router gateway ips.
>> >
>> > Alternate solution would be to assign zone ids for the router ports
>> > and send the packets from the router ports to the connection tracker.
>> >
>> > The approach used in this patch seems to be simpler.
>> >
>> > Signed-off-by: Numan Siddique 
>> > ---
>> >  ovn/northd/ovn-northd.8.xml | 29 +++---
>> >  ovn/northd/ovn-northd.c | 92
>> > +++--
>> >  2 files changed, 79 insertions(+), 42 deletions(-)
>>
>>
>> Can you clarify where the packet gets dropped?  It seems like we have
>> flows trying to handle this already.  We skip conntrack for the router
>> interface ports.  Roughly, I would expect:
>>
>
> The packets are getting dropped because of the flow
>
> table=52, n_packets=40, n_bytes=3920,
> priority=2001,ct_state=-est+trk,ip,reg15=0x3,metadata=0x3 ac
> tions=drop
>
> This flow corresponds to logical flow -
> table=4 (ls_out_acl ), priority=2001 , match=((!ct.est || (ct.est &&
> ct_label.blocked == 1)) && (outport ==
> "ce575934-f308-45b8-b9cd-457646da213d" && ip)), action=(drop;)
>
>
> This logical flow is added by neutron OVN plugin when the port is configured
> with security groups. When I clear the security groups for the port or add a
> specific security group rule to allow icmp, it works fine.

The above flow could be hit at two different points (#2 and #5 below).
In my local testing, it looks like it's happening in #5 so at least we
aren't hitting conntrack related flows in a part of the pipeline where
we don't expect it.

>
>>
>> 1) inport == lport1, logical switch ingress pipeline.  packet sent
>> through conntrack.
>>
>> 2) outport == router interface port, logical switch egress pipeline.
>> packet *skips* conntrack since it's a router interface.
>>
>> 3) router datapath, icmp respose generated, sent back to logical switch
>> ...
>>
>> 4) inport == router interface, logical switch ingress pipeline, packet
>> *skips* conntrack since it's a router interface
>>
>> 5) outport == lport1, logical switch egress pipeline, packet sent
>> through conntrack, which should find an existing conntrack entry
>> established in step 1.  packet delivered to lport1.
>>
>
> The connection tracking entry has this
>
> $ sudo conntrack -L | grep 10.0.0.1
> conntrack v1.4.3 (conntrack-tools): 54 flow entries have been shown.
> icmp 1 29 src=10.0.0.6 dst=10.0.0.1 type=8 code=0 id=7486 [UNREPLIED]
> src=10.0.0.1 dst=10.0.0.6 type=0 code=0 id=7486 mark=0
> secctx=system_u:object_r:unlabeled_t:s0 zone=1 use=1
>
> You think this should be addressed by neutron OVN plugin ?

I don't think it's a Neutron issue.

I see the conntrack entry remaining in the UNREPLIED state, even in
the working case where there's not an ACL dropping the reply.

icmp 1 29 src=10.0.0.10 dst=10.0.0.1 type=8 code=0 id=25857
[UNREPLIED] src=10.0.0.1 dst=10.0.0.10 type=0 code=0 id=25857 mark=0
zone=8 use=1

If I ping a different address (something past the logical router), I
see a proper conntrack entry that's not in the UNREPLIED state.

I wonder if there's something about how we are generating the ICMP
response from the logical router that's making conntrack not properly
associate it with the request?

-- 
Russell Bryant
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn-northd: Skip icmp4 packets destined for router ports from conntrack

[Numan Siddique]

On Fri, Mar 10, 2017 at 10:22 AM, Numan Siddique 
wrote:

> Thanks for the review. Please see inline.
>
>
> On Fri, Mar 10, 2017 at 1:44 AM, Russell Bryant  wrote:
>
>> On Mon, Feb 27, 2017 at 12:59 AM,   wrote:
>> > From: Numan Siddique 
>> >
>> > Presently, the icmp4 requests to the router gateway ip are sent to the
>> > connectiont tracker, but the icmp4 reply packets responded by
>> > 'lr_in_ip_input' stage are not sent to the connection tracker.
>> > Also no zone ids are assigned for the router ports. Because of which
>> > the icmp4 request packets in the connection tracker will be in the
>> > UNREPLIED state.
>
>
​​Also, this commit message is not accurate.
​

> If the CMS has added ACLs to drop packets which
>> > are not in ESTABLISHED state, the icmp4 replies will be dropped.
>> >
>> > To fix this issue, this patch adds a priority-110 flow in
>> 'ls_in_pre_acl'
>> > stage which doesn't set reg0[0] = 1 for icmp4 request packets destined
>> > to the router gateway ips.
>
> >
>> > Alternate solution would be to assign zone ids for the router ports
>> > and send the packets from the router ports to the connection tracker.
>> >
>> > The approach used in this patch seems to be simpler.
>> >
>> > Signed-off-by: Numan Siddique 
>> > ---
>> >  ovn/northd/ovn-northd.8.xml | 29 +++---
>> >  ovn/northd/ovn-northd.c | 92 +++---
>> ---
>> >  2 files changed, 79 insertions(+), 42 deletions(-)
>>
>>
>> Can you clarify where the packet gets dropped?  It seems like we have
>> flows trying to handle this already.  We skip conntrack for the router
>> interface ports.  Roughly, I would expect:
>>
>>
> ​
> ​The packets are getting dropped because of the flow
>
> ​
> table=52, n_packets=40, n_bytes=3920,  
> priority=2001,ct_state=-est+trk,ip,reg15=0x3,metadata=0x3
> ac
> tions=drop
>
> This flow corresponds to logical flow -
> table=4 (ls_out_acl ), priority=2001 , match=((!ct.est || (ct.est
> && ct_label.blocked == 1)) && (outport == 
> "ce575934-f308-45b8-b9cd-457646da213d"
> && ip)), action=(drop;)
> ​
>
>
> ​This logical flow is added by neutron OVN plugin when the port is
> configured with security groups. When I clear the security groups for the
> port or add a specific security group rule to allow icmp, it works fine.
> ​
>
>
>> 1) inport == lport1, logical switch ingress pipeline.  packet sent
>> through conntrack.
>>
>> 2) outport == router interface port, logical switch egress pipeline.
>> packet *skips* conntrack since it's a router interface.
>>
>> 3) router datapath, icmp respose generated, sent back to logical switch
>> ...
>>
>> 4) inport == router interface, logical switch ingress pipeline, packet
>> *skips* conntrack since it's a router interface
>>
>> 5) outport == lport1, logical switch egress pipeline, packet sent
>> through conntrack, which should find an existing conntrack entry
>> established in step 1.  packet delivered to lport1.
>>
>>
> The connection tracking entry ​has this
>
> $ sudo conntrack -L | grep 10.0.0.1
> conntrack v1.4.3 (conntrack-tools): 54 flow entries have been shown.
> icmp 1 29 src=10.0.0.6 dst=10.0.0.1 type=8 code=0 id=7486 [UNREPLIED]
> src=10.0.0.1 dst=10.0.0.6 type=0 code=0 id=7486 mark=0
> secctx=system_u:object_r:unlabeled_t:s0 zone=1 use=1
>
> You think this should be addressed by neutron OVN plugin ?
>
>
>
> Where in the above process is it coming apart?  If it's broken, it
>> sounds like a more general problem than ICMP.  It would be any type of
>> traffic to the router IP where we expect a response.
>>
>> --
>> Russell Bryant
>>
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn-northd: Skip icmp4 packets destined for router ports from conntrack

[Numan Siddique]

Thanks for the review. Please see inline.


On Fri, Mar 10, 2017 at 1:44 AM, Russell Bryant  wrote:

> On Mon, Feb 27, 2017 at 12:59 AM,   wrote:
> > From: Numan Siddique 
> >
> > Presently, the icmp4 requests to the router gateway ip are sent to the
> > connectiont tracker, but the icmp4 reply packets responded by
> > 'lr_in_ip_input' stage are not sent to the connection tracker.
> > Also no zone ids are assigned for the router ports. Because of which
> > the icmp4 request packets in the connection tracker will be in the
> > UNREPLIED state. If the CMS has added ACLs to drop packets which
> > are not in ESTABLISHED state, the icmp4 replies will be dropped.
> >
> > To fix this issue, this patch adds a priority-110 flow in 'ls_in_pre_acl'
> > stage which doesn't set reg0[0] = 1 for icmp4 request packets destined
> > to the router gateway ips.
> >
> > Alternate solution would be to assign zone ids for the router ports
> > and send the packets from the router ports to the connection tracker.
> >
> > The approach used in this patch seems to be simpler.
> >
> > Signed-off-by: Numan Siddique 
> > ---
> >  ovn/northd/ovn-northd.8.xml | 29 +++---
> >  ovn/northd/ovn-northd.c | 92 +++---
> ---
> >  2 files changed, 79 insertions(+), 42 deletions(-)
>
>
> Can you clarify where the packet gets dropped?  It seems like we have
> flows trying to handle this already.  We skip conntrack for the router
> interface ports.  Roughly, I would expect:
>
>
​
​The packets are getting dropped because of the flow

​
table=52, n_packets=40, n_bytes=3920,
 priority=2001,ct_state=-est+trk,ip,reg15=0x3,metadata=0x3 ac
tions=drop

This flow corresponds to logical flow -
table=4 (ls_out_acl ), priority=2001 , match=((!ct.est || (ct.est
&& ct_label.blocked == 1)) && (outport ==
"ce575934-f308-45b8-b9cd-457646da213d" && ip)), action=(drop;)
​


​This logical flow is added by neutron OVN plugin when the port is
configured with security groups. When I clear the security groups for the
port or add a specific security group rule to allow icmp, it works fine.
​


> 1) inport == lport1, logical switch ingress pipeline.  packet sent
> through conntrack.
>
> 2) outport == router interface port, logical switch egress pipeline.
> packet *skips* conntrack since it's a router interface.
>
> 3) router datapath, icmp respose generated, sent back to logical switch ...
>
> 4) inport == router interface, logical switch ingress pipeline, packet
> *skips* conntrack since it's a router interface
>
> 5) outport == lport1, logical switch egress pipeline, packet sent
> through conntrack, which should find an existing conntrack entry
> established in step 1.  packet delivered to lport1.
>
>
The connection tracking entry ​has this

$ sudo conntrack -L | grep 10.0.0.1
conntrack v1.4.3 (conntrack-tools): 54 flow entries have been shown.
icmp 1 29 src=10.0.0.6 dst=10.0.0.1 type=8 code=0 id=7486 [UNREPLIED]
src=10.0.0.1 dst=10.0.0.6 type=0 code=0 id=7486 mark=0
secctx=system_u:object_r:unlabeled_t:s0 zone=1 use=1

You think this should be addressed by neutron OVN plugin ?



Where in the above process is it coming apart?  If it's broken, it
> sounds like a more general problem than ICMP.  It would be any type of
> traffic to the router IP where we expect a response.
>
> --
> Russell Bryant
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn-northd: Skip icmp4 packets destined for router ports from conntrack

[Russell Bryant]

On Mon, Feb 27, 2017 at 12:59 AM,   wrote:
> From: Numan Siddique 
>
> Presently, the icmp4 requests to the router gateway ip are sent to the
> connectiont tracker, but the icmp4 reply packets responded by
> 'lr_in_ip_input' stage are not sent to the connection tracker.
> Also no zone ids are assigned for the router ports. Because of which
> the icmp4 request packets in the connection tracker will be in the
> UNREPLIED state. If the CMS has added ACLs to drop packets which
> are not in ESTABLISHED state, the icmp4 replies will be dropped.
>
> To fix this issue, this patch adds a priority-110 flow in 'ls_in_pre_acl'
> stage which doesn't set reg0[0] = 1 for icmp4 request packets destined
> to the router gateway ips.
>
> Alternate solution would be to assign zone ids for the router ports
> and send the packets from the router ports to the connection tracker.
>
> The approach used in this patch seems to be simpler.
>
> Signed-off-by: Numan Siddique 
> ---
>  ovn/northd/ovn-northd.8.xml | 29 +++---
>  ovn/northd/ovn-northd.c | 92 
> +++--
>  2 files changed, 79 insertions(+), 42 deletions(-)


Can you clarify where the packet gets dropped?  It seems like we have
flows trying to handle this already.  We skip conntrack for the router
interface ports.  Roughly, I would expect:

1) inport == lport1, logical switch ingress pipeline.  packet sent
through conntrack.

2) outport == router interface port, logical switch egress pipeline.
packet *skips* conntrack since it's a router interface.

3) router datapath, icmp respose generated, sent back to logical switch ...

4) inport == router interface, logical switch ingress pipeline, packet
*skips* conntrack since it's a router interface

5) outport == lport1, logical switch egress pipeline, packet sent
through conntrack, which should find an existing conntrack entry
established in step 1.  packet delivered to lport1.

Where in the above process is it coming apart?  If it's broken, it
sounds like a more general problem than ICMP.  It would be any type of
traffic to the router IP where we expect a response.

-- 
Russell Bryant
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn-northd: Skip icmp4 packets destined for router ports from conntrack

[Numan Siddique]

Thanks for the review and comments.


On Thu, Mar 9, 2017 at 10:00 AM, Darrell Ball  wrote:

> Daniele and I discussed
>
> 1) Seems ok in that there is security at the VM LP so weakening the
> Check at the router port for ICMP seems ok.
> 2) The same applies to V6 ?
>

​I need to test this before confirming.

​


>
> Thanks
>
>
> On 3/8/17, 1:32 PM, "ovs-dev-boun...@openvswitch.org on behalf of Ben
> Pfaff"  wrote:
>
> Hi Darrell and Daniele, do one of you have an opinion on whether this
> is
> the right approach?
>
> Thanks,
>
> Ben.
>
> On Mon, Feb 27, 2017 at 11:29:14AM +0530, nusid...@redhat.com wrote:
> > From: Numan Siddique 
> >
> > Presently, the icmp4 requests to the router gateway ip are sent to
> the
> > connectiont tracker, but the icmp4 reply packets responded by
> > 'lr_in_ip_input' stage are not sent to the connection tracker.
> > Also no zone ids are assigned for the router ports. Because of which
> > the icmp4 request packets in the connection tracker will be in the
> > UNREPLIED state. If the CMS has added ACLs to drop packets which
> > are not in ESTABLISHED state, the icmp4 replies will be dropped.
> >
> > To fix this issue, this patch adds a priority-110 flow in
> 'ls_in_pre_acl'
> > stage which doesn't set reg0[0] = 1 for icmp4 request packets
> destined
> > to the router gateway ips.
> >
> > Alternate solution would be to assign zone ids for the router ports
> > and send the packets from the router ports to the connection tracker.
> >
> > The approach used in this patch seems to be simpler.
> >
> > Signed-off-by: Numan Siddique 
> > ---
> >  ovn/northd/ovn-northd.8.xml | 29 +++---
> >  ovn/northd/ovn-northd.c | 92 +++---
> ---
> >  2 files changed, 79 insertions(+), 42 deletions(-)
> >
> > diff --git a/ovn/northd/ovn-northd.8.xml
> b/ovn/northd/ovn-northd.8.xml
> > index ab8fd88..06465ff 100644
> > --- a/ovn/northd/ovn-northd.8.xml
> > +++ b/ovn/northd/ovn-northd.8.xml
> > @@ -245,11 +245,30 @@
> >  
> >This table prepares flows for possible stateful ACL
> processing in
> >ingress table ACLs.  It contains a priority-0
> flow that
> > -  simply moves traffic to the next table.  If stateful ACLs are
> used in the
> > -  logical datapath, a priority-100 flow is added that sets a
> hint
> > -  (with reg0[0] = 1; next;) for table
> > -  Pre-stateful to send IP packets to the
> connection tracker
> > -  before eventually advancing to ingress table
> ACLs.
> > +  simply moves traffic to the next table. It adds the following
> flows if
> > +  stateful ACLs are used in the logical datapath.
> > +
> > +  
> > +
> > +  A priority-100 flow is added that sets a hint
> > +  (with reg0[0] = 1; next;) for table
> > +  Pre-stateful to send IP packets to the
> connection tracker
> > +  before eventually advancing to ingress table
> ACLs.
> > +
> > +
> > +
> > +  A priority-110 flow which doesn't set the connection
> tracking hint for
> > +  the packets from the router ports.
>
> This above comment is not needed as there is only the below flow added.
>

"build_pre_acls" adds the above flow (though this patch doesn't add this
flow and the comment is somewhat unrelated to this patch), but the
documentation is missing. I can spin another patch if it's not fine to have
this change in this patch.

https://github.com/openvswitch/ovs/blob/master/ovn/northd/ovn-northd.c#L2460



>
> > +
> > +
> > +
> > +  A priority-110 flow which doesn't set the connection
> tracking hint
> > +  for the packets with the match
> > +  ip4  icmp4  ip4.dst =
> {R}
> > +  where R is the IPv4 address(es) of the router
> ports
> > +  connected to the logical datapath.
> > +
> > +  
> >  
> >
> >  Ingress Table 4: Pre-LB
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index 03dc850..4fc86f4 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -2339,6 +2339,43 @@ has_stateful_acl(struct ovn_datapath *od)
> >  }
> >
> >  static void
> > +op_put_v4_networks(struct ds *ds, const struct ovn_port *op, bool
> add_bcast)
> > +{
> > +if (!add_bcast && op->lrp_networks.n_ipv4_addrs == 1) {
> > +ds_put_format(ds, "%s", op->lrp_networks.ipv4_addrs[0]
> .addr_s);
> > +return;
> > +}
> > +
> > +ds_put_cstr(ds, "{");
> > +for (int i = 0; i < 

Re: [ovs-dev] [PATCH] ovn-northd: Skip icmp4 packets destined for router ports from conntrack

[Russell Bryant]

I'm also looking at this one.  I was trying to review today, but have
been slowed down by getting an OpenStack test environment working for
testing this and looking closer.

On Wed, Mar 8, 2017 at 4:32 PM, Ben Pfaff  wrote:
> Hi Darrell and Daniele, do one of you have an opinion on whether this is
> the right approach?
>
> Thanks,
>
> Ben.
>
> On Mon, Feb 27, 2017 at 11:29:14AM +0530, nusid...@redhat.com wrote:
>> From: Numan Siddique 
>>
>> Presently, the icmp4 requests to the router gateway ip are sent to the
>> connectiont tracker, but the icmp4 reply packets responded by
>> 'lr_in_ip_input' stage are not sent to the connection tracker.
>> Also no zone ids are assigned for the router ports. Because of which
>> the icmp4 request packets in the connection tracker will be in the
>> UNREPLIED state. If the CMS has added ACLs to drop packets which
>> are not in ESTABLISHED state, the icmp4 replies will be dropped.
>>
>> To fix this issue, this patch adds a priority-110 flow in 'ls_in_pre_acl'
>> stage which doesn't set reg0[0] = 1 for icmp4 request packets destined
>> to the router gateway ips.
>>
>> Alternate solution would be to assign zone ids for the router ports
>> and send the packets from the router ports to the connection tracker.
>>
>> The approach used in this patch seems to be simpler.
>>
>> Signed-off-by: Numan Siddique 
>> ---
>>  ovn/northd/ovn-northd.8.xml | 29 +++---
>>  ovn/northd/ovn-northd.c | 92 
>> +++--
>>  2 files changed, 79 insertions(+), 42 deletions(-)
>>
>> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
>> index ab8fd88..06465ff 100644
>> --- a/ovn/northd/ovn-northd.8.xml
>> +++ b/ovn/northd/ovn-northd.8.xml
>> @@ -245,11 +245,30 @@
>>  
>>This table prepares flows for possible stateful ACL processing in
>>ingress table ACLs.  It contains a priority-0 flow that
>> -  simply moves traffic to the next table.  If stateful ACLs are used in 
>> the
>> -  logical datapath, a priority-100 flow is added that sets a hint
>> -  (with reg0[0] = 1; next;) for table
>> -  Pre-stateful to send IP packets to the connection tracker
>> -  before eventually advancing to ingress table ACLs.
>> +  simply moves traffic to the next table. It adds the following flows if
>> +  stateful ACLs are used in the logical datapath.
>> +
>> +  
>> +
>> +  A priority-100 flow is added that sets a hint
>> +  (with reg0[0] = 1; next;) for table
>> +  Pre-stateful to send IP packets to the connection 
>> tracker
>> +  before eventually advancing to ingress table ACLs.
>> +
>> +
>> +
>> +  A priority-110 flow which doesn't set the connection tracking 
>> hint for
>> +  the packets from the router ports.
>> +
>> +
>> +
>> +  A priority-110 flow which doesn't set the connection tracking hint
>> +  for the packets with the match
>> +  ip4  icmp4  ip4.dst = 
>> {R}
>> +  where R is the IPv4 address(es) of the router ports
>> +  connected to the logical datapath.
>> +
>> +  
>>  
>>
>>  Ingress Table 4: Pre-LB
>> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
>> index 03dc850..4fc86f4 100644
>> --- a/ovn/northd/ovn-northd.c
>> +++ b/ovn/northd/ovn-northd.c
>> @@ -2339,6 +2339,43 @@ has_stateful_acl(struct ovn_datapath *od)
>>  }
>>
>>  static void
>> +op_put_v4_networks(struct ds *ds, const struct ovn_port *op, bool add_bcast)
>> +{
>> +if (!add_bcast && op->lrp_networks.n_ipv4_addrs == 1) {
>> +ds_put_format(ds, "%s", op->lrp_networks.ipv4_addrs[0].addr_s);
>> +return;
>> +}
>> +
>> +ds_put_cstr(ds, "{");
>> +for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
>> +ds_put_format(ds, "%s, ", op->lrp_networks.ipv4_addrs[i].addr_s);
>> +if (add_bcast) {
>> +ds_put_format(ds, "%s, ", 
>> op->lrp_networks.ipv4_addrs[i].bcast_s);
>> +}
>> +}
>> +ds_chomp(ds, ' ');
>> +ds_chomp(ds, ',');
>> +ds_put_cstr(ds, "}");
>> +}
>> +
>> +static void
>> +op_put_v6_networks(struct ds *ds, const struct ovn_port *op)
>> +{
>> +if (op->lrp_networks.n_ipv6_addrs == 1) {
>> +ds_put_format(ds, "%s", op->lrp_networks.ipv6_addrs[0].addr_s);
>> +return;
>> +}
>> +
>> +ds_put_cstr(ds, "{");
>> +for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
>> +ds_put_format(ds, "%s, ", op->lrp_networks.ipv6_addrs[i].addr_s);
>> +}
>> +ds_chomp(ds, ' ');
>> +ds_chomp(ds, ',');
>> +ds_put_cstr(ds, "}");
>> +}
>> +
>> +static void
>>  build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
>>  {
>>  bool has_stateful = has_stateful_acl(od);
>> @@ -2378,6 +2415,24 @@ build_pre_acls(struct ovn_datapath *od, struct hmap 
>> *lflows)
>>
>>  ds_destroy(_in);

Re: [ovs-dev] [PATCH] ovn-northd: Skip icmp4 packets destined for router ports from conntrack

[Ben Pfaff]

Hi Darrell and Daniele, do one of you have an opinion on whether this is
the right approach?

Thanks,

Ben.

On Mon, Feb 27, 2017 at 11:29:14AM +0530, nusid...@redhat.com wrote:
> From: Numan Siddique 
> 
> Presently, the icmp4 requests to the router gateway ip are sent to the
> connectiont tracker, but the icmp4 reply packets responded by
> 'lr_in_ip_input' stage are not sent to the connection tracker.
> Also no zone ids are assigned for the router ports. Because of which
> the icmp4 request packets in the connection tracker will be in the
> UNREPLIED state. If the CMS has added ACLs to drop packets which
> are not in ESTABLISHED state, the icmp4 replies will be dropped.
> 
> To fix this issue, this patch adds a priority-110 flow in 'ls_in_pre_acl'
> stage which doesn't set reg0[0] = 1 for icmp4 request packets destined
> to the router gateway ips.
> 
> Alternate solution would be to assign zone ids for the router ports
> and send the packets from the router ports to the connection tracker.
> 
> The approach used in this patch seems to be simpler.
> 
> Signed-off-by: Numan Siddique 
> ---
>  ovn/northd/ovn-northd.8.xml | 29 +++---
>  ovn/northd/ovn-northd.c | 92 
> +++--
>  2 files changed, 79 insertions(+), 42 deletions(-)
> 
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index ab8fd88..06465ff 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
> @@ -245,11 +245,30 @@
>  
>This table prepares flows for possible stateful ACL processing in
>ingress table ACLs.  It contains a priority-0 flow that
> -  simply moves traffic to the next table.  If stateful ACLs are used in 
> the
> -  logical datapath, a priority-100 flow is added that sets a hint
> -  (with reg0[0] = 1; next;) for table
> -  Pre-stateful to send IP packets to the connection tracker
> -  before eventually advancing to ingress table ACLs.
> +  simply moves traffic to the next table. It adds the following flows if
> +  stateful ACLs are used in the logical datapath.
> +
> +  
> +
> +  A priority-100 flow is added that sets a hint
> +  (with reg0[0] = 1; next;) for table
> +  Pre-stateful to send IP packets to the connection 
> tracker
> +  before eventually advancing to ingress table ACLs.
> +
> +
> +
> +  A priority-110 flow which doesn't set the connection tracking hint 
> for
> +  the packets from the router ports.
> +
> +
> +
> +  A priority-110 flow which doesn't set the connection tracking hint
> +  for the packets with the match
> +  ip4  icmp4  ip4.dst = 
> {R}
> +  where R is the IPv4 address(es) of the router ports
> +  connected to the logical datapath.
> +
> +  
>  
>  
>  Ingress Table 4: Pre-LB
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 03dc850..4fc86f4 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -2339,6 +2339,43 @@ has_stateful_acl(struct ovn_datapath *od)
>  }
>  
>  static void
> +op_put_v4_networks(struct ds *ds, const struct ovn_port *op, bool add_bcast)
> +{
> +if (!add_bcast && op->lrp_networks.n_ipv4_addrs == 1) {
> +ds_put_format(ds, "%s", op->lrp_networks.ipv4_addrs[0].addr_s);
> +return;
> +}
> +
> +ds_put_cstr(ds, "{");
> +for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> +ds_put_format(ds, "%s, ", op->lrp_networks.ipv4_addrs[i].addr_s);
> +if (add_bcast) {
> +ds_put_format(ds, "%s, ", 
> op->lrp_networks.ipv4_addrs[i].bcast_s);
> +}
> +}
> +ds_chomp(ds, ' ');
> +ds_chomp(ds, ',');
> +ds_put_cstr(ds, "}");
> +}
> +
> +static void
> +op_put_v6_networks(struct ds *ds, const struct ovn_port *op)
> +{
> +if (op->lrp_networks.n_ipv6_addrs == 1) {
> +ds_put_format(ds, "%s", op->lrp_networks.ipv6_addrs[0].addr_s);
> +return;
> +}
> +
> +ds_put_cstr(ds, "{");
> +for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> +ds_put_format(ds, "%s, ", op->lrp_networks.ipv6_addrs[i].addr_s);
> +}
> +ds_chomp(ds, ' ');
> +ds_chomp(ds, ',');
> +ds_put_cstr(ds, "}");
> +}
> +
> +static void
>  build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
>  {
>  bool has_stateful = has_stateful_acl(od);
> @@ -2378,6 +2415,24 @@ build_pre_acls(struct ovn_datapath *od, struct hmap 
> *lflows)
>  
>  ds_destroy(_in);
>  ds_destroy(_out);
> +
> +/* The icmp4 request to the router ip (eg. ip4.dst = 10.0.0.1) 
> will
> + * be sent to the CT, but the reply from the router ip is not 
> sent
> + * to the CT. Also the zone id's are not assigned for the router
> + * ports. Because of which the icmp request packet in the 

[ovs-dev] [PATCH] ovn-northd: Skip icmp4 packets destined for router ports from conntrack

[nusiddiq]

From: Numan Siddique 

Presently, the icmp4 requests to the router gateway ip are sent to the
connectiont tracker, but the icmp4 reply packets responded by
'lr_in_ip_input' stage are not sent to the connection tracker.
Also no zone ids are assigned for the router ports. Because of which
the icmp4 request packets in the connection tracker will be in the
UNREPLIED state. If the CMS has added ACLs to drop packets which
are not in ESTABLISHED state, the icmp4 replies will be dropped.

To fix this issue, this patch adds a priority-110 flow in 'ls_in_pre_acl'
stage which doesn't set reg0[0] = 1 for icmp4 request packets destined
to the router gateway ips.

Alternate solution would be to assign zone ids for the router ports
and send the packets from the router ports to the connection tracker.

The approach used in this patch seems to be simpler.

Signed-off-by: Numan Siddique 
---
 ovn/northd/ovn-northd.8.xml | 29 +++---
 ovn/northd/ovn-northd.c | 92 +++--
 2 files changed, 79 insertions(+), 42 deletions(-)

diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index ab8fd88..06465ff 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -245,11 +245,30 @@
 
   This table prepares flows for possible stateful ACL processing in
   ingress table ACLs.  It contains a priority-0 flow that
-  simply moves traffic to the next table.  If stateful ACLs are used in the
-  logical datapath, a priority-100 flow is added that sets a hint
-  (with reg0[0] = 1; next;) for table
-  Pre-stateful to send IP packets to the connection tracker
-  before eventually advancing to ingress table ACLs.
+  simply moves traffic to the next table. It adds the following flows if
+  stateful ACLs are used in the logical datapath.
+
+  
+
+  A priority-100 flow is added that sets a hint
+  (with reg0[0] = 1; next;) for table
+  Pre-stateful to send IP packets to the connection 
tracker
+  before eventually advancing to ingress table ACLs.
+
+
+
+  A priority-110 flow which doesn't set the connection tracking hint 
for
+  the packets from the router ports.
+
+
+
+  A priority-110 flow which doesn't set the connection tracking hint
+  for the packets with the match
+  ip4  icmp4  ip4.dst = {R}
+  where R is the IPv4 address(es) of the router ports
+  connected to the logical datapath.
+
+  
 
 
 Ingress Table 4: Pre-LB
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 03dc850..4fc86f4 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -2339,6 +2339,43 @@ has_stateful_acl(struct ovn_datapath *od)
 }
 
 static void
+op_put_v4_networks(struct ds *ds, const struct ovn_port *op, bool add_bcast)
+{
+if (!add_bcast && op->lrp_networks.n_ipv4_addrs == 1) {
+ds_put_format(ds, "%s", op->lrp_networks.ipv4_addrs[0].addr_s);
+return;
+}
+
+ds_put_cstr(ds, "{");
+for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
+ds_put_format(ds, "%s, ", op->lrp_networks.ipv4_addrs[i].addr_s);
+if (add_bcast) {
+ds_put_format(ds, "%s, ", op->lrp_networks.ipv4_addrs[i].bcast_s);
+}
+}
+ds_chomp(ds, ' ');
+ds_chomp(ds, ',');
+ds_put_cstr(ds, "}");
+}
+
+static void
+op_put_v6_networks(struct ds *ds, const struct ovn_port *op)
+{
+if (op->lrp_networks.n_ipv6_addrs == 1) {
+ds_put_format(ds, "%s", op->lrp_networks.ipv6_addrs[0].addr_s);
+return;
+}
+
+ds_put_cstr(ds, "{");
+for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
+ds_put_format(ds, "%s, ", op->lrp_networks.ipv6_addrs[i].addr_s);
+}
+ds_chomp(ds, ' ');
+ds_chomp(ds, ',');
+ds_put_cstr(ds, "}");
+}
+
+static void
 build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
 {
 bool has_stateful = has_stateful_acl(od);
@@ -2378,6 +2415,24 @@ build_pre_acls(struct ovn_datapath *od, struct hmap 
*lflows)
 
 ds_destroy(_in);
 ds_destroy(_out);
+
+/* The icmp4 request to the router ip (eg. ip4.dst = 10.0.0.1) will
+ * be sent to the CT, but the reply from the router ip is not sent
+ * to the CT. Also the zone id's are not assigned for the router
+ * ports. Because of which the icmp request packet in the CT will
+ * be in the UNREPLIED state.
+ * The icmp4 reply packets could be dropped if the CMS has added
+ * ACLs to drop packets which are not in ct.est state.
+ * So, don't send the icpmp4 request packet for the router ips
+ * to the CT.
+ */
+if (op->peer && op->peer->lrp_networks.n_ipv4_addrs) {
+struct ds match = DS_EMPTY_INITIALIZER;
+