Re: [PATCH v2 (resend)] media: imon: reorganize serialization

[Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>]

On 2022/05/02 18:39, Sean Young wrote:
> So this part of the patch address the issue of driver_lock being
> unnecessary. This should be in its own patch, so I am going to merge:
> 
> https://patchwork.linuxtv.org/project/linux-media/patch/349f3e34-41ed-f832-3b22-ae10c50e3868@xxxxxxxxxxxxxxxxxxx/

driver_lock is not unnecessary, unless combined with kfree_rcu() approach.

>> +	/*
>> +	 * We need to wait for RCU grace period in order to allow
>> +	 * display_open() to safely check ->disconnected and increment ->users.
>> +	 */
>> +	struct rcu_head rcu;
> 
> Is it possible to modify the users/disconnected fields while holding the
> lock mutex in imon_context? This would make it unnecessary to use rcu and
> simplify the code.

I don't think it is possible, and I don't think it simplifies the code.

Unless we revive global driver_lock lock (untested delta diff is shown below),
nothing prevents from calling kfree(ictx) before holding ictx->lock or
checking ->disconnected or incrementing ->users.

As a whole, I think kfree_rcu() is simpler while removing global lock.

diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
index 9a4f24e294bc..469a2f869572 100644
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -166,11 +166,6 @@ struct imon_context {
 	 * imon_disconnect() was already called.
 	 */
 	bool disconnected;
-	/*
-	 * We need to wait for RCU grace period in order to allow
-	 * display_open() to safely check ->disconnected and increment ->users.
-	 */
-	struct rcu_head rcu;
 };
 
 #define TOUCH_TIMEOUT	(HZ/30)
@@ -457,6 +452,9 @@ static struct usb_driver imon_driver = {
 	.id_table	= imon_usb_id_table,
 };
 
+/* to prevent races between open() and kfree() */
+static DEFINE_MUTEX(driver_lock);
+
 /* Module bookkeeping bits */
 MODULE_AUTHOR(MOD_AUTHOR);
 MODULE_DESCRIPTION(MOD_DESC);
@@ -516,6 +514,8 @@ static int display_open(struct inode *inode, struct file *file)
 	int subminor;
 	int retval = 0;
 
+	mutex_lock(&driver_lock);
+
 	subminor = iminor(inode);
 	interface = usb_find_interface(&imon_driver, subminor);
 	if (!interface) {
@@ -524,15 +524,13 @@ static int display_open(struct inode *inode, struct file *file)
 		goto exit;
 	}
 
-	rcu_read_lock();
 	ictx = usb_get_intfdata(interface);
 	if (!ictx || ictx->disconnected || !refcount_inc_not_zero(&ictx->users)) {
-		rcu_read_unlock();
 		pr_err("no context found for minor %d\n", subminor);
 		retval = -ENODEV;
 		goto exit;
 	}
-	rcu_read_unlock();
+	mutex_unlock(&driver_lock);
 
 	mutex_lock(&ictx->lock);
 
@@ -550,10 +548,15 @@ static int display_open(struct inode *inode, struct file *file)
 
 	mutex_unlock(&ictx->lock);
 
-	if (retval && refcount_dec_and_test(&ictx->users))
-		free_imon_context(ictx);
-
+	if (retval) {
+		mutex_unlock(&driver_lock);
+		if (refcount_dec_and_test(&ictx->users))
+			free_imon_context(ictx);
+		mutex_unlock(&driver_lock);
+	}
+	return retval;
 exit:
+	mutex_unlock(&driver_lock);
 	return retval;
 }
 
@@ -2497,6 +2500,8 @@ static void imon_disconnect(struct usb_interface *interface)
 	struct device *dev;
 	int ifnum;
 
+	mutex_lock(&driver_lock);
+
 	ictx = usb_get_intfdata(interface);
 	ictx->disconnected = true;
 	dev = ictx->dev;
@@ -2542,6 +2547,8 @@ static void imon_disconnect(struct usb_interface *interface)
 	if (refcount_dec_and_test(&ictx->users))
 		free_imon_context(ictx);
 
+	mutex_unlock(&driver_lock);
+
 	dev_dbg(dev, "%s: iMON device (intf%d) disconnected\n",
 		__func__, ifnum);
 }