Re: [PATCH net] net/sched: cls_api: Fix lockup on flushing explicitly created chain

[Vlad Buslov <vladbu@nvidia.com>]

On Mon 12 Jun 2023 at 10:59, Pedro Tammela <pctammela@mojatatu.com> wrote:
> On 12/06/2023 06:34, Vlad Buslov wrote:
>> Mingshuai Ren reports:
>> When a new chain is added by using tc, one soft lockup alarm will be
>>   generated after delete the prio 0 filter of the chain. To reproduce
>>   the problem, perform the following steps:
>> (1) tc qdisc add dev eth0 root handle 1: htb default 1
>> (2) tc chain add dev eth0
>> (3) tc filter del dev eth0 chain 0 parent 1: prio 0
>> (4) tc filter add dev eth0 chain 0 parent 1:
>> Fix the issue by accounting for additional reference to chains that are
>> explicitly created by RTM_NEWCHAIN message as opposed to implicitly by
>> RTM_NEWTFILTER message.
>> Fixes: 726d061286ce ("net: sched: prevent insertion of new classifiers during
>> chain flush")
>> Reported-by: Mingshuai Ren <renmingshuai@huawei.com>
>> Closes: https://lore.kernel.org/lkml/87legswvi3.fsf@nvidia.com/T/
>> Signed-off-by: Vlad Buslov <vladbu@nvidia.com>
>> ---
>>   net/sched/cls_api.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>
>
> Hi Vlad,
>
> Thanks for taking a look.
> Could you also carry over the tdc test or ask Ren to post in a separate patch?

Sure. I was planning to ask Mingshuai Ren to submit the new test as
standalone patch after my fix has been accepted since including his code
with my fix would require explicit approval of the whole patch and his
Signed-off-by clause AFAIK.

>
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index 2621550bfddc..e4df96e133cd 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -659,8 +659,8 @@ static void __tcf_chain_put(struct tcf_chain *chain, bool by_act,
>>   {
>>   	struct tcf_block *block = chain->block;
>>   	const struct tcf_proto_ops *tmplt_ops;
>> +	unsigned int refcnt, non_act_refcnt;
>>   	bool free_block = false;
>> -	unsigned int refcnt;
>>   	void *tmplt_priv;
>>     	mutex_lock(&block->lock);
>> @@ -680,13 +680,15 @@ static void __tcf_chain_put(struct tcf_chain *chain, bool by_act,
>>   	 * save these to temporary variables.
>>   	 */
>>   	refcnt = --chain->refcnt;
>> +	non_act_refcnt = refcnt - chain->action_refcnt;
>>   	tmplt_ops = chain->tmplt_ops;
>>   	tmplt_priv = chain->tmplt_priv;
>>   -	/* The last dropped non-action reference will trigger notification. */
>> -	if (refcnt - chain->action_refcnt == 0 && !by_act) {
>> -		tc_chain_notify_delete(tmplt_ops, tmplt_priv, chain->index,
>> -				       block, NULL, 0, 0, false);
>> +	if (non_act_refcnt == chain->explicitly_created && !by_act) {
>> +		if (non_act_refcnt == 0)
>> +			tc_chain_notify_delete(tmplt_ops, tmplt_priv,
>> +					       chain->index, block, NULL, 0, 0,
>> +					       false);
>>   		/* Last reference to chain, no need to lock. */
>>   		chain->flushing = false;
>>   	}