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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 23 Apr 2022 19:08:01 +0300
From:   Eyal Birger <eyal.birger@...il.com>
To:     Marcelo Ricardo Leitner <mleitner@...hat.com>
Cc:     Hangbin Liu <liuhangbin@...il.com>, netdev@...r.kernel.org,
        jhs@...atatu.com, xiyou.wangcong@...il.com, jiri@...nulli.us,
        davem@...emloft.net, kuba@...nel.org, ahleihel@...hat.com,
        dcaratti@...hat.com, aconole@...hat.com, roid@...dia.com,
        Shmulik Ladkani <shmulik.ladkani@...il.com>
Subject: Re: [PATCH net] net: sched: act_mirred: Reset ct info when
 mirror/redirect skb

On Fri, Apr 22, 2022 at 4:41 PM Marcelo Ricardo Leitner
<mleitner@...hat.com> wrote:
>
> 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.

Makes sense to me too. The main reason for bringing this up was that it's a
subtle change and wasn't trivial to figure out.

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

FWIW the dst change was ok in our setups.

>
> >
> > 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.

For future reference, we worked around this issue by moving some of the
relevant ebpf functionality to the lwt output hook which allows classification
and rerouting.

Eyal.