Re: [PATCH net 1/2] net: sched: drop ct for the packets toward ingress only in act_mirred

From: mleitner@redhat.com
To: Xin Long <lucien.xin@gmail.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>,
	network dev <netdev@vger.kernel.org>,
	David Miller <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Davide Caratti <dcaratti@redhat.com>,
	Hangbin Liu <liuhangbin@gmail.com>, Jiri Pirko <jiri@resnulli.us>,
	Jamal Hadi Salim <jhs@mojatatu.com>
Subject: Re: [PATCH net 1/2] net: sched: drop ct for the packets toward ingress only in act_mirred
Date: Fri, 24 Sep 2021 16:04:27 -0700	[thread overview]
Message-ID: <CALnP8Za-nJHrVwd9dtzsvABEWH=bZaarEwUeOme9Pfu7iKJzsg@mail.gmail.com> (raw)
In-Reply-To: <CADvbK_dFyVdt3dU57_8=6eYH+bz3_M81=V4B_5sNd+kpXbnUHA@mail.gmail.com>

On Wed, Sep 22, 2021 at 01:17:05PM +0800, Xin Long wrote:
> On Wed, Sep 22, 2021 at 11:43 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Tue, Sep 21, 2021 at 12:02 AM Xin Long <lucien.xin@gmail.com> wrote:
> > >
> > > On Tue, Sep 21, 2021 at 2:31 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > >
> > > > On Mon, Sep 20, 2021 at 7:12 AM Xin Long <lucien.xin@gmail.com> 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?
> > > 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.
> 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.