Re: [PATCH RESEND net-next] net_sched: refactor TC action init API

[Ido Schimmel <idosch@idosch.org>]

On Thu, Jul 29, 2021 at 04:12:14PM -0700, Cong Wang wrote:
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 7be5b9d2aead..69185e311422 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -1949,6 +1949,7 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>  	int err;
>  	int tp_created;
>  	bool rtnl_held = false;
> +	u32 flags = 0;
>  
>  	if (!netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN))
>  		return -EPERM;
> @@ -2112,9 +2113,12 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>  		goto errout;
>  	}
>  
> +	if (!(n->nlmsg_flags & NLM_F_CREATE))
> +		flags |= TCA_ACT_FLAGS_REPLACE;
> +	if (!rtnl_held)
> +		flags |= TCA_ACT_FLAGS_NO_RTNL;

Cong, Vlad,

I'm getting deadlocks [1] after rebasing on net-next and I believe this
is the problematic part.

It is possible that during the first iteration RTNL mutex is not taken
and 'TCA_ACT_FLAGS_NO_RTNL' is set. However, in the second iteration
(after jumping to the 'replay' label) 'rtnl_held' is true, the mutex is
taken, but the flag is not cleared.

Will submit the following patch after I finish testing it unless you
have a better idea.

Thanks

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 69185e311422..af9ac2f4a84b 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -2117,6 +2117,8 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
                flags |= TCA_ACT_FLAGS_REPLACE;
        if (!rtnl_held)
                flags |= TCA_ACT_FLAGS_NO_RTNL;
+       else
+               flags &= ~TCA_ACT_FLAGS_NO_RTNL;
        err = tp->ops->change(net, skb, tp, cl, t->tcm_handle, tca, &fh,
                              flags, extack);
        if (err == 0) {

[1]
============================================
WARNING: possible recursive locking detected
5.14.0-rc3-custom-49011-g3d2bbb4f104d #447 Not tainted
--------------------------------------------
tc/37605 is trying to acquire lock:
ffffffff841df2f0 (rtnl_mutex){+.+.}-{3:3}, at: tc_setup_cb_add+0x14b/0x4d0

but task is already holding lock:
ffffffff841df2f0 (rtnl_mutex){+.+.}-{3:3}, at: tc_new_tfilter+0xb12/0x22e0

other info that might help us debug this:
 Possible unsafe locking scenario:
       CPU0
       ----
  lock(rtnl_mutex);
  lock(rtnl_mutex);

 *** DEADLOCK ***
 May be due to missing lock nesting notation
1 lock held by tc/37605:
 #0: ffffffff841df2f0 (rtnl_mutex){+.+.}-{3:3}, at: tc_new_tfilter+0xb12/0x22e0

stack backtrace:
CPU: 0 PID: 37605 Comm: tc Not tainted 5.14.0-rc3-custom-49011-g3d2bbb4f104d #447
Hardware name: Mellanox Technologies Ltd. MSN2010/SA002610, BIOS 5.6.5 08/24/2017
Call Trace:
 dump_stack_lvl+0x8b/0xb3
 __lock_acquire.cold+0x175/0x3cb
 lock_acquire+0x1a4/0x4f0
 __mutex_lock+0x136/0x10d0
 fl_hw_replace_filter+0x458/0x630 [cls_flower]
 fl_change+0x25f2/0x4a64 [cls_flower]
 tc_new_tfilter+0xa65/0x22e0
 rtnetlink_rcv_msg+0x86c/0xc60
 netlink_rcv_skb+0x14d/0x430
 netlink_unicast+0x539/0x7e0
 netlink_sendmsg+0x84d/0xd80
 ____sys_sendmsg+0x7ff/0x970
 ___sys_sendmsg+0xf8/0x170
 __sys_sendmsg+0xea/0x1b0
 do_syscall_64+0x35/0x80
 entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f7b93b6c0a7
Code: 0c 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 2e 00 00 00 0f 05 <48>
RSP: 002b:00007ffe365b3818 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f7b93b6c0a7
RDX: 0000000000000000 RSI: 00007ffe365b3880 RDI: 0000000000000003
RBP: 00000000610a75f6 R08: 0000000000000001 R09: 0000000000000000
R10: fffffffffffff3a9 R11: 0000000000000246 R12: 0000000000000001
R13: 0000000000000000 R14: 00007ffe365b7b58 R15: 00000000004822c0