[PATCH] btrfs: explicitly set control file's private_data

[PATCH] btrfs: explicitly set control file's private_data

[Tom Van Braeckel]

The private_data member of the Btrfs control device file
(/dev/btrfs-control) is used to hold the current transaction and needs
to be initialized to NULL to signify that no transaction is in progress.

We explicitly set the control file's private_data to NULL to be
independent of whatever value the misc subsystem initializes it to.

Backstory:
----------

The misc subsystem (which is used by /dev/btrfs-control) initializes
a file's private_data to point to the misc device when a driver has
registered a custom open file operation and initializes it to NULL
when a custom open file operation has *not* been provided.

This subtle quirk is confusing, to the point where kernel code registers
*empty* file open operations to have private_data point to the misc
device structure.

And it leads to bugs, where the addition or removal of a custom open
file operation surprisingly changes the initial contents of a file's
private_data structure.

To simplify things in the misc subsystem, a patch [1] has been proposed
to *always* set private_data to point to the misc device instead of
only doing this when a custom open file operation has been registered.

But before we can fix this in the misc subsystem itself, we need to
modify the (few) drivers that rely on this very subtle behavior.

[1] https://lkml.org/lkml/2014/12/4/939

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

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 05fef19..3cfb5ee 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1908,6 +1908,16 @@ static struct file_system_type btrfs_fs_type = {
 };
 MODULE_ALIAS_FS("btrfs");
 
+static int btrfs_control_open(struct inode *inode, struct file *file)
+{
+	/* The control file's private_data is used to hold the
+	 * transaction when it is started and is used to keep
+	 * track of whether a transaction is already in progress.
+	 */
+	file->private_data = NULL;
+	return 0;
+}
+
 /*
  * used by btrfsctl to scan devices when no FS is mounted
  */
