[PATCH net 0/2] net: sched: drop ct and dst for the packets toward ingress in act_mirred

With the old ct and dst carried on these packets mirred or redirrected
to ingress, it may cause these packets to be dropped on rx path. This
issue could be easily triggered when using OVS HWOL on OVN-k8s. To fix
it this patchset is to drop ct and dst for these packets, just as it
does in internal_dev_recv() in OVS module.

Xin Long (2):
  net: sched: drop ct for the packets toward ingress only in act_mirred
  net: sched: also drop dst for the packets toward ingress in act_mirred

 net/sched/act_mirred.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

-- 
2.27.0

nf_reset_ct() called in tcf_mirred_act() is supposed to drop ct for
those packets that are mirred or redirected to only ingress, not
ingress and egress.

This patch is to move nf_reset_ct() to tcf_mirred_forward() and to
be called only when want_ingress is true.

Fixes: d09c548dbf3b ("net: sched: act_mirred: Reset ct info when mirror")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sched/act_mirred.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index d64b0eeccbe4..46dff1f1e7c8 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c

@@ -205,14 +205,12 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 
 static int tcf_mirred_forward(bool want_ingress, struct sk_buff *skb)
 {
-	int err;
-
 	if (!want_ingress)
-		err = tcf_dev_queue_xmit(skb, dev_queue_xmit);
-	else
-		err = netif_receive_skb(skb);
+		return tcf_dev_queue_xmit(skb, dev_queue_xmit);
 
-	return err;
+	nf_reset_ct(skb);
+
+	return netif_receive_skb(skb);
 }
 
 static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,

@@ -271,9 +269,6 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
 			goto out;
 	}
 
-	/* All mirred/redirected skbs should clear previous ct info */
-	nf_reset_ct(skb2);
-
 	want_ingress = tcf_mirred_act_wants_ingress(m_eaction);
 
 	expects_nh = want_ingress || !m_mac_header_xmit;

-- 
2.27.0

Without dropping dst, the packets sent from local mirred/redirected
to ingress will may still use the old dst. ip_rcv() will drop it as
the old dst is for output and its .input is dst_discard.

This patch is to fix by also dropping dst for those packets that are
mirred or redirected to ingress in act_mirred.

Fixes: b57dc7c13ea9 ("net/sched: Introduce action ct")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sched/act_mirred.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 46dff1f1e7c8..5e30b3e64b63 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c

@@ -19,6 +19,7 @@
 #include <linux/if_arp.h>
 #include <net/net_namespace.h>
 #include <net/netlink.h>
+#include <net/dst.h>
 #include <net/pkt_sched.h>
 #include <net/pkt_cls.h>
 #include <linux/tc_act/tc_mirred.h>

@@ -209,6 +210,7 @@ static int tcf_mirred_forward(bool want_ingress, struct sk_buff *skb)
 		return tcf_dev_queue_xmit(skb, dev_queue_xmit);
 
 	nf_reset_ct(skb);
+	skb_dst_drop(skb);
 
 	return netif_receive_skb(skb);
 }

-- 
2.27.0

On Mon, Sep 20, 2021 at 7:12 AM Xin Long [off-list ref] wrote:

nf_reset_ct() called in tcf_mirred_act() is supposed to drop ct for
those packets that are mirred or redirected to only ingress, not
ingress and egress.

Any reason behind this? I think we at least need to reset it when
redirecting from ingress to egress as well? That is, when changing
directions?

Thanks.

On Mon, Sep 20, 2021 at 7:12 AM Xin Long [off-list ref] wrote:

Without dropping dst, the packets sent from local mirred/redirected
to ingress will may still use the old dst. ip_rcv() will drop it as
the old dst is for output and its .input is dst_discard.

This patch is to fix by also dropping dst for those packets that are
mirred or redirected to ingress in act_mirred.

Similar question: what about redirecting from ingress to egress?

BTW, please CC TC maintainers for TC patches.

Thanks.

On Tue, Sep 21, 2021 at 2:31 AM Cong Wang [off-list ref] wrote:

On Mon, Sep 20, 2021 at 7:12 AM Xin Long [off-list ref] wrote:

quoted

nf_reset_ct() called in tcf_mirred_act() is supposed to drop ct for
those packets that are mirred or redirected to only ingress, not
ingress and egress.

Any reason behind this? I think we at least need to reset it when
redirecting from ingress to egress as well? That is, when changing
directions?

For the reason why ct should be reset, it's said in
d09c548dbf3b ("net: sched: act_mirred: Reset ct info when mirror").
The user case is OVS HWOL using TC to do NAT and then redirecting
the NATed skb back to the ingress of one local dev, it's ingress only, this
patch is more like to minimize the side effect of d09c548dbf3b IF there is.

