[net/sched] Question: Locks for clearing ERR_PTR() value from idrinfo->action_idr ?

[net/sched] Question: Locks for clearing ERR_PTR() value from idrinfo->action_idr ?

[Tetsuo Handa]

Hello.

syzbot is reporting hung task problems involving rtnl_muxex. A debug printk()
patch added to linux-next-20240611 suggested that many of them are caused by
an infinite busy loop inside tcf_idr_check_alloc().

----------
again:
		rcu_read_lock();
		p = idr_find(&idrinfo->action_idr, *index);

		if (IS_ERR(p)) {
			/* This means that another process allocated
			 * index but did not assign the pointer yet.
			 */
			rcu_read_unlock();
			goto again;
		}
----------

Since there is no sleep (e.g. cond_resched()/schedule_timeout_uninterruptible(1))
before "goto again;", once idr_find() returns an IS_ERR() value, all of that CPU's
computation resource is wasted forever with rtnl_mutex held (and anybody else who
tries to hold rtnl_mutex at rtnl_lock() is reported as hung task, resulting in
various hung task reports waiting for rtnl_mutex at rtnl_lock()).

Therefore, I tried to add a sleep before "goto again;", but I can't know whether
a sleep added to linux-next-20240612 solves the hung task problem because syzbot
currently cannot test linux-next kernels due to some different problem.

Therefore, I'm posting a question here before syzbot can resume testing of
linux-next kernels. As far as I can see, the ERR_PTR(-EBUSY) assigned at

	mutex_lock(&idrinfo->lock);
	ret = idr_alloc_u32(&idrinfo->action_idr, ERR_PTR(-EBUSY), index, max,
			    GFP_KERNEL);
	mutex_unlock(&idrinfo->lock);

in tcf_idr_check_alloc() is cleared by either

	mutex_lock(&idrinfo->lock);
	/* Remove ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
	WARN_ON(!IS_ERR(idr_remove(&idrinfo->action_idr, index)));
	mutex_unlock(&idrinfo->lock);

in tcf_idr_cleanup() or

	mutex_lock(&idrinfo->lock);
	/* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
	idr_replace(&idrinfo->action_idr, a, a->tcfa_index);
	mutex_unlock(&idrinfo->lock);

in tcf_idr_insert_many().

But is there a possibility that rtnl_mutex is released between
tcf_idr_check_alloc() and tcf_idr_{cleanup,insert_many}() ? If yes,
adding a sleep before "goto again;" won't be sufficient. But if no,
how can

	/* This means that another process allocated
	 * index but did not assign the pointer yet.
	 */

happen (because both setting ERR_PTR(-EBUSY) and replacing with an !IS_ERR()
value are done without temporarily releasing rtnl_mutex) ?

Is there a possibility that tcf_idr_check_alloc() is called without holding
rtnl_mutex? If yes, adding a sleep before "goto again;" would help. But if no,
is this a sign that some path forgot to call tcf_idr_{cleanup,insert_many}() ?

Re: [net/sched] Question: Locks for clearing ERR_PTR() value from idrinfo->action_idr ?

[Kuniyuki Iwashima]

From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Fri, 14 Jun 2024 09:58:48 +0900
> Hello.
> 
> syzbot is reporting hung task problems involving rtnl_muxex. A debug printk()
> patch added to linux-next-20240611 suggested that many of them are caused by
> an infinite busy loop inside tcf_idr_check_alloc().

I think the fix is:
https://lore.kernel.org/netdev/20240613071021.471432-1-druth@chromium.org/


> 
> ----------
> again:
> 		rcu_read_lock();
> 		p = idr_find(&idrinfo->action_idr, *index);
> 
> 		if (IS_ERR(p)) {
> 			/* This means that another process allocated
> 			 * index but did not assign the pointer yet.
> 			 */
> 			rcu_read_unlock();
> 			goto again;
> 		}
> ----------
> 
> Since there is no sleep (e.g. cond_resched()/schedule_timeout_uninterruptible(1))
> before "goto again;", once idr_find() returns an IS_ERR() value, all of that CPU's
> computation resource is wasted forever with rtnl_mutex held (and anybody else who
> tries to hold rtnl_mutex at rtnl_lock() is reported as hung task, resulting in
> various hung task reports waiting for rtnl_mutex at rtnl_lock()).
> 
> Therefore, I tried to add a sleep before "goto again;", but I can't know whether
> a sleep added to linux-next-20240612 solves the hung task problem because syzbot
> currently cannot test linux-next kernels due to some different problem.
> 
> Therefore, I'm posting a question here before syzbot can resume testing of
> linux-next kernels. As far as I can see, the ERR_PTR(-EBUSY) assigned at
> 
> 	mutex_lock(&idrinfo->lock);
> 	ret = idr_alloc_u32(&idrinfo->action_idr, ERR_PTR(-EBUSY), index, max,
> 			    GFP_KERNEL);
> 	mutex_unlock(&idrinfo->lock);
> 
> in tcf_idr_check_alloc() is cleared by either
> 
> 	mutex_lock(&idrinfo->lock);
> 	/* Remove ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
> 	WARN_ON(!IS_ERR(idr_remove(&idrinfo->action_idr, index)));
> 	mutex_unlock(&idrinfo->lock);
> 
> in tcf_idr_cleanup() or
> 
> 	mutex_lock(&idrinfo->lock);
> 	/* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
> 	idr_replace(&idrinfo->action_idr, a, a->tcfa_index);
> 	mutex_unlock(&idrinfo->lock);
> 
> in tcf_idr_insert_many().
> 
> But is there a possibility that rtnl_mutex is released between
> tcf_idr_check_alloc() and tcf_idr_{cleanup,insert_many}() ? If yes,
> adding a sleep before "goto again;" won't be sufficient. But if no,
> how can
> 
> 	/* This means that another process allocated
> 	 * index but did not assign the pointer yet.
> 	 */
> 
> happen (because both setting ERR_PTR(-EBUSY) and replacing with an !IS_ERR()
> value are done without temporarily releasing rtnl_mutex) ?
> 
> Is there a possibility that tcf_idr_check_alloc() is called without holding
> rtnl_mutex? If yes, adding a sleep before "goto again;" would help. But if no,
> is this a sign that some path forgot to call tcf_idr_{cleanup,insert_many}() ?

