lkml.org 
[lkml]   [2011]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

--


\
 
 \ /
  Last update: 2011-04-27 15:57    [W:0.076 / U:7.076 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site