netdev - Re: [PATCH net v2 1/2] net/sched: act_mirred: use the backlog for 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zcx-9HkcmhDR5_r1@nanopsycho>
Date: Wed, 14 Feb 2024 09:51:00 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
	pabeni@...hat.com,
	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
	Davide Caratti <dcaratti@...hat.com>, jhs@...atatu.com,
	xiyou.wangcong@...il.com, shmulik.ladkani@...il.com
Subject: Re: [PATCH net v2 1/2] net/sched: act_mirred: use the backlog for
 mirred ingress

Wed, Feb 14, 2024 at 04:38:47AM CET, kuba@...nel.org wrote:
>The test Davide added in commit ca22da2fbd69 ("act_mirred: use the backlog
>for nested calls to mirred ingress") hangs our testing VMs every 10 or so
>runs, with the familiar tcp_v4_rcv -> tcp_v4_rcv deadlock reported by
>lockdep.
>
>In the past there was a concern that the backlog indirection will
>lead to loss of error reporting / less accurate stats. But the current
>workaround does not seem to address the issue.

Okay, so what the patch actually should change to fix this?


>
>Fixes: 53592b364001 ("net/sched: act_mirred: Implement ingress actions")
>Cc: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
>Suggested-by: Davide Caratti <dcaratti@...hat.com>
>Link: https://lore.kernel.org/netdev/33dc43f587ec1388ba456b4915c75f02a8aae226.1663945716.git.dcaratti@redhat.com/
>Signed-off-by: Jakub Kicinski <kuba@...nel.org>
>---
>CC: jhs@...atatu.com
>CC: xiyou.wangcong@...il.com
>CC: jiri@...nulli.us
>CC: shmulik.ladkani@...il.com
>---
> net/sched/act_mirred.c                             | 14 +++++---------
> .../testing/selftests/net/forwarding/tc_actions.sh |  3 ---
> 2 files changed, 5 insertions(+), 12 deletions(-)
>
>diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
>index 0a1a9e40f237..291d47c9eb69 100644
>--- a/net/sched/act_mirred.c
>+++ b/net/sched/act_mirred.c
>@@ -232,18 +232,14 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
> 	return err;
> }
> 
>-static bool is_mirred_nested(void)
>-{
>-	return unlikely(__this_cpu_read(mirred_nest_level) > 1);
>-}
>-
>-static int tcf_mirred_forward(bool want_ingress, struct sk_buff *skb)
>+static int
>+tcf_mirred_forward(bool at_ingress, bool want_ingress, struct sk_buff *skb)
> {
> 	int err;
> 
> 	if (!want_ingress)
> 		err = tcf_dev_queue_xmit(skb, dev_queue_xmit);
>-	else if (is_mirred_nested())
>+	else if (!at_ingress)
> 		err = netif_rx(skb);
> 	else
> 		err = netif_receive_skb(skb);
>@@ -319,9 +315,9 @@ static int tcf_mirred_to_dev(struct sk_buff *skb, struct tcf_mirred *m,
> 
> 		skb_set_redirected(skb_to_send, skb_to_send->tc_at_ingress);
> 
>-		err = tcf_mirred_forward(want_ingress, skb_to_send);
>+		err = tcf_mirred_forward(at_ingress, want_ingress, skb_to_send);
> 	} else {
>-		err = tcf_mirred_forward(want_ingress, skb_to_send);
>+		err = tcf_mirred_forward(at_ingress, want_ingress, skb_to_send);
> 	}
> 
> 	if (err) {
>diff --git a/tools/testing/selftests/net/forwarding/tc_actions.sh b/tools/testing/selftests/net/forwarding/tc_actions.sh
>index b0f5e55d2d0b..589629636502 100755
>--- a/tools/testing/selftests/net/forwarding/tc_actions.sh
>+++ b/tools/testing/selftests/net/forwarding/tc_actions.sh
>@@ -235,9 +235,6 @@ mirred_egress_to_ingress_tcp_test()
> 	check_err $? "didn't mirred redirect ICMP"
> 	tc_check_packets "dev $h1 ingress" 102 10
> 	check_err $? "didn't drop mirred ICMP"
>-	local overlimits=$(tc_rule_stats_get ${h1} 101 egress .overlimits)
>-	test ${overlimits} = 10
>-	check_err $? "wrong overlimits, expected 10 got ${overlimits}"
> 
> 	tc filter del dev $h1 egress protocol ip pref 100 handle 100 flower
> 	tc filter del dev $h1 egress protocol ip pref 101 handle 101 flower
>-- 
>2.43.0
>