Messages in this thread | | | From | Schspa Shi <> | Subject | Re: [syzbot] WARNING: locking bug in umh_complete | Date | Tue, 14 Feb 2023 10:31:58 +0800 |
| |
Peter Zijlstra <peterz@infradead.org> writes:
> On Mon, Feb 06, 2023 at 07:51:16AM -0800, Luis Chamberlain wrote: > >> I think this seems to be the same issue that Schspa Shi reported / provided a >> fix sugggestion for [0]. This lead me to ask if: >> >> a) incorrect usage of completion on stack could be generic and; >> b) if we should instead have an API helper for that? >> >> Although he already implemented a suggestion for b) to answer a) we need >> some SmPL constructs yet to be written by Schspa. The reason I asked for >> b) is that if this is a regular pattern it begs for a) as this sort of >> issue could be prevalent in other places. So the status of Schspa's work >> was that he was going to work on the SmPL grammar to check how frequent >> this incorrect patern could be found. > > Do I read correctly, from you above alphabet-soup, that someone is > working on some static analysis for on-stack completions or something? >
Yes, I was trying to do this.
> If so, perhaps the simplest rule would to be ensure there is an > unconditional uninterruptible wait-for-completion() before going out of > scope. > > This latter can be spelled like wait_for_completion() or > wait_for_completion_state(TASK_UNINTERRUPTIBLE). More specifically, > TASK_INTERRUPTIBLE and TASK_WAKEKILL must not be set in the state mask > for the wait to be uninterruptible. > > If it cannot be proven, raise a warning and audit or somesuch.
This is a good suggestion. I have written a SmPL patch to complete this check, and now I need to rule out the situation that the driver has added an additional lock to protect it.
And I have found a lot of bad usage, should we consider adding a new helper API to simplify the fix this?
Such as:
+ +void complete_on_stack(struct completion **x) +{ + struct completion *comp = xchg(*x, NULL); + + if (comp) + complete(comp); +} +EXPORT_SYMBOL(complete_on_stack); + +int __sched wait_for_completion_state_on_stack(struct completion **x, + unsigned int state) +{ + struct completion *comp = *x; + int retval; + + retval = wait_for_completion_state(comp, state); + if (retval) { + if (xchg(*x, NULL)) + return retval; + + /* + * complete_on_stack will call complete shortly. + */ + wait_for_completion(comp); + } + + return retval; +} +EXPORT_SYMBOL(wait_for_completion_state_on_stack);
Link: https://lore.kernel.org/all/20221115140233.21981-1-schspa@gmail.com/T/#mf6a41a7009bb47af1b15adf2b7b355e495f609c4
-- BRs Schspa Shi
| |