Re: [net/sched] Question: Locks for clearing ERR_PTR() value from idrinfo->action_idr ?

[Pedro Tammela]

On 13/06/2024 21:58, Tetsuo Handa wrote:
> 
> Is there a possibility that tcf_idr_check_alloc() is called without holding
> rtnl_mutex?

There is, but not in the code path of this reproducer.

> If yes, adding a sleep before "goto again;" would help. But if no,
> is this a sign that some path forgot to call tcf_idr_{cleanup,insert_many}() ?

The reproducer is sending a new action message with 2 actions.
Actions are committed to the idr after processing in order to make them 
visible together and after any errors are caught.

The bug happens when the actions in the message refer to the same index. 
Since the first processing succeeds, adding -EBUSY to the index, the 
second processing, which references the same index, will loop forever.

After the change to rely on RCU for this check, instead of the idr lock, 
the hangs became more noticeable to syzbot since now it's hanging a 
system-wide lock.

Re: [net/sched] Question: Locks for clearing ERR_PTR() value from idrinfo->action_idr ?

[Tetsuo Handa]

On 2024/06/14 11:47, Pedro Tammela wrote:
> On 13/06/2024 21:58, Tetsuo Handa wrote:
>>
>> Is there a possibility that tcf_idr_check_alloc() is called without holding
>> rtnl_mutex?
> 
> There is, but not in the code path of this reproducer.
> 
>> If yes, adding a sleep before "goto again;" would help. But if no,
>> is this a sign that some path forgot to call tcf_idr_{cleanup,insert_many}() ?
> 
> The reproducer is sending a new action message with 2 actions.
> Actions are committed to the idr after processing in order to make them visible
> together and after any errors are caught.
> 
> The bug happens when the actions in the message refer to the same index. Since
> the first processing succeeds, adding -EBUSY to the index, the second processing,
> which references the same index, will loop forever.
> 
> After the change to rely on RCU for this check, instead of the idr lock, the hangs
> became more noticeable to syzbot since now it's hanging a system-wide lock.

Thank you for explanation. Then, what type of sleep do we want?

schedule_timeout_uninteruptible(1)
(based on an assumption that conflict will be solved shortly) ?

wait_event()/wake_up_all() using one global waitqueue
(based on an assumption that conflict is rare) ?

wait_event()/wake_up_all() using per struct tcf_idrinfo waitqueue
(based on an assumption that conflict might not be rare) ?

Re: [net/sched] Question: Locks for clearing ERR_PTR() value from idrinfo->action_idr ?

[Pedro Tammela]

On 14/06/2024 01:00, Tetsuo Handa wrote:
> On 2024/06/14 11:47, Pedro Tammela wrote:
>> On 13/06/2024 21:58, Tetsuo Handa wrote:
>>>
>>> Is there a possibility that tcf_idr_check_alloc() is called without holding
>>> rtnl_mutex?
>>
>> There is, but not in the code path of this reproducer.
>>
>>> If yes, adding a sleep before "goto again;" would help. But if no,
>>> is this a sign that some path forgot to call tcf_idr_{cleanup,insert_many}() ?
>>
>> The reproducer is sending a new action message with 2 actions.
>> Actions are committed to the idr after processing in order to make them visible
>> together and after any errors are caught.
>>
>> The bug happens when the actions in the message refer to the same index. Since
>> the first processing succeeds, adding -EBUSY to the index, the second processing,
>> which references the same index, will loop forever.
>>
>> After the change to rely on RCU for this check, instead of the idr lock, the hangs
>> became more noticeable to syzbot since now it's hanging a system-wide lock.
> 
> Thank you for explanation. Then, what type of sleep do we want?

This patch should fix the bug: 
https://lore.kernel.org/netdev/20240613071021.471432-1-druth@chromium.org/