On 2021/08/28 16:18, Christoph Hellwig wrote:
> 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.
Oh, given that commit 222013f9ac30b9ce ("cryptoloop: add a deprecation warning") was
already merged into linux.git , there is no point with worrying about forced module
unloading. Then, I would warn like
+#ifdef CONFIG_MODULE_UNLOAD
+ if (module_refcount(xfer->owner) != -1)
+ pr_err("Unregistering a transfer function in use. Expect kernel crashes.\n");
+#endif
than
+ idr_for_each_entry(&loop_index_idr, lo, id)
+ WARN_ON_ONCE(lo->lo_encryption == xfer);
in your patch.
(Actually, nobody calls loop_unregister_transfer() if CONFIG_MODULE_UNLOAD=n ...)
Then, your atomic_cmpxchg(&lo->idr_visible, 1, 0) == 0 approach will be OK
(I would use
+ atomic_set(&lo->idr_visible, 1);
than
+ atomic_inc(&lo->idr_visible);
because it is "Show this loop device again.").
