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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 19 Apr 2022 19:50:38 +0300
From:   Eyal Birger <eyal.birger@...il.com>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     netdev@...r.kernel.org, jhs@...atatu.com, xiyou.wangcong@...il.com,
        jiri@...nulli.us, davem@...emloft.net, kuba@...nel.org,
        mleitner@...hat.com, 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

Hi,

On Mon, Aug 9, 2021 at 1:29 PM <patchwork-bot+netdevbpf@...nel.org> wrote:
>
> Hello:
>
> This patch was applied to netdev/net.git (refs/heads/master):
>
> 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.

Therefore I suggest this commit be reverted and a knob is introduced to mirred
for clearing ct as needed.

Eyal.

Reproduction script:

#!/bin/bash

ip netns add a
ip netns add b

ip netns exec a sysctl -w net.ipv4.conf.all.forwarding=1
ip netns exec a sysctl -w net.ipv4.conf.all.accept_local=1

ip link add veth0 netns a type veth peer name veth0 netns b
ip -net a link set veth0 up
ip -net a addr add dev veth0 198.51.100.1/30

ip -net a link add dum0 type dummy
ip -net a link set dev dum0 up
ip -net a addr add dev dum0 198.51.100.2/32

ip netns exec a iptables -t nat -I OUTPUT -d 10.0.0.1 -j DNAT
--to-destination 10.0.0.2
ip -net a route add default dev dum0
ip -net a rule add pref 50 iif dum0 lookup 1000
ip -net a route add table 1000 default dev veth0

ip netns exec a tc qdisc add dev dum0 clsact
ip netns exec a tc filter add dev dum0 parent ffff:fff3 prio 50 basic
action mirred ingress redirect dev dum0

ip -net b link set veth0 up
ip  -net b addr add 10.0.0.2/32 dev veth0
ip  -net b addr add 198.51.100.3/30 dev veth0

ip netns exec a ping 10.0.0.1
>
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html
>
>