Re: [PATCH] Change paride driver to use unlocked_ioctl instead of ioctl

Re: [PATCH] Change paride driver to use unlocked_ioctl instead of ioctl

[Christoph Hellwig]

On Thu, Jan 10, 2008 at 11:44:20AM +0530, Nikanth Karthikesan wrote:
> -static int pt_ioctl(struct inode *inode, struct file *file,
> -	 unsigned int cmd, unsigned long arg)
> +static long pt_ioctl(struct file *file, unsigned int cmd, unsigned long
> arg)

this looks line-wrapper by your mailer.

> -		if (copy_from_user(&mtop, p, sizeof(struct mtop)))
> +		if (copy_from_user(&mtop, p, sizeof(struct mtop))) {
> +			unlock_kernel();
>  			return -EFAULT;
> +		}
> 
>  		switch (mtop.mt_op) {
> 
>  		case MTREW:
>  			pt_rewind(tape);
> +			unlock_kernel();

It's generally considered good style to only have as few as possible
return values.  And this is especially important when returning from
a section that's under a lock.  So in this case it would be much better
if you changes this function to have a local 'int error' variable
and then just do

	error = -EFOO;
	goto out_unlock;

wherever you have an early return with the end of the function looking
like


out_unlock:
	unlock_kernel();
	return error;

Re: [PATCH] Change paride driver to use unlocked_ioctl instead of ioctl

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]

On Wed, 09 Jan 2008 08:06:20 GMT, Christoph Hellwig said:

> It's generally considered good style to only have as few as possible
> return values.  And this is especially important when returning from
> a section that's under a lock.  So in this case it would be much better
> if you changes this function to have a local 'int error' variable
> and then just do
> 
> 	error = -EFOO;
> 	goto out_unlock;

I think Christoph meant to say "as few as possible return locations".  One
should write the code to have as many different return values as are
meaningful, but return them from as few places as possible - which is what the
"assign error and goto end" paradigm does...


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] Change paride driver to use unlocked_ioctl instead of ioctl

[Jan Engelhardt]

