Change paride driver to use unlocked_ioctl instead of ioctl

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

---

b/arch/x86/kernel/cpu/mcheck/mce_64.c

index b91accf..d4fa468 100644
@@ -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;

}

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.

---

index b91accf..860b946 100644
@@ -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;

}

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

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 ?

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

Jiri Kosina

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"

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."

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.

---

index b91accf..b7215fa 100644
@@ -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;
 
 	}
 }

this looks line-wrapper by your mailer.

	error = -EFOO;
	goto out_unlock;

	unlock_kernel();
	return error;

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...

agreed. resending the patch

Is this following coding style, fine?

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

---

index b91accf..17f32f0 100644
@@ -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

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.