On 2021/08/28 16:18, Christoph Hellwig wrote:
On Sat, Aug 28, 2021 at 10:10:36AM +0900, Tetsuo Handa wrote:
quoted
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.").