On Jan 10 2008 14:25, Nikanth Karthikesan wrote:
>-static int pt_ioctl(struct inode *inode, struct file *file,
>-	 unsigned int cmd, unsigned long arg)
>+static long pt_ioctl(struct file *file, unsigned int cmd,
>+		unsigned long arg)
> {
> 	struct pt_unit *tape = file->private_data;
> 	struct mtop __user *p = (void __user *)arg;
> 	struct mtop mtop;
>+	long err = 0;
>+
>+	lock_kernel();
> 
> 	switch (cmd) {
> 	case MTIOCTOP:
>-		if (copy_from_user(&mtop, p, sizeof(struct mtop)))
>-			return -EFAULT;
>+		if (copy_from_user(&mtop, p, sizeof(struct mtop))) {
>+			err = -EFAULT;
>+			break;
>+		}

I wonder why a simple copy_from_user() requires the BKL.. if pt
does need locking, then probably some mutex inside pt.

Re: [PATCH] Change paride driver to use unlocked_ioctl instead of ioctl

[Christoph Hellwig]

On Wed, Jan 09, 2008 at 01:14:25PM +0100, Jan Engelhardt wrote:
> I wonder why a simple copy_from_user() requires the BKL.. if pt
> does need locking, then probably some mutex inside pt.

Given this is a janitorial project where the people don't even have
the hardware to test it's best to do a 1:1 conversion instead of making
it more complex.

Re: [PATCH] Change paride driver to use unlocked_ioctl instead of ioctl

[Matthew Wilcox]

On Thu, Jan 10, 2008 at 12:53:04PM +0530, Nikanth Karthikesan wrote:
>  	default:
>  		printk("%s: Unimplemented ioctl 0x%x\n", tape->name, cmd);
> +		unlock_kernel();
>  		return -EINVAL;

Surely a bug ... shouldn't this return -ENOTTY?

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Change paride driver to use unlocked_ioctl instead of ioctl

[Jiri Kosina]

On Wed, 9 Jan 2008, Matthew Wilcox wrote:

> >  	default:
> >  		printk("%s: Unimplemented ioctl 0x%x\n", tape->name, cmd);
> > +		unlock_kernel();
> >  		return -EINVAL;
> Surely a bug ... shouldn't this return -ENOTTY?

What POSIX states [1]:

[EINVAL]
    The request or arg argument is not valid for this device.

[ENOTTY]
    The fildes argument is not associated with a STREAMS device that
    accepts control functions.

[1] http://www.opengroup.org/onlinepubs/009695399/functions/ioctl.html

-- 
Jiri Kosina

Re: [PATCH] Change paride driver to use unlocked_ioctl instead of ioctl

[Alan Cox]

On Wed, 9 Jan 2008 14:26:26 +0100 (CET)
Jiri Kosina <jikos@jikos.cz> wrote:

> On Wed, 9 Jan 2008, Matthew Wilcox wrote:
> 
> > >  	default:
> > >  		printk("%s: Unimplemented ioctl 0x%x\n", tape->name, cmd);
> > > +		unlock_kernel();
> > >  		return -EINVAL;
> > Surely a bug ... shouldn't this return -ENOTTY?

Agreed - ENOTTY. The printk is wrong as well, but this is just getting
shown up by the other changes.

Does anyone even use parallel IDE devices any more ?

Re: [PATCH] Change paride driver to use unlocked_ioctl instead of ioctl

[Nikanth Karthikesan]

On Wed, 2008-01-09 at 16:56 +0000, Alan Cox wrote:
> On Wed, 9 Jan 2008 14:26:26 +0100 (CET)
> Jiri Kosina <jikos@jikos.cz> wrote:
> 
> > On Wed, 9 Jan 2008, Matthew Wilcox wrote:
> > 
> > > >  	default:
> > > >  		printk("%s: Unimplemented ioctl 0x%x\n", tape->name, cmd);
> > > > +		unlock_kernel();
> > > >  		return -EINVAL;
> > > Surely a bug ... shouldn't this return -ENOTTY?
> 
> Agreed - ENOTTY. The printk is wrong as well, but this is just getting
> shown up by the other changes.
> 
> Does anyone even use parallel IDE devices any more ?

Resending the patch. Also made the locks a little tighter, but I am not
sure whether the locks are required.

The ioctl handler is called with the BKL held. Registering
unlocked_ioctl handler instead of registering ioctl handler.
Also changed the return value to -ENOTTY for invalid ioctls.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---


diff --git a/drivers/block/paride/pt.c b/drivers/block/paride/pt.c
index b91accf..b7215fa 100644
--- a/drivers/block/paride/pt.c
+++ b/drivers/block/paride/pt.c
@@ -146,6 +146,7 @@ static int (*drives[4])[6] = {&drive0, &drive1, &drive2, &drive3};
 #include <linux/mtio.h>
 #include <linux/device.h>
 #include <linux/sched.h>	/* current, TASK_*, schedule_timeout() */
+#include <linux/smp_lock.h>
 
 #include <asm/uaccess.h>
 
@@ -189,8 +190,8 @@ module_param_array(drive3, int, NULL, 0);
 #define ATAPI_LOG_SENSE		0x4d
 
 static int pt_open(struct inode *inode, struct file *file);
-static int pt_ioctl(struct inode *inode, struct file *file,
-		    unsigned int cmd, unsigned long arg);
+static long pt_ioctl(struct file *file, unsigned int cmd,
+			unsigned long arg);
 static int pt_release(struct inode *inode, struct file *file);
 static ssize_t pt_read(struct file *filp, char __user *buf,
 		       size_t count, loff_t * ppos);
@@ -236,7 +237,7 @@ static const struct file_operations pt_fops = {
 	.owner = THIS_MODULE,
 	.read = pt_read,
 	.write = pt_write,
-	.ioctl = pt_ioctl,
+	.unlocked_ioctl = pt_ioctl,
 	.open = pt_open,
 	.release = pt_release,
 };
@@ -685,8 +686,8 @@ out:
 	return err;
 }
 
-static int pt_ioctl(struct inode *inode, struct file *file,
-	 unsigned int cmd, unsigned long arg)
+static long pt_ioctl(struct file *file, unsigned int cmd,
+	unsigned long arg)
 {
 	struct pt_unit *tape = file->private_data;
 	struct mtop __user *p = (void __user *)arg;
@@ -700,11 +701,15 @@ static int pt_ioctl(struct inode *inode, struct file *file,
 		switch (mtop.mt_op) {
 
 		case MTREW:
+			lock_kernel();
 			pt_rewind(tape);
+			unlock_kernel();
 			return 0;
 
 		case MTWEOF:
+			lock_kernel();
 			pt_write_fm(tape);
+			unlock_kernel();
 			return 0;
 
 		default:
@@ -714,8 +719,8 @@ static int pt_ioctl(struct inode *inode, struct file *file,
 		}
 
 	default:
-		printk("%s: Unimplemented ioctl 0x%x\n", tape->name, cmd);
-		return -EINVAL;
+		printk("%s: Invalid ioctl 0x%x\n", tape->name, cmd);
+		return -ENOTTY;
 
 	}
 }

[PATCH] Change paride driver to use unlocked_ioctl instead of ioctl

[Nikanth Karthikesan]

The ioctl handler is called with the BKL held. Registering
unlocked_ioctl handler instead of registering ioctl handler.


Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

diff --git a/arch/x86/kernel/cpu/mcheck/mce_64.c
b/arch/x86/kernel/cpu/mcheck/mce_64.c
diff --git a/drivers/block/paride/pt.c b/drivers/block/paride/pt.c
index b91accf..d4fa468 100644
--- a/drivers/block/paride/pt.c
+++ b/drivers/block/paride/pt.c
@@ -236,7 +236,7 @@ static const struct file_operations pt_fops = {
 	.owner = THIS_MODULE,
 	.read = pt_read,
 	.write = pt_write,
-	.ioctl = pt_ioctl,
+	.unlocked_ioctl = pt_ioctl,
 	.open = pt_open,
 	.release = pt_release,
 };
@@ -685,36 +685,43 @@ out:
 	return err;
 }

-static int pt_ioctl(struct inode *inode, struct file *file,
-	 unsigned int cmd, unsigned long arg)
+static long pt_ioctl(struct file *file, unsigned int cmd, unsigned long
arg)
 {
 	struct pt_unit *tape = file->private_data;
 	struct mtop __user *p = (void __user *)arg;
 	struct mtop mtop;

+	lock_kernel();
+
 	switch (cmd) {
 	case MTIOCTOP:
-		if (copy_from_user(&mtop, p, sizeof(struct mtop)))
+		if (copy_from_user(&mtop, p, sizeof(struct mtop))) {
+			unlock_kernel();
 			return -EFAULT;
+		}

 		switch (mtop.mt_op) {

 		case MTREW:
 			pt_rewind(tape);
+			unlock_kernel();
 			return 0;

 		case MTWEOF:
 			pt_write_fm(tape);
+			unlock_kernel();
 			return 0;

 		default:
 			printk("%s: Unimplemented mt_op %d\n", tape->name,
 			       mtop.mt_op);
+			unlock_kernel();
 			return -EINVAL;
 		}

 	default:
 		printk("%s: Unimplemented ioctl 0x%x\n", tape->name, cmd);
+		unlock_kernel();
 		return -EINVAL;

 	}

Re: [PATCH] Change paride driver to use unlocked_ioctl instead of ioctl

[Nikanth Karthikesan]

Sorry missed the function prototype and includes earlier.
Here is the corrected patch. Build tested.

The ioctl handler is called with the BKL held. Registering
unlocked_ioctl handler instead of registering ioctl handler.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

diff --git a/drivers/block/paride/pt.c b/drivers/block/paride/pt.c
index b91accf..860b946 100644
--- a/drivers/block/paride/pt.c
+++ b/drivers/block/paride/pt.c
@@ -146,6 +146,7 @@ static int (*drives[4])[6] = {&drive0, &drive1, &drive2,
&drive3};
 #include <linux/mtio.h>
 #include <linux/device.h>
 #include <linux/sched.h>	/* current, TASK_*, schedule_timeout() */
+#include <linux/smp_lock.h>

 #include <asm/uaccess.h>

@@ -189,8 +190,8 @@ module_param_array(drive3, int, NULL, 0);
 #define ATAPI_LOG_SENSE		0x4d

 static int pt_open(struct inode *inode, struct file *file);
-static int pt_ioctl(struct inode *inode, struct file *file,
-		    unsigned int cmd, unsigned long arg);
+static long pt_ioctl(struct file *file, unsigned int cmd,
+		unsigned long arg);
 static int pt_release(struct inode *inode, struct file *file);
 static ssize_t pt_read(struct file *filp, char __user *buf,
 		       size_t count, loff_t * ppos);
@@ -236,7 +237,7 @@ static const struct file_operations pt_fops = {
 	.owner = THIS_MODULE,
 	.read = pt_read,
 	.write = pt_write,
-	.ioctl = pt_ioctl,
+	.unlocked_ioctl = pt_ioctl,
 	.open = pt_open,
 	.release = pt_release,
 };
@@ -685,36 +686,44 @@ out:
 	return err;
 }

-static int pt_ioctl(struct inode *inode, struct file *file,
-	 unsigned int cmd, unsigned long arg)
+static long pt_ioctl(struct file *file, unsigned int cmd,
+		unsigned long arg)
 {
 	struct pt_unit *tape = file->private_data;
 	struct mtop __user *p = (void __user *)arg;
 	struct mtop mtop;

+	lock_kernel();
+
 	switch (cmd) {
 	case MTIOCTOP:
-		if (copy_from_user(&mtop, p, sizeof(struct mtop)))
+		if (copy_from_user(&mtop, p, sizeof(struct mtop))) {
+			unlock_kernel();
 			return -EFAULT;
+		}

 		switch (mtop.mt_op) {

 		case MTREW:
 			pt_rewind(tape);
+			unlock_kernel();
 			return 0;

 		case MTWEOF:
 			pt_write_fm(tape);
+			unlock_kernel();
 			return 0;

 		default:
 			printk("%s: Unimplemented mt_op %d\n", tape->name,
 			       mtop.mt_op);
+			unlock_kernel();
 			return -EINVAL;
 		}

 	default:
 		printk("%s: Unimplemented ioctl 0x%x\n", tape->name, cmd);
+		unlock_kernel();
 		return -EINVAL;

 	}

Re: [PATCH] Change paride driver to use unlocked_ioctl instead of ioctl

[Nikanth Karthikesan]

On Wed, 2008-01-09 at 03:23 -0500, Valdis.Kletnieks@vt.edu wrote:
> On Wed, 09 Jan 2008 08:06:20 GMT, Christoph Hellwig said:
> 
> > It's generally considered good style to only have as few as possible
> > return values.  And this is especially important when returning from
> > a section that's under a lock.  So in this case it would be much better
> > if you changes this function to have a local 'int error' variable
> > and then just do
> > 
> > 	error = -EFOO;
> > 	goto out_unlock;

agreed. resending the patch


> I think Christoph meant to say "as few as possible return locations".  One
> should write the code to have as many different return values as are
> meaningful, but return them from as few places as possible - which is what the
> "assign error and goto end" paradigm does...
> 

Is this following coding style, fine?

The ioctl handler is called with the BKL held. Registering
unlocked_ioctl handler instead of registering ioctl handler.

Signed-off-by: Nikanth Karthikesan <knikanth@suse.de>

---

diff --git a/drivers/block/paride/pt.c b/drivers/block/paride/pt.c
index b91accf..17f32f0 100644
--- a/drivers/block/paride/pt.c
+++ b/drivers/block/paride/pt.c
@@ -146,6 +146,7 @@ static int (*drives[4])[6] = {&drive0, &drive1, &drive2, &drive3};
 #include <linux/mtio.h>
 #include <linux/device.h>
 #include <linux/sched.h>	/* current, TASK_*, schedule_timeout() */
+#include <linux/smp_lock.h>
 
 #include <asm/uaccess.h>
 
@@ -189,8 +190,8 @@ module_param_array(drive3, int, NULL, 0);
 #define ATAPI_LOG_SENSE		0x4d
 
 static int pt_open(struct inode *inode, struct file *file);
-static int pt_ioctl(struct inode *inode, struct file *file,
-		    unsigned int cmd, unsigned long arg);
+static long pt_ioctl(struct file *file, unsigned int cmd,
+		unsigned long arg);
 static int pt_release(struct inode *inode, struct file *file);
 static ssize_t pt_read(struct file *filp, char __user *buf,
 		       size_t count, loff_t * ppos);
@@ -236,7 +237,7 @@ static const struct file_operations pt_fops = {
 	.owner = THIS_MODULE,
 	.read = pt_read,
 	.write = pt_write,
-	.ioctl = pt_ioctl,
+	.unlocked_ioctl = pt_ioctl,
 	.open = pt_open,
 	.release = pt_release,
 };
@@ -685,39 +686,47 @@ out:
 	return err;
 }
 
-static int pt_ioctl(struct inode *inode, struct file *file,
-	 unsigned int cmd, unsigned long arg)
+static long pt_ioctl(struct file *file, unsigned int cmd,
+		unsigned long arg)
 {
 	struct pt_unit *tape = file->private_data;
 	struct mtop __user *p = (void __user *)arg;
 	struct mtop mtop;
+	long err = 0;
+
+	lock_kernel();
 
 	switch (cmd) {
 	case MTIOCTOP:
-		if (copy_from_user(&mtop, p, sizeof(struct mtop)))
-			return -EFAULT;
+		if (copy_from_user(&mtop, p, sizeof(struct mtop))) {
+			err = -EFAULT;
+			break;
+		}
 
 		switch (mtop.mt_op) {
 
 		case MTREW:
 			pt_rewind(tape);
-			return 0;
+			break;
 
 		case MTWEOF:
 			pt_write_fm(tape);
-			return 0;
+			break;
 
 		default:
 			printk("%s: Unimplemented mt_op %d\n", tape->name,
 			       mtop.mt_op);
-			return -EINVAL;
+			err = -EINVAL;
 		}
+		break;
 
 	default:
 		printk("%s: Unimplemented ioctl 0x%x\n", tape->name, cmd);
-		return -EINVAL;
-
+		err = -EINVAL;
+		
 	}
+	unlock_kernel();
+	return err;
 }
 
 static int

Re: [PATCH] Change paride driver to use unlocked_ioctl instead of ioctl

[Jiri Kosina]

On Wed, 9 Jan 2008, Alan Cox wrote:

> > > >  	default:
> > > >  		printk("%s: Unimplemented ioctl 0x%x\n", tape->name, cmd);
> > > > +		unlock_kernel();
> > > >  		return -EINVAL;
> > > Surely a bug ... shouldn't this return -ENOTTY?
> Agreed - ENOTTY. 

Just out of curiosity, where does POSIX happen to specify ENOTTY as the 
correct one for unimplemented ioctl?

-- 
Jiri Kosina

Re: [PATCH] Change paride driver to use unlocked_ioctl instead of ioctl

[Nikanth Karthikesan]

On Thu, 2008-01-10 at 16:01 +0100, Jiri Kosina wrote:
> On Wed, 9 Jan 2008, Alan Cox wrote:
> 
> > > > >  	default:
> > > > >  		printk("%s: Unimplemented ioctl 0x%x\n", tape->name, cmd);
> > > > > +		unlock_kernel();
> > > > >  		return -EINVAL;
> > > > Surely a bug ... shouldn't this return -ENOTTY?
> > Agreed - ENOTTY. 
> 
> Just out of curiosity, where does POSIX happen to specify ENOTTY as the 
> correct one for unimplemented ioctl?
> 

The printk is also wrong, It should have been, Invalid ioctl for the
device

Re: [PATCH] Change paride driver to use unlocked_ioctl instead of ioctl

[Matthew Wilcox]

On Thu, Jan 10, 2008 at 09:01:41PM +0530, Nikanth Karthikesan wrote:
> On Thu, 2008-01-10 at 16:01 +0100, Jiri Kosina wrote:
> > On Wed, 9 Jan 2008, Alan Cox wrote:
> > 
> > > > > >  	default:
> > > > > >  		printk("%s: Unimplemented ioctl 0x%x\n", tape->name, cmd);
> > > > > > +		unlock_kernel();
> > > > > >  		return -EINVAL;
> > > > > Surely a bug ... shouldn't this return -ENOTTY?
> > > Agreed - ENOTTY. 
> > 
> > Just out of curiosity, where does POSIX happen to specify ENOTTY as the 
> > correct one for unimplemented ioctl?
> > 
> 
> The printk is also wrong, It should have been, Invalid ioctl for the
> device

It shouldn't print anything.  That printk lets unprivileged users DoS
the syslog.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Change paride driver to use unlocked_ioctl instead of ioctl

[Alan Cox]

On Thu, 10 Jan 2008 16:01:44 +0100 (CET)
Jiri Kosina <jikos@jikos.cz> wrote:

> On Wed, 9 Jan 2008, Alan Cox wrote:
> 
> > > > >  	default:
> > > > >  		printk("%s: Unimplemented ioctl 0x%x\n", tape->name, cmd);
> > > > > +		unlock_kernel();
> > > > >  		return -EINVAL;
> > > > Surely a bug ... shouldn't this return -ENOTTY?
> > Agreed - ENOTTY. 
> 
> Just out of curiosity, where does POSIX happen to specify ENOTTY as the 
> correct one for unimplemented ioctl?

I don't know if POSIX does, but Unix has always used ENOTTY for "I don't
know what this ioctl is" and -EINVAL "for I know what this ioctl is but
the values passed are stupid"