Misc: pass miscdevice through file's private_data

Place the miscdevice in the file's private_data to be used by the file operations.

Previously, this was done only when an open() operation had been registered. But it is also useful in the other file operations, so we pass it on unconditionally.

---
 drivers/char/misc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

index ffa97d2..c3681fb 100644
@@ -140,10 +140,16 @@ static int misc_open(struct inode * inode, struct file * file)
 			goto fail;
 	}
 
+	/*
+	 * Place the miscdevice in the file's
+	 * private_data so it can be used by the
+	 * file operations, including f_op->open below
+	 */
+	file->private_data = c;
+
 	err = 0;
 	replace_fops(file, new_fops);
 	if (file->f_op->open) {
-		file->private_data = c;
 		err = file->f_op->open(inode,file);
 	}
 fail:

1.9.1

Thanks for the fast feedback!

Good point, so I made a list of all drivers that use private_data (find drivers/ -iname "*.c" -exec grep -l private_data {} \; | sort -u) and manually verified that all of them either register an open() operation, do not use the misc subsystem, or matched because of vm_private_data.

I also removed the unnecessary curly braces. Please find the new patch below.

misc: pass miscdevice through file's private_data

Place the miscdevice in the file's private_data to be used by the file operations.

Previously, this was done only when an open() operation had been registered. But it is also useful in the other file operations, so we pass it on unconditionally.

All drivers that use private_data and misc_register() were checked to ensure they register an open() operation, in order to verify that this improvement does not break any existing driver that uses the misc subsystem.

---
 drivers/char/misc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

index ffa97d2..d6445de 100644
@@ -140,12 +140,17 @@ static int misc_open(struct inode * inode, struct file * file)
 			goto fail;
 	}
 
+	/*
+	 * Place the miscdevice in the file's
+	 * private_data so it can be used by the
+	 * file operations, including f_op->open below
+	 */
+	file->private_data = c;
+
 	err = 0;
 	replace_fops(file, new_fops);
-	if (file->f_op->open) {
-		file->private_data = c;
+	if (file->f_op->open)
 		err = file->f_op->open(inode,file);
-	}
 fail:
 	mutex_unlock(&misc_mtx);
 	return err;

1.9.1

Please resend this in a format I can apply it in (standalone, lines properly wrapped, etc.)

thanks,

greg k-h

Make the miscdevice accessible through the file's private_data.

Previously, this was done only when an open() operation had been registered. But it's also useful in other file operations so we pass it on unconditionally.

All drivers that use private_data and misc_register() were checked to ensure they register an open() operation in order to verify that this improvement does not break any existing driver that uses the misc subsystem.

---
 drivers/char/misc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

index ffa97d2..d6445de 100644
@@ -140,12 +140,17 @@ static int misc_open(struct inode * inode, struct file * file)
 			goto fail;
 	}
 
+	/*
+	 * Place the miscdevice in the file's
+	 * private_data so it can be used by the
+	 * file operations, including f_op->open below
+	 */
+	file->private_data = c;
+
 	err = 0;
 	replace_fops(file, new_fops);
-	if (file->f_op->open) {
-		file->private_data = c;
+	if (file->f_op->open)
 		err = file->f_op->open(inode,file);
-	}
 fail:
 	mutex_unlock(&misc_mtx);
 	return err;

1.9.1

Wait, didn't this break FUSE? Or was that some other variant of this patch?

confused,

greg k-h

You're right. How come I missed this?

I traced back my steps and as stated, I had reviewed all relevant code in the "drivers" folder. And therein lies the problem: I didn't think of reviewing the code in the "fs" folder, hence I missed fuse. I also went over the history of misc.c, but I used the default "git log", which pruned the reverted commits, while I should have used git log --full-history.

I'm sorry I missed this... and I'm not giving up :-)

I spent the whole day re-doing the investigation, checking *all* relevant code this time. The code in arch, fs, lib, net and virt uses misc_register, but most code is safe because it either registers a custom open() file operation or it doesn't rely on private_data being initialized to NULL when it didn't.

And yes, I did find *one* additional case that is affected; btrfs' transaction starting and stopping ioctl code in fs/btrfs/super.c uses private_data to store the transaction, and relies on the fact that it is NULL initially. This only works because btrfs doesn't register a custom open() file operation for this device file.

I'm currently testing out patches to make fuse and btrfs independent of this subtle behavior of the misc subsystem. I'll send my patches to the fuse and btrfs maintainers first and when they get accepted, I'll re-send this patch to misc.

Kind regards and thanks for the feedback,

Tom Van Braeckel.

I've been trying this a few months ago, ( https://lkml.org/lkml/2014/10/8/98 ) in a very bad attempt that (thankfully) failed.

I'm happy to see you trying this now and got the change in drivers/fuse merged now. I've been running a kernel with this change for quite some time, it's obviously fine.

I also only found the btrfs ioctl miscdevice as a main user that has to change, and the lguest driver (I sent off the lguest patch to check if this is correct.)

