Message-ID: <ZXcJrNZxt5uY1sdn@dcaratti.users.ipa.redhat.com>
Date: Mon, 11 Dec 2023 14:07:56 +0100
From: Davide Caratti <dcaratti@...hat.com>
To: Jamal Hadi Salim <jhs@...atatu.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

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.

[...]

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?

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

TIA!

-- 
davide

[1] https://github.com/multipath-tcp/mptcp_net-next/issues/451#issuecomment-1782690200
 

Powered by blists - more mailing lists