netdev - Re: [PATCH] net/sched: Forbid assigning mirred action to a filter attached to the egress

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: <20240315015645.4851-1-renmingshuai@huawei.com>
Date: Fri, 15 Mar 2024 09:56:45 +0800
From: renmingshuai <renmingshuai@...wei.com>
To: <jhs@...atatu.com>
CC: <caowangbao@...wei.com>, <davem@...emloft.net>, <jiri@...nulli.us>,
	<liaichun@...wei.com>, <netdev@...r.kernel.org>, <renmingshuai@...wei.com>,
	<vladbu@...dia.com>, <xiyou.wangcong@...il.com>, <yanan@...wei.com>
Subject: Re: [PATCH] net/sched: Forbid assigning mirred action to a filter attached to the egress

>> As we all know the mirred action is used to mirroring or redirecting the
>> packet it receives. Howerver, add mirred action to a filter attached to
>> a egress qdisc might cause a deadlock. To reproduce the problem, perform
>> the following steps:
>> (1)tc qdisc add dev eth0 root handle 1: htb default 30 \n
>> (2)tc filter add dev eth2 protocol ip prio 2 flower verbose \
>>      action police rate 100mbit burst 12m conform-exceed jump 1 \
>>      / pipe mirred egress redirect dev eth2 action drop
>>
>
>I think you meant both to be the same device eth0 or eth2?

Sorry, a careless mistake. eth2 in step 2 should be eth0.
(2)tc filter add dev eth0 protocol ip prio 2 flower verbose \
     action police rate 100mbit burst 12m conform-exceed jump 1 \
     / pipe mirred egress redirect dev eth0 action drop

>> The stack is show as below:
>> [28848.883915]  _raw_spin_lock+0x1e/0x30
>> [28848.884367]  __dev_queue_xmit+0x160/0x850
>> [28848.884851]  ? 0xffffffffc031906a
>> [28848.885279]  tcf_mirred_act+0x3ab/0x596 [act_mirred]
>> [28848.885863]  tcf_action_exec.part.0+0x88/0x130
>> [28848.886401]  fl_classify+0x1ca/0x1e0 [cls_flower]
>> [28848.886970]  ? dequeue_entity+0x145/0x9e0
>> [28848.887464]  ? newidle_balance+0x23f/0x2f0
>> [28848.887973]  ? nft_lookup_eval+0x57/0x170 [nf_tables]
>> [28848.888566]  ? nft_do_chain+0xef/0x430 [nf_tables]
>> [28848.889137]  ? __flush_work.isra.0+0x35/0x80
>> [28848.889657]  ? nf_ct_get_tuple+0x1cf/0x210 [nf_conntrack]
>> [28848.890293]  ? do_select+0x637/0x870
>> [28848.890735]  tcf_classify+0x52/0xf0
>> [28848.891177]  htb_classify+0x9d/0x1c0 [sch_htb]
>> [28848.891722]  htb_enqueue+0x3a/0x1c0 [sch_htb]
>> [28848.892251]  __dev_queue_xmit+0x2d8/0x850
>> [28848.892738]  ? nf_hook_slow+0x3c/0xb0
>> [28848.893198]  ip_finish_output2+0x272/0x580
>> [28848.893692]  __ip_queue_xmit+0x193/0x420
>> [28848.894179]  __tcp_transmit_skb+0x8cc/0x970
>>
>> In this case, the process has hold the qdisc spin lock in __dev_queue_xmit
>> before the egress packets are mirred, and it will attempt to obtain the
>> spin lock again after packets are mirred, which cause a deadlock.
>>
>> Fix the issue by forbidding assigning mirred action to a filter attached
>> to the egress.
>>
>> Signed-off-by: Mingshuai Ren <renmingshuai@...wei.com>
>> ---
>>  net/sched/act_mirred.c                        |  4 +++
>>  .../tc-testing/tc-tests/actions/mirred.json   | 32 +++++++++++++++++++
>>  2 files changed, 36 insertions(+)
>>
>> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
>> index 5b3814365924..fc96705285fb 100644
>> --- a/net/sched/act_mirred.c
>> +++ b/net/sched/act_mirred.c
>> @@ -120,6 +120,10 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
>>                 NL_SET_ERR_MSG_MOD(extack, "Mirred requires attributes to be passed");
>>                 return -EINVAL;
>>         }
>> +       if (tp->chain->block->q->parent != TC_H_INGRESS) {
>> +               NL_SET_ERR_MSG_MOD(extack, "Mirred can only be assigned to the filter attached to ingress");
>> +               return -EINVAL;
>> +       }
>
>Sorry, this is too restrictive as Jiri said. We'll try to reproduce. I
>am almost certain this used to work in the old days.
>
>cheers,
>jamal
>
>PS:- thanks for the tdc test, you are a hero just for submitting that!

As Jiri said, that is really quite restrictive. It might be better to forbid mirred attached to egress filter
to mirror or redirect packets to the egress. Just like:

--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -152,6 +152,12 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
                return -EINVAL;
        }

+       if ((tp->chain->block->q->parent != TC_H_INGRESS) &&
+           (parm->eaction == TCA_EGRESS_MIRROR || parm->eaction == TCA_EGRESS_REDIR)) {
+               NL_SET_ERR_MSG_MOD(extack, "Mirred assigned to egress filter can only mirror or redirect to ingress");
+               return -EINVAL;
+       }
+
        switch (parm->eaction) {
        case TCA_EGRESS_MIRROR:
        case TCA_EGRESS_REDIR: