netdev - Re: Mirred broken WAS(Re: [PATCH net-next 2/2] act_mirred: use the backlog for nested calls to mirred ingress

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: Mon, 11 Dec 2023 10:50:40 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Davide Caratti <dcaratti@...hat.com>
Cc: Jiri Pirko <jiri@...nulli.us>, Xin Long <lucien.xin@...il.com>, 
	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>, 
	Linux Kernel Network Developers <netdev@...r.kernel.org>, Paolo Abeni <pabeni@...hat.com>, wizhao@...hat.com, 
	Cong Wang <xiyou.wangcong@...il.com>, Florian Westphal <fw@...len.de>
Subject: Re: Mirred broken WAS(Re: [PATCH net-next 2/2] act_mirred: use the
 backlog for nested calls to mirred ingress

Hi Davide,

On Mon, Dec 11, 2023 at 8:08 AM Davide Caratti <dcaratti@...hat.com> wrote:
>
> hello Jamal,
>
> On Thu, Dec 07, 2023 at 09:10:13AM -0500, Jamal Hadi Salim wrote:
> > Hi Davide,
> >
>
> [...]
>
> > > > > I am afraid this broke things. Here's a simple use case which causes
> > > > > an infinite loop (that we found while testing blockcasting but
> > > > > simplified to demonstrate the issue):
> > > >
> > > > [...]
> > > >
> > > > > sudo ip netns exec p4node tc qdisc add dev port0 clsact
> > > > > sudo ip netns exec p4node tc filter add dev port0 ingress protocol ip
> > > > > prio 10 matchall action mirred ingress redirect dev port0
> > > >
> > > > the above rule is taking packets from port0 ingress and putting it
> > > > again in the mirred ingress of the same device, hence the loop.
> > >
> > > Right - that was intentional to show the loop. We are worrying about
> > > extending mirred now to also broadcast (see the blockcast discussion)
> > > to more ports making the loop even worse. The loop should terminate at
> > > some point - in this case it does not...
> > >
> > > > I don't see it much different than what we can obtain with bridges:
> > > >
> > > > # ip link add name one type veth peer name two
> > > > # ip link add name three type veth peer name four
> > > > # for n in even odd; do ip link add name $n type bridge; done
> > > > # for n in one two three four even odd; do ip link set dev $n up; done
> > > > # for n in one three; do ip link set dev $n master odd; done
> > > > # for n in two four; do ip link set dev $n master even; done
> > > >
> > >
> > > Sure that is another way to reproduce.
> >
> > Ok, so i can verify that re-introduction of the ttl field in the
> > skb[1] fixes the issue. But restoring that patch may cause too much
> > bikeshedding. Victor will work on a better approach using the cb
> > struct instead - there may. Are you able to test with/out your patch
> > and see if this same patch fixes it?
>
> I'm also more optimistic on the use of qdisc cb for that purpose :)
> Just share the code, i will be happy to review/test.
> With regular TC mirred, the deadlock happened with TCP and SCTP socket
> locks, as they were sending an ACK back for a packet that was sent by
> the peer using egress->ingress.
>
> AFAIR there is a small reproducer in tc_actions.sh kselftest, namely
>
> mirred_egress_to_ingress_tcp_test()
>
> maybe it's useful for pre-verification also.
>

Ah, good - didnt realize there was a reproducer for your use case.
We'll try it out.

> [...]
>
> my 2 cents  below:
>
> > > I dont think we can run something equivalent inside the kernel. The
> > > ttl worked fine. BTW, the example shown breaks even when you have
> > > everything running on a single cpu (and packets being queued on the
> > > backlog)
>
> [...]
>
> > > Yes, we need to make sure those are fixed with whatever replacement..
> > > The loops will happen even on egress->egress (the example only showed
> > > ingress-ingress).
>
> if you try to make a loop using mirred egress/redirect, the first packet
> will trigger a deadlock on the root qdisc lock - see [1]. It's worse
> than a loop, because user can't fix it by just removing the "offending"
> mirred action. Would the ttl be helpful here?
>

Possible. So the test is just to create a loop?
Lets test with the reproducer you pointed out then see where we go from there.

> (in the meanwhile, I ill try to figure out if it's possible at least to
> silence false lockdep warnings without using dynamic keys, as per
> Eric reply).
>

Sorry, wasnt helpful - i have been in travel mode for the last week.

cheers,
jamal
> TIA!
>
> --
> davide
>
> [1] https://github.com/multipath-tcp/mptcp_net-next/issues/451#issuecomment-1782690200
>
>