Not sure if it's too much to do for that from ingress to egress.
What I was thinking is this should happen on rx path(ingress), like it
does in internal_dev_recv() in the OVS kernel module. But IF there is
any user case needing doing this for ingress to egress, I would add it.

Thanks.

On Tue, Sep 21, 2021 at 2:34 AM Cong Wang [off-list ref] wrote:

On Mon, Sep 20, 2021 at 7:12 AM Xin Long [off-list ref] wrote:

quoted

Without dropping dst, the packets sent from local mirred/redirected
to ingress will may still use the old dst. ip_rcv() will drop it as
the old dst is for output and its .input is dst_discard.

This patch is to fix by also dropping dst for those packets that are
mirred or redirected to ingress in act_mirred.

Similar question: what about redirecting from ingress to egress?

We can do it IF there's any user case needing it.
But for now, The problem I've met occurred in ip_rcv() for the user case.

BTW, please CC TC maintainers for TC patches.

added Jamal and Jiri.

Thanks.

Thanks.

On Tue, Sep 21, 2021 at 12:02 AM Xin Long [off-list ref] wrote:

On Tue, Sep 21, 2021 at 2:31 AM Cong Wang [off-list ref] wrote:

quoted

On Mon, Sep 20, 2021 at 7:12 AM Xin Long [off-list ref] wrote:

quoted

nf_reset_ct() called in tcf_mirred_act() is supposed to drop ct for
those packets that are mirred or redirected to only ingress, not
ingress and egress.

Any reason behind this? I think we at least need to reset it when
redirecting from ingress to egress as well? That is, when changing
directions?

For the reason why ct should be reset, it's said in
d09c548dbf3b ("net: sched: act_mirred: Reset ct info when mirror").
The user case is OVS HWOL using TC to do NAT and then redirecting
the NATed skb back to the ingress of one local dev, it's ingress only, this
patch is more like to minimize the side effect of d09c548dbf3b IF there is.

What is the side effect here? Or what is wrong with resetting CT on
egress side?

Not sure if it's too much to do for that from ingress to egress.
What I was thinking is this should happen on rx path(ingress), like it
does in internal_dev_recv() in the OVS kernel module. But IF there is
any user case needing doing this for ingress to egress, I would add it.

If that is the case, then this patch is completely unnecessary. So
instead of going back and forth, please elaborate on why resetting
CT for egress is a problem here.

Thanks.

On Tue, Sep 21, 2021 at 12:02 AM Xin Long [off-list ref] wrote:

On Tue, Sep 21, 2021 at 2:34 AM Cong Wang [off-list ref] wrote:

quoted

On Mon, Sep 20, 2021 at 7:12 AM Xin Long [off-list ref] wrote:

quoted

Without dropping dst, the packets sent from local mirred/redirected
to ingress will may still use the old dst. ip_rcv() will drop it as
the old dst is for output and its .input is dst_discard.

This patch is to fix by also dropping dst for those packets that are
mirred or redirected to ingress in act_mirred.

Similar question: what about redirecting from ingress to egress?

We can do it IF there's any user case needing it.
But for now, The problem I've met occurred in ip_rcv() for the user case.

I think input route is different from output route, so essentially we need
a reset when changing the direction, but I don't see any bugs so far,
except this one.

Thanks.

On Wed, Sep 22, 2021 at 11:43 AM Cong Wang [off-list ref] wrote:

On Tue, Sep 21, 2021 at 12:02 AM Xin Long [off-list ref] wrote:

quoted

On Tue, Sep 21, 2021 at 2:31 AM Cong Wang [off-list ref] wrote:

quoted

On Mon, Sep 20, 2021 at 7:12 AM Xin Long [off-list ref] wrote:

quoted

nf_reset_ct() called in tcf_mirred_act() is supposed to drop ct for
those packets that are mirred or redirected to only ingress, not
ingress and egress.

Any reason behind this? I think we at least need to reset it when
redirecting from ingress to egress as well? That is, when changing
directions?

For the reason why ct should be reset, it's said in
d09c548dbf3b ("net: sched: act_mirred: Reset ct info when mirror").
The user case is OVS HWOL using TC to do NAT and then redirecting
the NATed skb back to the ingress of one local dev, it's ingress only, this
patch is more like to minimize the side effect of d09c548dbf3b IF there is.

What is the side effect here? Or what is wrong with resetting CT on
egress side?

quoted

Not sure if it's too much to do for that from ingress to egress.
What I was thinking is this should happen on rx path(ingress), like it
does in internal_dev_recv() in the OVS kernel module. But IF there is
any user case needing doing this for ingress to egress, I would add it.

If that is the case, then this patch is completely unnecessary. So
instead of going back and forth, please elaborate on why resetting
CT for egress is a problem here.

