Messages in this thread | | | Date | Wed, 27 Apr 2011 21:54:00 +0800 | From | Lifeng Sun <> | Subject | Re: [PATCH] Applying inappropriate ioctl operation on socket should return ENOTTY |
| |
On 13:09 Wed 04/27/11 Apr, Alan Cox wrote: > > diff --git a/drivers/char/applicom.c b/drivers/char/applicom.c > > index 25373df..50c09e4 100644 > > --- a/drivers/char/applicom.c > > +++ b/drivers/char/applicom.c > > @@ -838,6 +838,6 @@ static long ac_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > > Dummy = readb(apbs[IndexCard].RamIO + VERS); > > kfree(adgl); > > mutex_unlock(&ac_mutex); > > - return 0; > > + return ret; > > } > > This one in fact is a bug fix where 0 gets returned not an error code it > ought to be submitted separately and described as such.
Will do.
> > > diff --git a/drivers/char/dtlk.c b/drivers/char/dtlk.c > > index 85156dd..2d116d5 100644 > > --- a/drivers/char/dtlk.c > > +++ b/drivers/char/dtlk.c > > @@ -289,7 +289,7 @@ static long dtlk_ioctl(struct file *file, > > return put_user(portval, argp); > > > > default: > > - return -EINVAL; > > + return -ENOTTY; > > } > > } > > This one looks good (and the driver has another error in the ioctl > handler too that wants fixing where it returnds -EINVAL not -EFAULT)
Will fix.
> > diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c > > index d72433f..4ba9b9f 100644 > > --- a/drivers/char/i8k.c > > +++ b/drivers/char/i8k.c > > @@ -370,7 +370,7 @@ i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg) > > break; > > > > default: > > - return -EINVAL; > > + return -ENOTTY; > > This one is incomplete - the driver also has a bogus check for arg being > non zero. That means ioctl(fd, BOGUS, 0) will return the wrong error code > still.
Will fix.
> > diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c > > index 2aa3977..bc8af5a 100644 > > --- a/drivers/char/ipmi/ipmi_devintf.c > > +++ b/drivers/char/ipmi/ipmi_devintf.c > > @@ -232,7 +232,7 @@ static int ipmi_ioctl(struct file *file, > > unsigned int cmd, > > unsigned long data) > > { > > - int rv = -EINVAL; > > + int rv = -ENOTTY; > > struct ipmi_file_private *priv = file->private_data; > > void __user *arg = (void __user *)data; > > No - there are cases that should return -EINVAL that this will break - a > default case needs adding
All execution paths overwrite the return value except those should return -ENOTTY, but it's more clear to add a default case.
Will do.
> > diff --git a/drivers/char/viotape.c b/drivers/char/viotape.c > > index ad6e64a..a427d40 100644 > > --- a/drivers/char/viotape.c > > +++ b/drivers/char/viotape.c > > @@ -529,7 +529,7 @@ static int viotap_ioctl(struct inode *inode, struct file *file, > > > > down(&reqSem); > > > > - ret = -EINVAL; > > + ret = -ENOTTY; > > Again this messes up the returns because code assumes the initial > default.
Likewise, except the unsupported MTIOCPOS command. SuSv4 has two appropriate errno's for this unsupported case and the one below returns EOPNOTSUPP,
[ENOTSUP] Not supported (may be the same value as [EOPNOTSUPP]).
[EOPNOTSUPP] Operation not supported on socket (may be the same value as [ENOTSUP]).
but the manpage of ioctl disagree. I am wondering how to handle unsupported ioctl operations. Maybe following the manpage is a better choice though it's not exact.
> The original code also has bugs too (wrong error off copy_*_user() > again)
Will fix.
> > diff --git a/fs/pipe.c b/fs/pipe.c > > index da42f7d..fe7ffe4 100644 > > --- a/fs/pipe.c > > +++ b/fs/pipe.c > > @@ -665,7 +665,7 @@ static long pipe_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > > > > return put_user(count, (int __user *)arg); > > default: > > - return -EINVAL; > > + return -ENOTTY; > > } > > Looks good - but this one really does want to be a patch on its own as if > anything causes compatibility funnies it will be this, and we need to be > sure we can bisect nicely to it should this occur.
will submit serparately.
> > @@ -5041,7 +5041,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg) > > /* Set the per device memory buffer space. > > * Not applicable in our case */ > > case SIOCSIFLINK: > > - return -EINVAL; > > + return -EOPNOTSUPP; > > This change seems unrelated to anything in your description and outside > of anything SuS cares about or demands.
As stated above. I would submit separately.
- Lifeng
--
| |