On Sat, Aug 28, 2021 at 10:10:36AM +0900, Tetsuo Handa wrote:
That is, it seems that unregister_transfer_cb() is there in case forced module
unload of cryptoloop module was requested. And in that case, there is no point
with crashing the kernel via panic_on_warn == 1 && WARN_ON_ONCE(). Simple printk()
will be sufficient.
If we have that case for forced module unload a WARN_ON is the right thing.
That being said we can simply do the cmpxchg based protection for that
case as well if you want to keep it. That will lead to a spurious
loop remove failure with -EBUSY when a concurrent force module removal
for cryptoloop is happening, but if you do something like that you get
to keep the pieces.
In order to annotate that extra operations that might sleep should not be
added inside this section. Use of sleepable locks tends to get extra
operations (e.g. wait for a different mutex / completion) and makes it unclear
what the lock is protecting. I can imagine a future that someone adds an
unwanted dependency inside this section if we use mutex here.
Technically, we can add preempt_diable() after mutex_lock() and
preempt_enable() before mutex_unlock() in order to annotate that
extra operations that might sleep should be avoided.
But idr_alloc(GFP_ATOMIC)/idr_find()/idr_for_each_entry() etc. will be
fast enough.
Well, split that into a cleanup patch if you think it is worth the
effort, with a good changelog. Not really part of the bug fix.