The ioctl handler is called with the BKL held. Registering
unlocked_ioctl handler instead of registering ioctl handler.
Signed-off-by: Nikanth Karthikesan <[email protected]>
---
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;
}
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 <[email protected]>
---
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;
}
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;
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...
On Wed, 2008-01-09 at 03:23 -0500, [email protected] 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 <[email protected]>
---
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
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.
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.
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."
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
On Wed, 9 Jan 2008 14:26:26 +0100 (CET)
Jiri Kosina <[email protected]> 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 ?
On Wed, 2008-01-09 at 16:56 +0000, Alan Cox wrote:
> On Wed, 9 Jan 2008 14:26:26 +0100 (CET)
> Jiri Kosina <[email protected]> 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 <[email protected]>
---
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;
}
}
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
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
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."
On Thu, 10 Jan 2008 16:01:44 +0100 (CET)
Jiri Kosina <[email protected]> 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"