I append the additional changes I have applied locally (to the fuse change) for you. Feel free to use, test, use my signed-off-by (if you have exactly these changes) or send them off, however you want. Please be careful and double check to have all necessary changes merged before doing the change in misc_open().

Also, try to find drivers that assign or use struct miscdevice themselves. Those can be simplified. And of course drivers that basically do nothing in open(), just to have the link in private_data.

Thanks for the work!

an open syscall now assignes file->private_data to a pointer to the miscdevice structure. This reminds people not to duplicate code if they want this and not to depend on it being NULL.

---
 drivers/char/misc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 32ab943..c05e2f4 100644
@@ -168,7 +168,9 @@ static const struct file_operations misc_fops = {
  *	the minor number requested is used.
  *
  *	The structure passed is linked into the kernel and may not be
- *	destroyed until it has been unregistered.
+ *	destroyed until it has been unregistered. By default, an open()
+ *	syscall to the device sets file->private_data to point to the
+ *	structure. Drivers don't need open in fops for this.
  *
  *	A zero is returned on success and a negative errno code for
  *	failure.

2.1.4

the misc core does this now in any case.

---
 drivers/video/fbdev/pxa3xx-gcu.c | 10 ----------
 1 file changed, 10 deletions(-)

index 86bd457..5b69dec 100644
@@ -373,15 +373,6 @@ static inline struct pxa3xx_gcu_priv *to_pxa3xx_gcu_priv(struct file *file)
 	return container_of(dev, struct pxa3xx_gcu_priv, misc_dev);
 }
 
-/*
- * provide an empty .open callback, so the core sets file->private_data
- * for us.
- */
-static int pxa3xx_gcu_open(struct inode *inode, struct file *file)
-{
-	return 0;
-}
-
 static ssize_t
 pxa3xx_gcu_write(struct file *file, const char *buff,
 		 size_t count, loff_t *offp)

@@ -580,7 +571,6 @@ pxa3xx_gcu_free_buffers(struct device *dev,
 
 static const struct file_operations pxa3xx_gcu_miscdev_fops = {
 	.owner =		THIS_MODULE,
-	.open =			pxa3xx_gcu_open,
 	.write =		pxa3xx_gcu_write,
 	.unlocked_ioctl =	pxa3xx_gcu_ioctl,
 	.mmap =			pxa3xx_gcu_mmap,

2.1.4

We depend on private_data being NULL to record transaction ioctl start and end. This allows the misc core's misc_open() to do whatever it wants.

---
 fs/btrfs/super.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

index 05fef19..00dd2a6 100644
@@ -1993,6 +1993,16 @@ static int btrfs_show_devname(struct seq_file *m, struct dentry *root)
 	return 0;
 }
 
+static int btrfs_control_open(struct inode *inode, struct file *file)
+{
+	/*
+	 * we use private_data in the transaction start ioctl to record
+	 * it being started.
+	 */
+	file->private_data = NULL;
+	return 0;
+}
+
 static const struct super_operations btrfs_super_ops = {
 	.drop_inode	= btrfs_drop_inode,
 	.evict_inode	= btrfs_evict_inode,

@@ -2013,6 +2023,7 @@ static const struct file_operations btrfs_ctl_fops = {
 	.compat_ioctl = btrfs_control_ioctl,
 	.owner	 = THIS_MODULE,
 	.llseek = noop_llseek,
+	.open = btrfs_control_open,
 };
 
 static struct miscdevice btrfs_misc = {

2.1.4

There is a proposed change to the miscdevice's behaviour on open(). Currently file->private_data stays NULL, but only because we don't have an open-entry in struct file_operations.

This may change so that private_data, more consistently, is always set to struct miscdevice, not only *if* the driver has it's own open() routine and fops-entry, see https://lkml.org/lkml/2014/12/4/939

In short: If we rely on file->private_data being NULL, we should ensure it is NULL ourselves.

---
 drivers/lguest/lguest_user.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index c4c6113..30c6068 100644
@@ -339,6 +339,13 @@ static ssize_t write(struct file *file, const char __user *in,
 	}
 }
 
+static int open(struct inode *inode, struct file *file)
+{
+	file->private_data = NULL;
+
+	return 0;
+}
+
 /*L:060
  * The final piece of interface code is the close() routine.  It reverses
  * everything done in initialize().  This is usually called because the

@@ -409,6 +416,7 @@ static int close(struct inode *inode, struct file *file)
  */
 static const struct file_operations lguest_fops = {
 	.owner	 = THIS_MODULE,
+	.open	 = open,
 	.release = close,
 	.write	 = write,
 	.read	 = read,

2.1.4

The fix for FUSE is in 4.0-rc5 and linux-next. Btrfs was not affected, in the end. All kernel code has been checked for potential regressions.

So in my opinion, the time has come to apply this patch to char-misc so it can get some more testing in linux-next...

Thanks,

Tom.

What is "this patch"? I don't see it in the email :)

Please resend it, I don't have it in my patch queue anymore.

thanks,

greg k-h