[PATCH net v2 1/2] net/sched: act_mirred: use the backlog for mirred ingress

[PATCH net v2 1/2] net/sched: act_mirred: use the backlog for mirred ingress

[Jakub Kicinski]

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.

Fixes: 53592b364001 ("net/sched: act_mirred: Implement ingress actions")
Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Suggested-by: Davide Caratti <dcaratti@redhat.com>
Link: https://lore.kernel.org/netdev/33dc43f587ec1388ba456b4915c75f02a8aae226.1663945716.git.dcaratti@redhat.com/
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: jhs@mojatatu.com
CC: xiyou.wangcong@gmail.com
CC: jiri@resnulli.us
CC: shmulik.ladkani@gmail.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

[PATCH net v2 2/2] net/sched: act_mirred: don't override retval if we already lost the skb

[Jakub Kicinski]

If we're redirecting the skb, and haven't called tcf_mirred_forward(),
yet, we need to tell the core to drop the skb by setting the retcode
to SHOT. If we have called tcf_mirred_forward(), however, the skb
is out of our hands and returning SHOT will lead to UaF.

Move the retval override to the error path which actually need it.

Fixes: e5cf1baf92cb ("act_mirred: use TC_ACT_REINSERT when possible")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: jhs@mojatatu.com
CC: xiyou.wangcong@gmail.com
CC: jiri@resnulli.us
---
 net/sched/act_mirred.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 291d47c9eb69..6faa7d00da09 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -266,8 +266,7 @@ static int tcf_mirred_to_dev(struct sk_buff *skb, struct tcf_mirred *m,
 	if (unlikely(!(dev->flags & IFF_UP)) || !netif_carrier_ok(dev)) {
 		net_notice_ratelimited("tc mirred to Houston: device %s is down\n",
 				       dev->name);
-		err = -ENODEV;
-		goto out;
+		goto err_cant_do;
 	}
 
 	/* we could easily avoid the clone only if called by ingress and clsact;
@@ -279,10 +278,8 @@ static int tcf_mirred_to_dev(struct sk_buff *skb, struct tcf_mirred *m,
 		tcf_mirred_can_reinsert(retval);
 	if (!dont_clone) {
 		skb_to_send = skb_clone(skb, GFP_ATOMIC);
-		if (!skb_to_send) {
-			err =  -ENOMEM;
-			goto out;
-		}
+		if (!skb_to_send)
+			goto err_cant_do;
 	}
 
 	want_ingress = tcf_mirred_act_wants_ingress(m_eaction);
@@ -319,15 +316,16 @@ static int tcf_mirred_to_dev(struct sk_buff *skb, struct tcf_mirred *m,
 	} else {
 		err = tcf_mirred_forward(at_ingress, want_ingress, skb_to_send);
 	}
-
-	if (err) {
-out:
+	if (err)
 		tcf_action_inc_overlimit_qstats(&m->common);
-		if (is_redirect)
-			retval = TC_ACT_SHOT;
-	}
 
 	return retval;
+
+err_cant_do:
+	if (is_redirect)
+		retval = TC_ACT_SHOT;
+	tcf_action_inc_overlimit_qstats(&m->common);
+	return retval;
 }
 
 static int tcf_blockcast_redir(struct sk_buff *skb, struct tcf_mirred *m,
-- 
2.43.0

Re: [PATCH net v2 1/2] net/sched: act_mirred: use the backlog for mirred ingress

[Jiri Pirko]

Wed, Feb 14, 2024 at 04:38:47AM CET, kuba@kernel.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@gmail.com>
>Suggested-by: Davide Caratti <dcaratti@redhat.com>
>Link: https://lore.kernel.org/netdev/33dc43f587ec1388ba456b4915c75f02a8aae226.1663945716.git.dcaratti@redhat.com/
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>---
>CC: jhs@mojatatu.com
>CC: xiyou.wangcong@gmail.com
>CC: jiri@resnulli.us
>CC: shmulik.ladkani@gmail.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
>

Re: [PATCH net v2 1/2] net/sched: act_mirred: use the backlog for mirred ingress

[Jakub Kicinski]

On Wed, 14 Feb 2024 09:51:00 +0100 Jiri Pirko wrote:
> Wed, Feb 14, 2024 at 04:38:47AM CET, kuba@kernel.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?

Sorry I'm not sure what you're asking.

We can't redirect traffic back to ourselves because we can end up
trying to take the socket lock for a socket that is generating
the packet.

Or are you asking how we can get the stats from the packet
asynchronously? We could build a local async scheme but I'd rather
not go there unless someone actually cares about these stats.

Re: [PATCH net v2 2/2] net/sched: act_mirred: don't override retval if we already lost the skb

[Michal Swiatkowski]

On Tue, Feb 13, 2024 at 07:38:48PM -0800, Jakub Kicinski wrote:
> If we're redirecting the skb, and haven't called tcf_mirred_forward(),
> yet, we need to tell the core to drop the skb by setting the retcode
> to SHOT. If we have called tcf_mirred_forward(), however, the skb
> is out of our hands and returning SHOT will lead to UaF.
> 
Thanks for fixing it. I had this UaF after ingress to ingress
redirection (was wondering if it is reasonable filter configuration).

[ 1459.177197] refcount_t: underflow; use-after-free.
[ 1459.177219] WARNING: CPU: 32 PID: 2427 at lib/refcount.c:28 refcount_warn_saturate+0xba/0x110
[ 1459.177227] Modules linked in: act_mirred cls_flower sch_ingress veth irdma ice(E) nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip_set nf_tables nfnetlink ipmi_ssif intel_rapl_msr intel_rapl_common intel_uncore_frequency intel_uncore_frequency_common i10nm_edac nfit libnvdimm x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel mlx5_ib i40e kvm vfat fat ib_uverbs dell_wmi irqbypass ledtrig_audio rapl sparse_keymap acpi_ipmi iTCO_wdt rfkill intel_cstate intel_pmc_bxt video ipmi_si dell_smbios mei_me iTCO_vendor_support ib_core intel_uncore dcdbas joydev dell_wmi_descriptor wmi_bmof dax_hmem isst_if_mmio pcspkr isst_if_mbox_pci ipmi_devintf mei i2c_i801 isst_if_common intel_pch_thermal i2c_smbus ipmi_msghandler acpi_power_meter intel_vsec fuse zram mlx5_core crct10dif_pclmul crc32_pclmul crc32c_intel polyval_clmulni polyval_generic ghash_clmulni_intel mlxfw sha512_ssse3 bnxt_en tls
[ 1459.177300]  mgag200 sha256_ssse3 megaraid_sas tg3
[ 1459.177302] dev_queue_xmit: bootnet
[ 1459.177304]  sha1_ssse3 psample gnss pci_hyperv_intf
[ 1459.177309] sch_handle_egress on: bootnet
[ 1459.177312]  i2c_algo_bit wmi [last unloaded: ice]
[ 1459.177318] CPU: 32 PID: 2427 Comm: scapy Kdump: loaded Tainted: G            E      6.8.0-rc2+ #3
[ 1459.177321] Hardware name: Dell Inc. PowerEdge R750/0PJ80M, BIOS 1.12.1 09/13/2023
[ 1459.177322] RIP: 0010:refcount_warn_saturate+0xba/0x110
[ 1459.177326] Code: 01 01 e8 19 1f 92 ff 0f 0b c3 cc cc cc cc 80 3d fd 12 cf 01 00 75 85 48 c7 c7 70 de 93 89 c6 05 ed 12 cf 01 01 e8 f6 1e 92 ff <0f> 0b c3 cc cc cc cc 80 3d d8 12 cf 01 00 0f 85 5e ff ff ff 48 c7
[ 1459.177328] RSP: 0018:ff5e8ab706db4df0 EFLAGS: 00010282
[ 1459.177331] RAX: 0000000000000026 RBX: ff4f7131460b1000 RCX: 0000000000000000
[ 1459.177332] RDX: 0000000000000103 RSI: ffffffff89924668 RDI: 00000000ffffffff
[ 1459.177334] RBP: ff4f7138e904c000 R08: 0000000000000000 R09: ff5e8ab706db4c90
[ 1459.177335] R10: 0000000000000003 R11: ff4f7140bfd49b28 R12: 0000000000000000
[ 1459.177336] R13: ff4f7138e904c120 R14: 0000000000000002 R15: 0000000000000000
[ 1459.177337] FS:  00007f69ec975b80(0000) GS:ff4f7138a0200000(0000) knlGS:0000000000000000
[ 1459.177339] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1459.177340] CR2: 00007f6861c9d030 CR3: 0000000157fd0004 CR4: 0000000000771ef0
[ 1459.177342] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1459.177343] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 1459.177344] PKRU: 55555554
[ 1459.177345] Call Trace:
[ 1459.177347]  <IRQ>
[ 1459.177348]  ? refcount_warn_saturate+0xba/0x110
[ 1459.177351]  ? __warn+0x7d/0x130
[ 1459.177358]  ? refcount_warn_saturate+0xba/0x110
[ 1459.177360]  ? report_bug+0x18d/0x1c0
[ 1459.177366]  ? prb_read_valid+0x17/0x20
[ 1459.177373]  ? handle_bug+0x41/0x70
[ 1459.177378]  ? exc_invalid_op+0x13/0x60
[ 1459.177382]  ? asm_exc_invalid_op+0x16/0x20
[ 1459.177388]  ? refcount_warn_saturate+0xba/0x110
[ 1459.177390]  skb_release_head_state+0x7d/0x90
[ 1459.177398]  kfree_skb_reason+0x35/0x110
[ 1459.177400]  __netif_receive_skb_core.constprop.0+0x971/0x1000
[ 1459.177406]  ? __blk_mq_free_request+0x70/0x100
[ 1459.177410]  ? blk_queue_exit+0xe/0x40
[ 1459.177416]  ? scsi_end_request+0xfb/0x1b0
[ 1459.177422]  __netif_receive_skb_one_core+0x2c/0x80
[ 1459.177424]  process_backlog+0x81/0x120
[ 1459.177427]  __napi_poll+0x28/0x1c0
[ 1459.177430]  net_rx_action+0x283/0x360
[ 1459.177432]  ? sched_clock+0xc/0x30
[ 1459.177438]  __do_softirq+0xf2/0x316
[ 1459.177443]  ? irqtime_account_irq+0xa4/0xd0
[ 1459.177448]  do_softirq.part.0+0x72/0x90
[ 1459.177452]  </IRQ>

After applying the patch it is fine.

> Move the retval override to the error path which actually need it.
> 
> Fixes: e5cf1baf92cb ("act_mirred: use TC_ACT_REINSERT when possible")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: jhs@mojatatu.com
> CC: xiyou.wangcong@gmail.com
> CC: jiri@resnulli.us
> ---
>  net/sched/act_mirred.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

Re: [PATCH net v2 1/2] net/sched: act_mirred: use the backlog for mirred ingress

[Paolo Abeni]

On Wed, 2024-02-14 at 07:04 -0800, Jakub Kicinski wrote:
> On Wed, 14 Feb 2024 09:51:00 +0100 Jiri Pirko wrote:
> > Wed, Feb 14, 2024 at 04:38:47AM CET, kuba@kernel.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?
> 
> Sorry I'm not sure what you're asking.
> 
> We can't redirect traffic back to ourselves because we can end up
> trying to take the socket lock for a socket that is generating
> the packet.
> 
> Or are you asking how we can get the stats from the packet
> asynchronously? We could build a local async scheme but I'd rather
> not go there unless someone actually cares about these stats.

I *guess* Jiri is suggesting to expand the commit message describing
how the fix implemented by this patch works.

@Jiri, feel free to provide the actual correct interpretation :)

Cheers,

Paolo

Re: [PATCH net v2 1/2] net/sched: act_mirred: use the backlog for mirred ingress

[Jiri Pirko]

Thu, Feb 15, 2024 at 01:56:12PM CET, pabeni@redhat.com wrote:
>On Wed, 2024-02-14 at 07:04 -0800, Jakub Kicinski wrote:
>> On Wed, 14 Feb 2024 09:51:00 +0100 Jiri Pirko wrote:
>> > Wed, Feb 14, 2024 at 04:38:47AM CET, kuba@kernel.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?
>> 
>> Sorry I'm not sure what you're asking.
>> 
>> We can't redirect traffic back to ourselves because we can end up
>> trying to take the socket lock for a socket that is generating
>> the packet.
>> 
>> Or are you asking how we can get the stats from the packet
>> asynchronously? We could build a local async scheme but I'd rather
>> not go there unless someone actually cares about these stats.
>
>I *guess* Jiri is suggesting to expand the commit message describing
>how the fix implemented by this patch works.
>
>@Jiri, feel free to provide the actual correct interpretation :)

Yes, but I was silent not to get beaten by another maintainer for
pointing this out :) But really, the patch desctiption should make it
simpler to understand the code, that's my motivation. Not to bug
people...


>
>Cheers,
>
>Paolo
>

Re: [PATCH net v2 1/2] net/sched: act_mirred: use the backlog for mirred ingress

[Jakub Kicinski]

On Thu, 15 Feb 2024 13:56:12 +0100 Paolo Abeni wrote:
> > Sorry I'm not sure what you're asking.
> > 
> > We can't redirect traffic back to ourselves because we can end up
> > trying to take the socket lock for a socket that is generating
> > the packet.
> > 
> > Or are you asking how we can get the stats from the packet
> > asynchronously? We could build a local async scheme but I'd rather
> > not go there unless someone actually cares about these stats.  
> 
> I *guess* Jiri is suggesting to expand the commit message describing
> how the fix implemented by this patch works.

The irony... ;]

v3 with more words:
https://lore.kernel.org/all/20240215143346.1715054-1-kuba@kernel.org/