[RFC] Remove or convert empty ioctls ?

[RFC] Remove or convert empty ioctls ?

[Thomas Gleixner]

Hi,

while looking into pushing down BKL to ioctls I noticed that we have a
lot of ioctls which simply return -EINVAL or some other fancy error
code.

The question is whether we should convert them to unlocked_ioctl or
simply remove them and let the sys_ioctl code return the default
-ENOTTY error code.

One could argue that this is a user visible change, but OTOH there is
no particular value of EINVAL or any other weird error code when it
just says: there is no ioctl for this fd.

Thanks,

	tglx

Re: [RFC] Remove or convert empty ioctls ?

[Jeff Garzik]

On 10/15/2009 05:20 AM, Thomas Gleixner wrote:
> Hi,
>
> while looking into pushing down BKL to ioctls I noticed that we have a
> lot of ioctls which simply return -EINVAL or some other fancy error
> code.
>
> The question is whether we should convert them to unlocked_ioctl or
> simply remove them and let the sys_ioctl code return the default
> -ENOTTY error code.
>
> One could argue that this is a user visible change, but OTOH there is
> no particular value of EINVAL or any other weird error code when it
> just says: there is no ioctl for this fd.

It seems like a mistake to generalize a rule out of this, especially if 
it leads to error return values changing unexpectedly in years-old code.

"no particular value" is highly subjective, and I think unprovable, 
without an exhaustive survey of userland programs interacting with 
kernel drivers.  Userland programs often interact with a -class- of 
drivers, expecting predictable behavior from a DoThisThing ioctl, with 
EINVAL or "other weird error code" returned intentionally.

Changing the return codes seems quite unwise.

	Jeff

Re: [RFC] Remove or convert empty ioctls ?

[Alan Cox]

> "no particular value" is highly subjective, and I think unprovable, 
> without an exhaustive survey of userland programs interacting with 
> kernel drivers.  Userland programs often interact with a -class- of 
> drivers, expecting predictable behavior from a DoThisThing ioctl, with 
> EINVAL or "other weird error code" returned intentionally.
> 
> Changing the return codes seems quite unwise.

We've changed lots of them to -ENOTTY over the past few years, nobody has
even noticed (you included ;))

SuS says an unknown ioctl code returns -ENOTTY.

Alan

Re: [RFC] Remove or convert empty ioctls ?

[Jeff Garzik]

On 10/15/2009 11:01 AM, Alan Cox wrote:
>> "no particular value" is highly subjective, and I think unprovable,
>> without an exhaustive survey of userland programs interacting with
>> kernel drivers.  Userland programs often interact with a -class- of
>> drivers, expecting predictable behavior from a DoThisThing ioctl, with
>> EINVAL or "other weird error code" returned intentionally.
>>
>> Changing the return codes seems quite unwise.
>
> We've changed lots of them to -ENOTTY over the past few years, nobody has
> even noticed (you included ;))
>
> SuS says an unknown ioctl code returns -ENOTTY.

These are not unknown ioctls; they are ioctls that the driver author 
close to implement rather than the default (ENOTTY).

	Jeff

Re: [RFC] Remove or convert empty ioctls ?

[Alan Cox]

On Thu, 15 Oct 2009 11:22:40 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> On 10/15/2009 11:01 AM, Alan Cox wrote:
> >> "no particular value" is highly subjective, and I think unprovable,
> >> without an exhaustive survey of userland programs interacting with
> >> kernel drivers.  Userland programs often interact with a -class- of
> >> drivers, expecting predictable behavior from a DoThisThing ioctl, with
> >> EINVAL or "other weird error code" returned intentionally.
> >>
> >> Changing the return codes seems quite unwise.
> >
> > We've changed lots of them to -ENOTTY over the past few years, nobody has
> > even noticed (you included ;))
> >
> > SuS says an unknown ioctl code returns -ENOTTY.
> 
> These are not unknown ioctls; they are ioctls that the driver author 
> close to implement rather than the default (ENOTTY).

That makes them ENOTTY please go and read the standards document.

EINVAL means you used an ioctl that is correct for the driver but that
for some reason the driver didn't like it.

Re: [RFC] Remove or convert empty ioctls ?

[Jeff Garzik]

On 10/15/2009 11:31 AM, Alan Cox wrote:
> EINVAL means you used an ioctl that is correct for the driver but that
> for some reason the driver didn't like it.

Precisely.

The driver author proactively chose to implement the ioctl and return a 
value other than ENOTTY.

It is invalid to assume that all such cases are a direct result of 
author mistakes or ignorance.

	Jeff

Re: [RFC] Remove or convert empty ioctls ?

[Alan Cox]

