Re: [PATCH] loop: replace loop_ctl_mutex with loop_idr_spinlock

[Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>]

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.").