@@ -2009,6 +2019,7 @@ static const struct super_operations btrfs_super_ops = {
 };
 
 static const struct file_operations btrfs_ctl_fops = {
+	.open = btrfs_control_open,
 	.unlocked_ioctl	 = btrfs_control_ioctl,
 	.compat_ioctl = btrfs_control_ioctl,
 	.owner	 = THIS_MODULE,
-- 
2.1.0

Re: [PATCH] btrfs: explicitly set control file's private_data

[David Sterba]

On Mon, Mar 23, 2015 at 06:34:13PM +0100, Tom Van Braeckel wrote:
> The private_data member of the Btrfs control device file
> (/dev/btrfs-control) is used to hold the current transaction and needs
> to be initialized to NULL to signify that no transaction is in progress.
> 
> We explicitly set the control file's private_data to NULL to be
> independent of whatever value the misc subsystem initializes it to.
> 
> Backstory:
> ----------
> 
> The misc subsystem (which is used by /dev/btrfs-control) initializes
> a file's private_data to point to the misc device when a driver has
> registered a custom open file operation and initializes it to NULL
> when a custom open file operation has *not* been provided.
> 
> This subtle quirk is confusing, to the point where kernel code registers
> *empty* file open operations to have private_data point to the misc
> device structure.
> 
> And it leads to bugs, where the addition or removal of a custom open
> file operation surprisingly changes the initial contents of a file's
> private_data structure.
> 
> To simplify things in the misc subsystem, a patch [1] has been proposed
> to *always* set private_data to point to the misc device instead of
> only doing this when a custom open file operation has been registered.
> 
> But before we can fix this in the misc subsystem itself, we need to
> modify the (few) drivers that rely on this very subtle behavior.
> 
> [1] https://lkml.org/lkml/2014/12/4/939
> 
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> Signed-off-by: Tom Van Braeckel <tomvanbraeckel@gmail.com>

Thanks for the explanation.

Acked-by: David Sterba <dsterba@suse.cz>

> +static int btrfs_control_open(struct inode *inode, struct file *file)
> +{
> +	/* The control file's private_data is used to hold the
> +	 * transaction when it is started and is used to keep
> +	 * track of whether a transaction is already in progress.
> +	 */

That's not the common comment style (newline after /* ) but I'm never
sure whether I should nitpick about such things or just say yes to
fixes.

> +	file->private_data = NULL;
> +	return 0;
> +}

[PATCH v2] btrfs: explicitly set control file's private_data

[Tom Van Braeckel]

The private_data member of the Btrfs control device file
(/dev/btrfs-control) is used to hold the current transaction and needs
to be initialized to NULL to signify that no transaction is in progress.

We explicitly set the control file's private_data to NULL to be
independent of whatever value the misc subsystem initializes it to.

Backstory:
----------

The misc subsystem (which is used by /dev/btrfs-control) initializes
a file's private_data to point to the misc device when a driver has
registered a custom open file operation and initializes it to NULL
when a custom open file operation has *not* been provided.

This subtle quirk is confusing, to the point where kernel code registers
*empty* file open operations to have private_data point to the misc
device structure.

And it leads to bugs, where the addition or removal of a custom open
file operation surprisingly changes the initial contents of a file's
private_data structure.

To simplify things in the misc subsystem, a patch [1] has been proposed
to *always* set private_data to point to the misc device instead of
only doing this when a custom open file operation has been registered.

But before we can fix this in the misc subsystem itself, we need to
modify the (few) drivers that rely on this very subtle behavior.

[1] https://lkml.org/lkml/2014/12/4/939

Signed-off-by: Martin Kepplinger <martink@posteo.de>
Signed-off-by: Tom Van Braeckel <tomvanbraeckel@gmail.com>
---
 fs/btrfs/super.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 05fef19..3c30195 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1908,6 +1908,17 @@ static struct file_system_type btrfs_fs_type = {
 };
 MODULE_ALIAS_FS("btrfs");
 
+static int btrfs_control_open(struct inode *inode, struct file *file)
+{
+	/*
+	 * The control file's private_data is used to hold the
+	 * transaction when it is started and is used to keep
+	 * track of whether a transaction is already in progress.
+	 */
+	file->private_data = NULL;
+	return 0;
+}
+
 /*
  * used by btrfsctl to scan devices when no FS is mounted
  */
@@ -2009,6 +2020,7 @@ static const struct super_operations btrfs_super_ops = {
 };
 
 static const struct file_operations btrfs_ctl_fops = {
+	.open = btrfs_control_open,
 	.unlocked_ioctl	 = btrfs_control_ioctl,
 	.compat_ioctl = btrfs_control_ioctl,
 	.owner	 = THIS_MODULE,
-- 
2.1.0

Re: [PATCH v2] btrfs: explicitly set control file's private_data

[Tom Van Braeckel]

Err, upon further inspection, I think that this was a false positive.

Btrfs relies on the initial value of the private_data member of a file
being NULL in the regular ioctl operation handler for
BTRFS_IOC_TRANS_START but it does not use the miscdevice framework for
those files.

It *does* use the miscdevice framework in the ioctl operation handler
of the /dev/btrfs-control file but there it does not use the file's
private_data member. So IMHO, the proposed patch is not necessary...

Re: [PATCH v2] btrfs: explicitly set control file's private_data

[Martin Kepplinger]

Am 31.03.2015 14:31 schrieb Tom Van Braeckel:
> Err, upon further inspection, I think that this was a false positive.
> 
> Btrfs relies on the initial value of the private_data member of a file
> being NULL in the regular ioctl operation handler for
> BTRFS_IOC_TRANS_START but it does not use the miscdevice framework for
> those files.
> 
> It *does* use the miscdevice framework in the ioctl operation handler
> of the /dev/btrfs-control file but there it does not use the file's
> private_data member. So IMHO, the proposed patch is not necessary...

This is offtopic, assuming you are right and didn't find more affected 
places:

Then I would say you could re-post the real change (to misc_open() ) to 
the
relevant people for 4.2 (not 4.1), so either wait for 4.0 to be released 
or try
something like "for 4.2" in the topic (or as a comment after the --- 
dashes in
the patch email)

I would want to have it in -next for one cycle at least.

Further, I would remove the code-comment you had here
https://lkml.org/lkml/2015/1/9/718 because GregKH already pulled this in
(a little too early ;) :
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=03190c67ff72b5c56b24266762ab8abe68970f45
which is extractable kernel documenation. You could somehow link to it
in the commit message.

                                   martin