On Thu, 15 Oct 2009 11:35:45 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> On 10/15/2009 11:31 AM, Alan Cox wrote:
> > EINVAL means you used an ioctl that is correct for the driver but that
> > for some reason the driver didn't like it.
> 
> Precisely.
> 
> The driver author proactively chose to implement the ioctl and return a 
> value other than ENOTTY.

Jeff - wake up - take coffee if neeeded.

The case in question is the case where ->unlocked_ioctl() == NULL. In
that case the driver author didn't implement anything and the correct
return from the layer doing the handling is -ENOTTY.

In the case where there is an ioctl handler the common 

foo_ioctl()
{
	return -EINVAL;
}

stub case is simply broken, and caused by the fact a lot of early Linux
drivers were wrong and people keep propogating mistakes (a serious
problem in the open source world is that most code is produced by copying
stuff but often by copying buggy code).

Essentially the only times you should be returning -EINVAL is where the
driver actually has


	case IOCWIBBLE:
		/* Do stuff */
		if (foo < 1)
			return -EINVAL;

All the "I don't know this" cases should be -ENOTTY.

There are cases where

	case IOCWIBBLE:
		return -EINVAL;

is correct but they are unusual - basically the case where the driver
author wants to "support" the ioctl but there is no argument that it can
be given which is correct.


Anyway the case discussed which is ".unlocked_ioctl = NULL" should return
-ENOTTY, and there isn't any argument about the driver authors intentions.

Re: [RFC] Remove or convert empty ioctls ?

[Jeff Garzik]

On 10/15/2009 11:49 AM, Alan Cox wrote:
> There are cases where
>
> 	case IOCWIBBLE:
> 		return -EINVAL;
>
> is correct but they are unusual - basically the case where the driver
> author wants to "support" the ioctl but there is no argument that it can
> be given which is correct.

This is the type of case I was referring to.

	Jeff

Re: [RFC] Remove or convert empty ioctls ?

[Thomas Gleixner]

On Thu, 15 Oct 2009, Jeff Garzik wrote:

> On 10/15/2009 11:49 AM, Alan Cox wrote:
> > There are cases where
> > 
> > 	case IOCWIBBLE:
> > 		return -EINVAL;
> > 
> > is correct but they are unusual - basically the case where the driver
> > author wants to "support" the ioctl but there is no argument that it can
> > be given which is correct.
> 
> This is the type of case I was referring to.

There is no real reason to believe that

int ioctl()
{
	return -EINVAL;
}

fall into above category.

Thanks,

	tglx

Re: [RFC] Remove or convert empty ioctls ?

[Thomas Gleixner]

On Thu, 15 Oct 2009, Alan Cox wrote:
> Anyway the case discussed which is ".unlocked_ioctl = NULL" should return
> -ENOTTY, and there isn't any argument about the driver authors intentions.

I think we got some confusion finally. :)

If both unlocked_ioctl and ioctl are NULL the return code is -ENOTTY.

We have locked ioctl functions which return -ENOIOCTLCMD. vfs_ioctl()
returns that to user space, but for unlocked_ioctl it is translated to
-EINVAL. I guess we need to fix that either in the ioctl
implementations or let vfs_ioctl() translate it for locked ioctls as
well.

The other category of ioctls (both locked and unlocked) are the stub
functions which simply return -EINVAL or -ENOIOCTLCMD.

The spec says:
    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.

So for the stub ioctl functions EINVAL is a correct return value
because the driver has an ioctl function, but does not handle the
request.

But I completely agree, that we should remove those stubs simply
because they handle no request at all which is basically the same as
no ioctl function.

Thanks,

	tglx

Re: [RFC] Remove or convert empty ioctls ?

[Ingo Molnar]

* Jeff Garzik <jeff@garzik.org> wrote:

> On 10/15/2009 11:31 AM, Alan Cox wrote:
>> EINVAL means you used an ioctl that is correct for the driver but that
>> for some reason the driver didn't like it.
>
> Precisely.
>
> The driver author proactively chose to implement the ioctl and return 
> a value other than ENOTTY.
>
> It is invalid to assume that all such cases are a direct result of 
> author mistakes or ignorance.

If 10% of the cases the error return was for a reason it's still worth 
doing this. If it matters we'll hear about it.

Something that looks like crap should not get extra protection to stay 
in the kernel just because it 'might' be non-crap. If it's not clearly 
documented in the source to be -EINVAL for a good reason (and frankly i 
cannot think of any) then i'd suggest to do what Alan has been doing for 
years: just change it and see if it causes any problems.

It's not like it's hard to revert such a change. (and we'll also add a 
comment explaining the reason for the -EINVAL in that case.)

	Ingo