The ioctl is empty and needs no serialization. We might remove it
completely but that would change the return value from -ENOIOCTLCMD to
-ENOTTY.
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Jeff Dike <[email protected]>
---
arch/um/drivers/mmapper_kern.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Index: linux-2.6-tip/arch/um/drivers/mmapper_kern.c
===================================================================
--- linux-2.6-tip.orig/arch/um/drivers/mmapper_kern.c
+++ linux-2.6-tip/arch/um/drivers/mmapper_kern.c
@@ -46,8 +46,8 @@ static ssize_t mmapper_write(struct file
return count;
}
-static int mmapper_ioctl(struct inode *inode, struct file *file,
- unsigned int cmd, unsigned long arg)
+static long mmapper_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
{
return -ENOIOCTLCMD;
}
@@ -91,7 +91,7 @@ static const struct file_operations mmap
.owner = THIS_MODULE,
.read = mmapper_read,
.write = mmapper_write,
- .ioctl = mmapper_ioctl,
+ .unlocked_ioctl = mmapper_ioctl,
.mmap = mmapper_mmap,
.open = mmapper_open,
.release = mmapper_release,
On Thursday 15 October 2009, Thomas Gleixner wrote:
> The ioctl is empty and needs no serialization. We might remove it
> completely but that would change the return value from -ENOIOCTLCMD to
> -ENOTTY.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Cc: Jeff Dike <[email protected]>
This one is tricky if you want to get it right according to the
book. ENOIOCTLCMD is never a valid return code for user space,
but sys_ioctl passes it down anyway.
However, returning -ENOIOCTLCMD from an *unlocked_ioctl* function
automatically gets turned into -EINVAL. It does this to allow
the same functions to be used for unlocked_ioctl and compat_ioctl.
In effect, this patch is functionally identical to removing the
ioctl function, which I think is what should be done here.
Arnd <><
On Thu, 15 Oct 2009, Arnd Bergmann wrote:
> On Thursday 15 October 2009, Thomas Gleixner wrote:
> > The ioctl is empty and needs no serialization. We might remove it
> > completely but that would change the return value from -ENOIOCTLCMD to
> > -ENOTTY.
> >
> > Signed-off-by: Thomas Gleixner <[email protected]>
> > Cc: Jeff Dike <[email protected]>
>
> This one is tricky if you want to get it right according to the
> book. ENOIOCTLCMD is never a valid return code for user space,
> but sys_ioctl passes it down anyway.
>
> However, returning -ENOIOCTLCMD from an *unlocked_ioctl* function
> automatically gets turned into -EINVAL. It does this to allow
> the same functions to be used for unlocked_ioctl and compat_ioctl.
> In effect, this patch is functionally identical to removing the
> ioctl function, which I think is what should be done here.
Yeah, we get either -ENOTTY or -EINVAL which always changes the user
space return value. So yes, removing it at least saves some line of
code :)
Thanks,
tglx
On Thu, 15 Oct 2009 15:00:34 +0200
Arnd Bergmann <[email protected]> wrote:
> On Thursday 15 October 2009, Thomas Gleixner wrote:
> > The ioctl is empty and needs no serialization. We might remove it
> > completely but that would change the return value from -ENOIOCTLCMD to
> > -ENOTTY.
> >
> > Signed-off-by: Thomas Gleixner <[email protected]>
> > Cc: Jeff Dike <[email protected]>
>
> This one is tricky if you want to get it right according to the
> book. ENOIOCTLCMD is never a valid return code for user space,
> but sys_ioctl passes it down anyway.
>
> However, returning -ENOIOCTLCMD from an *unlocked_ioctl* function
> automatically gets turned into -EINVAL. It does this to allow
> the same functions to be used for unlocked_ioctl and compat_ioctl.
> In effect, this patch is functionally identical to removing the
> ioctl function, which I think is what should be done here.
That is wrong.
SuS requires an unknown ioctl code returns -ENOTTY. If the code is
currently remapping it to EINVAL then it wants fixing.
On Thursday 15 October 2009, Alan Cox wrote:
> On Thu, 15 Oct 2009 15:00:34 +0200
> Arnd Bergmann <[email protected]> wrote:
>
> > However, returning -ENOIOCTLCMD from an *unlocked_ioctl* function
> > automatically gets turned into -EINVAL. It does this to allow
> > the same functions to be used for unlocked_ioctl and compat_ioctl.
> > In effect, this patch is functionally identical to removing the
> > ioctl function, which I think is what should be done here.
>
> That is wrong.
>
> SuS requires an unknown ioctl code returns -ENOTTY. If the code is
> currently remapping it to EINVAL then it wants fixing.
Right, I forgot about the EINVAL/ENOTTY difference. The code currently
returns -ENOIOCTLCMD, which is worse. Thomas' patch makes it return
-EINVAL, which as you said is still wrong. Removing the ioctl function
will do the right thing and return -ENOTTY, so that should be done
here in um/mmapper, with an appropriate changelog.
For the common code in fs/ioctl.c, I think the current behaviour is
correct. It returns -EINVAL if the driver returns -ENOIOCTLCMD, iow
"the request [...] argument is not valid for this device", as specified
by http://www.opengroup.org/onlinepubs/009695399/functions/ioctl.html.
Drivers returning ENOIOCTLCMD for every request are broken and should
be changed to have no ioctl function.
Arnd <><
On Fri, 16 Oct 2009, Arnd Bergmann wrote:
> On Thursday 15 October 2009, Alan Cox wrote:
> > On Thu, 15 Oct 2009 15:00:34 +0200
> > Arnd Bergmann <[email protected]> wrote:
> >
> > > However, returning -ENOIOCTLCMD from an *unlocked_ioctl* function
> > > automatically gets turned into -EINVAL. It does this to allow
> > > the same functions to be used for unlocked_ioctl and compat_ioctl.
> > > In effect, this patch is functionally identical to removing the
> > > ioctl function, which I think is what should be done here.
> >
> > That is wrong.
> >
> > SuS requires an unknown ioctl code returns -ENOTTY. If the code is
> > currently remapping it to EINVAL then it wants fixing.
>
> Right, I forgot about the EINVAL/ENOTTY difference. The code currently
> returns -ENOIOCTLCMD, which is worse. Thomas' patch makes it return
> -EINVAL, which as you said is still wrong. Removing the ioctl function
> will do the right thing and return -ENOTTY, so that should be done
> here in um/mmapper, with an appropriate changelog.
>
> For the common code in fs/ioctl.c, I think the current behaviour is
> correct. It returns -EINVAL if the driver returns -ENOIOCTLCMD, iow
Only the unlocked_ioctl code path, the locked one returns whatever
crap comes from the ioctl implementation.
> "the request [...] argument is not valid for this device", as specified
> by http://www.opengroup.org/onlinepubs/009695399/functions/ioctl.html.
> Drivers returning ENOIOCTLCMD for every request are broken and should
> be changed to have no ioctl function.
Ack.
tglx