Re: [PATCH net] net: sched: act_mirred: Reset ct info when mirror/redirect skb

From: Marcelo Ricardo Leitner <mleitner@redhat.com>
To: Hangbin Liu <liuhangbin@gmail.com>
Cc: Eyal Birger <eyal.birger@gmail.com>,
	netdev@vger.kernel.org, jhs@mojatatu.com,
	xiyou.wangcong@gmail.com, jiri@resnulli.us, davem@davemloft.net,
	kuba@kernel.org, ahleihel@redhat.com, dcaratti@redhat.com,
	aconole@redhat.com, roid@nvidia.com,
	Shmulik Ladkani <shmulik.ladkani@gmail.com>
Subject: Re: [PATCH net] net: sched: act_mirred: Reset ct info when mirror/redirect skb
Date: Fri, 22 Apr 2022 08:41:16 -0500	[thread overview]
Message-ID: <CALnP8ZY9hkiWyxjrVTdq=NFA0PYjt7f9YbSEJrbt-EQoRAk6gw@mail.gmail.com> (raw)
In-Reply-To: <YmE5N0aNisKVLAyt@Laptop-X1>

On Thu, Apr 21, 2022 at 07:00:07PM +0800, Hangbin Liu wrote:
> Hi Eyal,
> On Tue, Apr 19, 2022 at 09:14:38PM +0300, Eyal Birger wrote:
> > > > > On Mon,  9 Aug 2021 15:04:55 +0800 you wrote:
> > > > > > When mirror/redirect a skb to a different port, the ct info should be reset
> > > > > > for reclassification. Or the pkts will match unexpected rules. For example,
> > > > > > with following topology and commands:
> > > > > >
> > > > > >     -----------
> > > > > >               |
> > > > > >        veth0 -+-------
> > > > > >               |
> > > > > >        veth1 -+-------
> > > > > >               |
> > > > > >
> > > > > > [...]
> > > > >
> > > > > Here is the summary with links:
> > > > >   - [net] net: sched: act_mirred: Reset ct info when mirror/redirect skb
> > > > >     https://git.kernel.org/netdev/net/c/d09c548dbf3b
> > > >
> > > > Unfortunately this commit breaks DNAT when performed before going via mirred
> > > > egress->ingress.
> > > >
> > > > The reason is that connection tracking is lost and therefore a new state
> > > > is created on ingress.
> > > >
> > > > This breaks existing setups.
> > > >
> > > > See below a simplified script reproducing this issue.
>
> I think we come in to a paradox state. Some user don't want to have previous
> ct info after mirror, while others would like to keep. In my understanding,
> when we receive a pkt from a interface, the skb should be clean and no ct info
> at first. But I may wrong.

Makes sense to me. Moreover, there were a couple of fixes on this on
mirred around that time frame/area (like f799ada6bf23 ("net: sched:
act_mirred: drop dst for the direction from egress to ingress")). That's
because we are seeing that mirred xmit action when switching to
ingress direction should be as close skb_scrub_packet. OVS needs this
scrubbing as well, btw. This ct information could be easily stale if
there were other packet changes after it.

Point being, if we really need the knob for backwards compatibility
here, it may have to be a broader one.

>
> Jamal, Wang Cong, Jiri, do you have any comments?
>
> > >
> > > I guess I can understand why the reproducer triggers it, but I fail to
> > > see the actual use case you have behind it. Can you please elaborate
> > > on it?
> >
> > One use case we use mirred egress->ingress redirect for is when we want to
> > reroute a packet after applying some change to the packet which would affect
> > its routing. for example consider a bpf program running on tc ingress (after
> > mirred) setting the skb->mark based on some criteria.
> >
> > So you have something like:
> >
> > packet routed to dummy device based on some criteria ->
> >   mirred redirect to ingress ->
> >     classification by ebpf logic at tc ingress ->
> >        packet routed again
> >
> > We have a setup where DNAT is performed before this flow in that case the
> > ebpf logic needs to see the packet after the NAT.
>
> Is it possible to check whether it's need to set the skb->mark before DNAT?
> So we can update it before egress and no need to re-route.
>
> Thanks
> Hangbin
>