[PATCH] btrfs: Don't check for file->private_data on open(). It is set by the core.

[PATCH] btrfs: Don't check for file->private_data on open(). It is set by the core.

[Martin Kepplinger]

The miscdevice core now sets file->private_data to the struct miscdevice
so don't fail when this is not NULL.

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
This is a question: what does this check provide and does overwriting
file->private_data make any difference?

Is miscdevice's open() by the user not allowed here, if file->private_data
is set?

thanks!!

 fs/btrfs/ioctl.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 4399f0c..066ce41 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3752,10 +3752,6 @@ static long btrfs_ioctl_trans_start(struct file *file)
 	if (!capable(CAP_SYS_ADMIN))
 		goto out;
 
-	ret = -EINPROGRESS;
-	if (file->private_data)
-		goto out;
-
 	ret = -EROFS;
 	if (btrfs_root_readonly(root))
 		goto out;
-- 
1.7.10.4

Re: [PATCH] btrfs: Don't check for file->private_data on open(). It is set by the core.

[Chris Mason]

On Wed, Nov 12, 2014 at 11:38 AM, Martin Kepplinger <martink@posteo.de> 
wrote:
> The miscdevice core now sets file->private_data to the struct 
> miscdevice
> so don't fail when this is not NULL.
> 
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> ---
> This is a question: what does this check provide and does overwriting
> file->private_data make any difference?
> 
> Is miscdevice's open() by the user not allowed here, if 
> file->private_data
> is set?
> 
> thanks!!

Btrfs uses this in the transaction start ioctl to record the 
transaction handle being started.  Ceph is the main user of the ioctl, 
and we could setup a hash table if needed.  But which call path in 
miscdevice is doing this?

With your patch in place, btrfs would end up overwriting the miscdevice 
private_data field, which would probably cause problems.

-chris

Re: [PATCH] btrfs: Don't check for file->private_data on open(). It is set by the core.

[Martin Kepplinger]

Am 2014-11-12 um 18:59 schrieb Chris Mason:
> On Wed, Nov 12, 2014 at 11:38 AM, Martin Kepplinger <martink@posteo.de>
> wrote:
>> The miscdevice core now sets file->private_data to the struct miscdevice
>> so don't fail when this is not NULL.
>>
>> Signed-off-by: Martin Kepplinger <martink@posteo.de>
>> ---
>> This is a question: what does this check provide and does overwriting
>> file->private_data make any difference?
>>
>> Is miscdevice's open() by the user not allowed here, if
>> file->private_data
>> is set?
>>
>> thanks!!
> 
> Btrfs uses this in the transaction start ioctl to record the transaction
> handle being started.  Ceph is the main user of the ioctl, and we could
> setup a hash table if needed.  But which call path in miscdevice is
> doing this?
> 
> With your patch in place, btrfs would end up overwriting the miscdevice
> private_data field, which would probably cause problems.
> 
> -chris
> 

I think i was mistaken, sorry. misc_open() used to set
file->private_data _only_ if you use set .open in struct file_operations.

In current -next this changed and file->private_data is set to struct
miscdevice on a (userspace's) open call (misc_open()) just in any case.

You do set .open so this wouldn't affect you and this patch can be ignored.

                        martin

Re: [PATCH] btrfs: Don't check for file->private_data on open(). It is set by the core.

[Al Viro]

On Wed, Nov 12, 2014 at 07:34:50PM +0100, Martin Kepplinger wrote:

> > Btrfs uses this in the transaction start ioctl to record the transaction
> > handle being started.  Ceph is the main user of the ioctl, and we could
> > setup a hash table if needed.  But which call path in miscdevice is
> > doing this?
> > 
> > With your patch in place, btrfs would end up overwriting the miscdevice
> > private_data field, which would probably cause problems.
> > 
> > -chris
> > 
> 
> I think i was mistaken, sorry. misc_open() used to set
> file->private_data _only_ if you use set .open in struct file_operations.
> 
> In current -next this changed and file->private_data is set to struct
> miscdevice on a (userspace's) open call (misc_open()) just in any case.
> 
> You do set .open so this wouldn't affect you and this patch can be ignored.

More to the point, this function is not reachable from anything in
file_operations of any miscdevice.  btrfs_ioctl != btrfs_control_ioctl...