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.