What I'm afraid is: after resetting CT for the packets redirected to egress,
the tc rules on the next dev egress that may need these CT will break.

I can't give a use case right now, and just don't want to introduce extra
change for which we don't see a use. If you think that's safe, I'm fine
to do these resets when the direction is changed.

On Wed, Sep 22, 2021 at 11:53 AM Cong Wang [off-list ref] wrote:

On Tue, Sep 21, 2021 at 12:02 AM Xin Long [off-list ref] wrote:

quoted

On Tue, Sep 21, 2021 at 2:34 AM Cong Wang [off-list ref] wrote:

quoted

On Mon, Sep 20, 2021 at 7:12 AM Xin Long [off-list ref] wrote:

quoted

Without dropping dst, the packets sent from local mirred/redirected
to ingress will may still use the old dst. ip_rcv() will drop it as
the old dst is for output and its .input is dst_discard.

This patch is to fix by also dropping dst for those packets that are
mirred or redirected to ingress in act_mirred.

Similar question: what about redirecting from ingress to egress?

We can do it IF there's any user case needing it.
But for now, The problem I've met occurred in ip_rcv() for the user case.

I think input route is different from output route, so essentially we need
a reset when changing the direction, but I don't see any bugs so far,
except this one.

Yes, this one seems more reasonable to do this reset when changing
the direction.

Maybe because in common env, it rarely redirects an egress pkt to ingress by TC.
OVS does flow control quite flexibly(complicatedly), when it offloads NAT to TC,
this problem starts showing up.

+CC Shmulik.
Unfortunately we dont have good test cases in tdc to test different
scenarios of this setup (packets being redirected in both directions
once or several times).

cheers,
jamal

On 2021-09-21 11:52 p.m., Cong Wang wrote:

On Tue, Sep 21, 2021 at 12:02 AM Xin Long [off-list ref] wrote:

quoted

On Tue, Sep 21, 2021 at 2:34 AM Cong Wang [off-list ref] wrote:

quoted

On Mon, Sep 20, 2021 at 7:12 AM Xin Long [off-list ref] wrote:

quoted

Without dropping dst, the packets sent from local mirred/redirected
to ingress will may still use the old dst. ip_rcv() will drop it as
the old dst is for output and its .input is dst_discard.

This patch is to fix by also dropping dst for those packets that are
mirred or redirected to ingress in act_mirred.

Similar question: what about redirecting from ingress to egress?

We can do it IF there's any user case needing it.
But for now, The problem I've met occurred in ip_rcv() for the user case.

I think input route is different from output route, so essentially we need
a reset when changing the direction, but I don't see any bugs so far,
except this one.

Thanks.

On Wed, Sep 22, 2021 at 01:17:05PM +0800, Xin Long wrote:

On Wed, Sep 22, 2021 at 11:43 AM Cong Wang [off-list ref] wrote:

quoted

On Tue, Sep 21, 2021 at 12:02 AM Xin Long [off-list ref] wrote:

quoted

On Tue, Sep 21, 2021 at 2:31 AM Cong Wang [off-list ref] wrote:

quoted

On Mon, Sep 20, 2021 at 7:12 AM Xin Long [off-list ref] wrote:

quoted

nf_reset_ct() called in tcf_mirred_act() is supposed to drop ct for
those packets that are mirred or redirected to only ingress, not
ingress and egress.

Any reason behind this? I think we at least need to reset it when
redirecting from ingress to egress as well? That is, when changing
directions?

For the reason why ct should be reset, it's said in
d09c548dbf3b ("net: sched: act_mirred: Reset ct info when mirror").
The user case is OVS HWOL using TC to do NAT and then redirecting
the NATed skb back to the ingress of one local dev, it's ingress only, this
patch is more like to minimize the side effect of d09c548dbf3b IF there is.

What is the side effect here? Or what is wrong with resetting CT on
egress side?

quoted

Not sure if it's too much to do for that from ingress to egress.
What I was thinking is this should happen on rx path(ingress), like it
does in internal_dev_recv() in the OVS kernel module. But IF there is
any user case needing doing this for ingress to egress, I would add it.

If that is the case, then this patch is completely unnecessary. So
instead of going back and forth, please elaborate on why resetting
CT for egress is a problem here.

What I'm afraid is: after resetting CT for the packets redirected to egress,
the tc rules on the next dev egress that may need these CT will break.

This shouldn't happen. Well, it's not expected from OVS use case POV,
and that would probably be a nightmare to maintain (without something
like OVS), considering packets can be directed to egress from multiple
places and they would have different starting points then.

I can't give a use case right now, and just don't want to introduce extra
change for which we don't see a use. If you think that's safe, I'm fine
to do these resets when the direction is changed.

I don't see why it wouldn't be safe.