[PATCH] Misc: pass miscdevice through file's private_data

[PATCH] Misc: pass miscdevice through file's private_data

[Tom Van Braeckel]

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.

Signed-off-by: Tom Van Braeckel <tomvanbraeckel@gmail.com>
---
 drivers/char/misc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index ffa97d2..c3681fb 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -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

Re: [PATCH] Misc: pass miscdevice through file's private_data

[Greg KH]

On Thu, Dec 04, 2014 at 10:08:15PM +0100, Tom Van Braeckel wrote:
> 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.
> 
> Signed-off-by: Tom Van Braeckel <tomvanbraeckel@gmail.com>
> ---
>  drivers/char/misc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> index ffa97d2..c3681fb 100644
> --- a/drivers/char/misc.c
> +++ b/drivers/char/misc.c
> @@ -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);
>  	}

These braces aren't needed anymore, right?

Also, what is this now going to break?  :)

thanks,

greg k-h

Re: [PATCH] Misc: pass miscdevice through file's private_data

[Tom Van Braeckel]

> >  
> > +	/*
> > +	 * 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);
> >  	}
>
> These braces aren't needed anymore, right?
>
> Also, what is this now going to break?  :)
> 
> thanks,
> 
> greg k-h

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.

>From this investigation, I conclude that it does not break anything.

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.

Signed-off-by: Tom Van Braeckel <tomvanbraeckel@gmail.com>
---
 drivers/char/misc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index ffa97d2..d6445de 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -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

Re: [PATCH] Misc: pass miscdevice through file's private_data

[Greg KH]

On Fri, Dec 05, 2014 at 12:01:03AM +0100, Tom Van Braeckel wrote:
> > >  
> > > +	/*
> > > +	 * 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);
> > >  	}
> >
> > These braces aren't needed anymore, right?
> >
> > Also, what is this now going to break?  :)
> > 
> > thanks,
> > 
> > greg k-h
> 
> 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.
> 
> >From this investigation, I conclude that it does not break anything.
> 
> 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.
> 
> Signed-off-by: Tom Van Braeckel <tomvanbraeckel@gmail.com>
> ---
>  drivers/char/misc.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

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

thanks,

greg k-h

[PATCH] misc: pass miscdevice through file's private_data

[Tom Van Braeckel]

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.

Signed-off-by: Tom Van Braeckel <tomvanbraeckel@gmail.com>
---
 drivers/char/misc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index ffa97d2..d6445de 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -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

Re: [PATCH] misc: pass miscdevice through file's private_data

[Greg KH]

On Fri, Dec 05, 2014 at 05:37:54AM +0100, Tom Van Braeckel wrote:
> 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.
> 
> Signed-off-by: Tom Van Braeckel <tomvanbraeckel@gmail.com>
> ---
>  drivers/char/misc.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> index ffa97d2..d6445de 100644
> --- a/drivers/char/misc.c
> +++ b/drivers/char/misc.c
> @@ -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

Re: [PATCH] misc: pass miscdevice through file's private_data

[Tom Van Braeckel]

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

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.

always assign miscdevice to file->private_data

[Martin Kepplinger]

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!

[PATCH 1/4] char: misc: document behaviour of open()

[Martin Kepplinger]

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.

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/char/misc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 32ab943..c05e2f4 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -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

[PATCH 2/4] fbdev: pxa3xx-gcu: remove redundant implementation of open()

[Martin Kepplinger]

the misc core does this now in any case.

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/video/fbdev/pxa3xx-gcu.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/video/fbdev/pxa3xx-gcu.c b/drivers/video/fbdev/pxa3xx-gcu.c
index 86bd457..5b69dec 100644
--- a/drivers/video/fbdev/pxa3xx-gcu.c
+++ b/drivers/video/fbdev/pxa3xx-gcu.c
@@ -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

[PATCH 3/4] fs: btrfs: set file->private data NULL after open() because we depend on it

[Martin Kepplinger]

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.

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 fs/btrfs/super.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 05fef19..00dd2a6 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -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

[PATCH 4/4] lguest: explicitly set miscdevice's private_data NULL

[Martin Kepplinger]

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.

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/lguest/lguest_user.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/lguest/lguest_user.c b/drivers/lguest/lguest_user.c
index c4c6113..30c6068 100644
--- a/drivers/lguest/lguest_user.c
+++ b/drivers/lguest/lguest_user.c
@@ -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

Re: [PATCH] misc: pass miscdevice through file's private_data

[Tom Van Braeckel]

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

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.

Re: [PATCH] misc: pass miscdevice through file's private_data

[Greg KH]

On Tue, Mar 31, 2015 at 03:08:38PM +0200, Tom Van Braeckel wrote:
> >> Wait, didn't this break FUSE?  Or was that some other variant of this
> >> patch?
> 
> 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...

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

[PATCH] misc: pass miscdevice through file's private_data

[Tom Van Braeckel]

Make the miscdevice accessible through the file's private_data.

Previously, this was done only when an open() file operation had been
registered. If no custom open() file operation was defined,
private_data was set to NULL.

This subtle quirk was confusing, to the point where kernel code
registered *empty* file open operations to have private_data point to
the misc device structure and avoid duplicating that logic.

And it could easily lead to bugs, where the addition or removal of a
custom open() file operation surprisingly changes the initial value of
a file's private_data structure.

To resolve this, we now place the miscdevice in the file's private_data
member unconditionally when open() is called.

Signed-off-by: Tom Van Braeckel <tomvanbraeckel@gmail.com>
---
All kernel code that uses private_data and misc_register() has been
checked for potential regressions and the code that relied on this
subtle behavior (only FUSE) has been fixed.

Although I feel confident about this patch, it might still be a good
idea to keep this in linux-next for at least a whole cycle, as Martin
Kepplinger suggested. As you wish.

Mind that the documentation about this change has already been
submitted by Martin and accepted into linux-next (commit 03190c67).

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

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 5bb3a21..9fd5a91 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -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;
-- 
2.1.0