Re: [PATCH] Applying inappropriate ioctl operation on socket should return ENOTTY

Re: [PATCH] Applying inappropriate ioctl operation on socket should return ENOTTY

[Eric Dumazet]

Le mercredi 27 avril 2011 à 13:37 +0800, Lifeng Sun a écrit :
> ioctl() calls against a socket with an inappropriate ioctl operation
> are incorrectly returning EINVAL rather than ENOTTY:
> 
>   [ENOTTY]
>       Inappropriate I/O control operation.
> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=33992
> 
> This bug is not limited to socket, it also occurs in a lot of, maybe
> some hundred, other ioctl operations, while in the patch I only fixed
> about a dozen of additional ones in pipe, fifo and character device
> drivers.

Really ?

EINVAL is ok too : Request or argp is not valid.

I would say, its not a bug as you claim. 

Its really too late to make such change and risk regressions.

isatty(fd) performs well. Please use it instead.

Also, networking patches should be sent to netdev@vger.kernel.org and
David Miller, as mentioned in MAINTAINERS file.

Re: [PATCH] Applying inappropriate ioctl operation on socket should return ENOTTY

[Lifeng Sun]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

On 07:58 Wed 04/27/11 Apr, Eric Dumazet wrote:
> Really ?
> 
> EINVAL is ok too : Request or argp is not valid.

I'm afraid not. SUSv4 specifies, say,

  int tcsetattr(int fildes, int optional_actions,
         const struct termios *termios_p);

 ERROR:
  [EINVAL]
      The optional_actions argument is not a supported value, or an
      attempt was made to change an attribute represented in the
      termios structure to an unsupported value.

  [ENOTTY]
      The file associated with fildes is not a terminal.

which means when we apply tcsetattr (implemented by ioctl) to _any_
non-terminal file descriptor, it should set errno to ENOTTY rather
than EINVAL.

> I would say, its not a bug as you claim. 
> 
> Its really too late to make such change and risk regressions.
> 
> isatty(fd) performs well. Please use it instead.
> 
> Also, networking patches should be sent to netdev@vger.kernel.org and
> David Miller, as mentioned in MAINTAINERS file.

Thank you.

-- 

[-- Attachment #2: GnuPG digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] Applying inappropriate ioctl operation on socket should return ENOTTY

[Eric Dumazet]

Le mercredi 27 avril 2011 à 14:37 +0800, Lifeng Sun a écrit :
> On 07:58 Wed 04/27/11 Apr, Eric Dumazet wrote:
> > Really ?
> > 
> > EINVAL is ok too : Request or argp is not valid.
> 
> I'm afraid not. SUSv4 specifies, say,
> 
>   int tcsetattr(int fildes, int optional_actions,
>          const struct termios *termios_p);
> 
>  ERROR:
>   [EINVAL]
>       The optional_actions argument is not a supported value, or an
>       attempt was made to change an attribute represented in the
>       termios structure to an unsupported value.
> 
>   [ENOTTY]
>       The file associated with fildes is not a terminal.
> 
> which means when we apply tcsetattr (implemented by ioctl) to _any_
> non-terminal file descriptor, it should set errno to ENOTTY rather
> than EINVAL.

Thats not so simple. This is a known and documented artifact.

In old days, ioctl() had a meaning for TTYS (mostly).



man isatty

ERRORS
       EBADF  fd is not a valid file descriptor.

       EINVAL fd refers to a file other than a terminal.  POSIX.1-2001 specifies the error ENOTTY for this case.


This is not because POSIX changes rules that we must change kernel and break applications.

Conformant applications use isatty(fd) and test result code being 1 or not 1

This way, they work with linux 1.0, 2.0, 2.2, 2.4, .... and other OSes as well.

Re: [PATCH] Applying inappropriate ioctl operation on socket should return ENOTTY

[Eric Dumazet]

Le mercredi 27 avril 2011 à 14:37 +0800, Lifeng Sun a écrit :
> On 07:58 Wed 04/27/11 Apr, Eric Dumazet wrote:
> > Really ?
> > 
> > EINVAL is ok too : Request or argp is not valid.
> 
> I'm afraid not. SUSv4 specifies, say,
> 
>   int tcsetattr(int fildes, int optional_actions,
>          const struct termios *termios_p);
> 
>  ERROR:
>   [EINVAL]
>       The optional_actions argument is not a supported value, or an
>       attempt was made to change an attribute represented in the
>       termios structure to an unsupported value.
> 
>   [ENOTTY]
>       The file associated with fildes is not a terminal.
> 
> which means when we apply tcsetattr (implemented by ioctl) to _any_
> non-terminal file descriptor, it should set errno to ENOTTY rather
> than EINVAL.
> 

You quote manpage for a library call, not a system call.

If you feel your glibc doesnt implement well this, please complain to
glibc maintainer.

Re: [PATCH] Applying inappropriate ioctl operation on socket should return ENOTTY

[Lifeng Sun]

On 08:57 Wed 04/27/11 Apr, Eric Dumazet wrote:
> You quote manpage for a library call, not a system call.

okay, let me quote the one for ioctl system call:

man 2 ioctl

int ioctl(int d, int request, ...);

ERRORS
   EBADF  d is not a valid descriptor.
   EFAULT argp references an inaccessible memory area.
   EINVAL Request or argp is not valid.
   ENOTTY d is not associated with a character special device.
   ENOTTY The specified request does not apply to the kind of object
     that the descriptor d references.

we see ENOTTY and EFAULT refine EVINAL and it should return ENOTTY or
EFAULT whenever possible rather than EINVAL, otherwise we could always
return EBADF or EINVAL.

Regarding to isatty, well, it's only a library call, isn't it? :-) If
you insist on the significance of the manpage of isatty, there are
also a lot of ioctl operations return ENOTTY, if not less than those
return EINVAL, for inappropriated command and eventually violate the
ERRORS section of the manpage. Certainly we could complain to c
library maintainers.


> If you feel your glibc doesnt implement well this, please complain to
> glibc maintainer.


-- 

[PATCH] Applying inappropriate ioctl operation on socket should return ENOTTY

[Lifeng Sun]

ioctl() calls against a socket with an inappropriate ioctl operation
are incorrectly returning EINVAL rather than ENOTTY:

  [ENOTTY]
      Inappropriate I/O control operation.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=33992

This bug is not limited to socket, it also occurs in a lot of, maybe
some hundred, other ioctl operations, while in the patch I only fixed
about a dozen of additional ones in pipe, fifo and character device
drivers.

Signed-off-by: Lifeng Sun <lifongsun@gmail.com>
---
 drivers/char/applicom.c          |    2 +-
 drivers/char/dtlk.c              |    2 +-
 drivers/char/generic_nvram.c     |    2 +-
 drivers/char/genrtc.c            |    2 +-
 drivers/char/hpet.c              |    2 +-
 drivers/char/i8k.c               |    2 +-
 drivers/char/ipmi/ipmi_devintf.c |    2 +-
 drivers/char/lp.c                |    2 +-
 drivers/char/nwflash.c           |    2 +-
 drivers/char/ppdev.c             |    2 +-
 drivers/char/random.c            |    2 +-
 drivers/char/raw.c               |    4 ++--
 drivers/char/viotape.c           |    2 +-
 fs/pipe.c                        |    2 +-
 net/core/dev.c                   |    6 +++---
 15 files changed, 18 insertions(+), 18 deletions(-)

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;
 }
 
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;
 	}
 }
 
diff --git a/drivers/char/generic_nvram.c b/drivers/char/generic_nvram.c
index 0e941b5..95278e9 100644
--- a/drivers/char/generic_nvram.c
+++ b/drivers/char/generic_nvram.c
@@ -111,7 +111,7 @@ static int nvram_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		nvram_sync();
 		break;
 	default:
-		return -EINVAL;
+		return -ENOTTY;
 	}
 
 	return 0;
diff --git a/drivers/char/genrtc.c b/drivers/char/genrtc.c
index f773a9d..6f4c3da 100644
--- a/drivers/char/genrtc.c
+++ b/drivers/char/genrtc.c
@@ -330,7 +330,7 @@ static int gen_rtc_ioctl(struct file *file,
 	    }
 	}
 
-	return -EINVAL;
+	return -ENOTTY;
 }
 
 static long gen_rtc_unlocked_ioctl(struct file *file, unsigned int cmd,
diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index 7066e80..720de66 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -575,7 +575,7 @@ hpet_ioctl_common(struct hpet_dev *devp, int cmd, unsigned long arg,
 	case HPET_IE_ON:
 		return hpet_ioctl_ieon(devp);
 	default:
-		return -EINVAL;
+		return -ENOTTY;
 	}
 
 	err = 0;
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;
 	}
 
 	if (val < 0)
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;
 
diff --git a/drivers/char/lp.c b/drivers/char/lp.c
index 97c3edb..2ff32c8 100644
--- a/drivers/char/lp.c
+++ b/drivers/char/lp.c
@@ -650,7 +650,7 @@ static int lp_do_ioctl(unsigned int minor, unsigned int cmd,
 			break;
 
 		default:
-			retval = -EINVAL;
+			retval = -ENOTTY;
 	}
 	return retval;
 }
diff --git a/drivers/char/nwflash.c b/drivers/char/nwflash.c
index a12f524..45b7a7a 100644
--- a/drivers/char/nwflash.c
+++ b/drivers/char/nwflash.c
@@ -115,7 +115,7 @@ static long flash_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 		gbWriteBase64Enable = 0;
 		gbWriteEnable = 0;
 		mutex_unlock(&flash_mutex);
-		return -EINVAL;
+		return -ENOTTY;
 	}
 	mutex_unlock(&flash_mutex);
 	return 0;
diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
index f176dba..8dce214 100644
--- a/drivers/char/ppdev.c
+++ b/drivers/char/ppdev.c
@@ -622,7 +622,7 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 	default:
 		pr_debug(CHRDEV "%x: What? (cmd=0x%x)\n", minor, cmd);
-		return -EINVAL;
+		return -ENOTTY;
 	}
 
 	/* Keep the compiler happy */
diff --git a/drivers/char/random.c b/drivers/char/random.c
index d4ddeba..40aad1c 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1157,7 +1157,7 @@ static long random_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
 		rand_initialize();
 		return 0;
 	default:
-		return -EINVAL;
+		return -ENOTTY;
 	}
 }
 
diff --git a/drivers/char/raw.c b/drivers/char/raw.c
index b4b9d5a..a992bf1 100644
--- a/drivers/char/raw.c
+++ b/drivers/char/raw.c
@@ -231,7 +231,7 @@ static long raw_ctl_ioctl(struct file *filp, unsigned int command,
 		return 0;
 	}
 
-	return -EINVAL;
+	return -ENOTTY;
 }
 
 #ifdef CONFIG_COMPAT
@@ -273,7 +273,7 @@ static long raw_ctl_compat_ioctl(struct file *file, unsigned int cmd,
 		return 0;
 	}
 
-	return -EINVAL;
+	return -ENOTTY;
 }
 #endif
 
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;
 
 	switch (cmd) {
 	case MTIOCTOP:
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;
 	}
 }
 
diff --git a/net/core/dev.c b/net/core/dev.c
index c2ac599..b93c76d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4773,7 +4773,7 @@ static int dev_ifsioc_locked(struct net *net, struct ifreq *ifr, unsigned int cm
 		 * is never reached
 		 */
 		WARN_ON(1);
-		err = -EINVAL;
+		err = -ENOTTY;
 		break;
 
 	}
@@ -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;
 
 	/*
 	 *	Unknown or private ioctl.
@@ -5062,7 +5062,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
 		/* Take care of Wireless Extensions */
 		if (cmd >= SIOCIWFIRST && cmd <= SIOCIWLAST)
 			return wext_handle_ioctl(net, &ifr, cmd, arg);
-		return -EINVAL;
+		return -ENOTTY;
 	}
 }
 
-- 
1.7.5.rc1

Re: [PATCH] Applying inappropriate ioctl operation on socket should return ENOTTY

[Alan Cox]

> EINVAL is ok too : Request or argp is not valid.

It confuses portable code in some situations
 
> I would say, its not a bug as you claim. 

POSIX and SuS tend to disagree

> Its really too late to make such change and risk regressions.

We've been quietly doing it for hundreds of cases including the entire
tty driver. Almost nobody (you included) has actually noticed and in
doing so we fixed various porting funnies without any reported
regressions.

Networking may be a more tricky one (not that tty wasn't a large one we
fixed) but most of the other driver ones are clearly sensible.

Alan

Re: [PATCH] Applying inappropriate ioctl operation on socket should return ENOTTY

[Eric Dumazet]

Le mercredi 27 avril 2011 à 10:47 +0100, Alan Cox a écrit :
> > EINVAL is ok too : Request or argp is not valid.
> 
> It confuses portable code in some situations
>  
> > I would say, its not a bug as you claim. 
> 
> POSIX and SuS tend to disagree
> 
> > Its really too late to make such change and risk regressions.
> 
> We've been quietly doing it for hundreds of cases including the entire
> tty driver. Almost nobody (you included) has actually noticed and in
> doing so we fixed various porting funnies without any reported
> regressions.
> 
> Networking may be a more tricky one (not that tty wasn't a large one we
> fixed) but most of the other driver ones are clearly sensible.
> 

Well, I wont argue the point, especially if you Ack the changes ;)

My only concern was to not break old applications, I dont know if it is
going to break _any_ of them. Probably these old applications stick with
old kernels.

If you ask me ENOTTY is plain wrong.
ioctl() is not restricted to terminal devices at all.

Any unknown ioctl command would return ENOTTY, regardless of fd being a
tty or not...

IF we add a new ioctl() to sockets in 2.6.42, using it on 2.6.41 would
give ENOTTY status, and EINVAL status on 2.6.24

Go figure...

Re: [PATCH] Applying inappropriate ioctl operation on socket should return ENOTTY

[Alan Cox]

> Well, I wont argue the point, especially if you Ack the changes ;)
> 
> My only concern was to not break old applications, I dont know if it is
> going to break _any_ of them. Probably these old applications stick with
> old kernels.

The number of applications that actually check ioctl error codes beyond
if error perror; return is pretty small and those that do generally do so
for very narrow cases or for things like EWOULDBLOCK/EINTR stuff.

> If you ask me ENOTTY is plain wrong.
> ioctl() is not restricted to terminal devices at all.

Like the tab/space thing in Makefiles and Qwerty keyboards its now part of
how stuff all works but EINVAL is even worse because you cannot tell
between 'this ioctl isn't know/is used on the wrong fd' and 'argument
wrong to valid ioctl'

Alan

Re: [PATCH] Applying inappropriate ioctl operation on socket should return ENOTTY

[Alan Cox]

> 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.

> 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)

>  
> diff --git a/drivers/char/generic_nvram.c b/drivers/char/generic_nvram.c
> index 0e941b5..95278e9 100644
> --- a/drivers/char/generic_nvram.c
> +++ b/drivers/char/generic_nvram.c
> @@ -111,7 +111,7 @@ static int nvram_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		nvram_sync();
>  		break;
>  	default:
> -		return -EINVAL;
> +		return -ENOTTY;
>  	}

Looks good

>  	return 0;
> diff --git a/drivers/char/genrtc.c b/drivers/char/genrtc.c
> index f773a9d..6f4c3da 100644
> --- a/drivers/char/genrtc.c
> +++ b/drivers/char/genrtc.c
> @@ -330,7 +330,7 @@ static int gen_rtc_ioctl(struct file *file,
>  	    }
>  	}
>  
> -	return -EINVAL;
> +	return -ENOTTY;
>  }

Likewise


>  static long gen_rtc_unlocked_ioctl(struct file *file, unsigned int cmd,
> diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
> index 7066e80..720de66 100644
> --- a/drivers/char/hpet.c
> +++ b/drivers/char/hpet.c
> @@ -575,7 +575,7 @@ hpet_ioctl_common(struct hpet_dev *devp, int cmd, unsigned long arg,
>  	case HPET_IE_ON:
>  		return hpet_ioctl_ieon(devp);
>  	default:
> -		return -EINVAL;
> +		return -ENOTTY;
>  	}

Ok

>  	err = 0;
> 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.

>  	}
>  
>  	if (val < 0)
> 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

> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> index 97c3edb..2ff32c8 100644
> --- a/drivers/char/lp.c
> +++ b/drivers/char/lp.c
> @@ -650,7 +650,7 @@ static int lp_do_ioctl(unsigned int minor, unsigned int cmd,
>  			break;
>  
>  		default:
> -			retval = -EINVAL;
> +			retval = -ENOTTY;
>  	}
>  	return retval;

Looks good

>  }
> diff --git a/drivers/char/nwflash.c b/drivers/char/nwflash.c
> index a12f524..45b7a7a 100644
> --- a/drivers/char/nwflash.c
> +++ b/drivers/char/nwflash.c
> @@ -115,7 +115,7 @@ static long flash_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>  		gbWriteBase64Enable = 0;
>  		gbWriteEnable = 0;
>  		mutex_unlock(&flash_mutex);
> -		return -EINVAL;
> +		return -ENOTTY;

Ok

>  	}
>  	mutex_unlock(&flash_mutex);
>  	return 0;
> diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
> index f176dba..8dce214 100644
> --- a/drivers/char/ppdev.c
> +++ b/drivers/char/ppdev.c
> @@ -622,7 +622,7 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  
>  	default:
>  		pr_debug(CHRDEV "%x: What? (cmd=0x%x)\n", minor, cmd);
> -		return -EINVAL;
> +		return -ENOTTY;
>  	}

Looks good

>  
>  	/* Keep the compiler happy */
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index d4ddeba..40aad1c 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1157,7 +1157,7 @@ static long random_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
>  		rand_initialize();
>  		return 0;
>  	default:
> -		return -EINVAL;
> +		return -ENOTTY;

Ok
>  	}
>  }
>  
> diff --git a/drivers/char/raw.c b/drivers/char/raw.c
> index b4b9d5a..a992bf1 100644
> --- a/drivers/char/raw.c
> +++ b/drivers/char/raw.c
> @@ -231,7 +231,7 @@ static long raw_ctl_ioctl(struct file *filp, unsigned int command,
>  		return 0;
>  	}
>  
> -	return -EINVAL;
> +	return -ENOTTY;

Ok

>  }
>  
>  #ifdef CONFIG_COMPAT
> @@ -273,7 +273,7 @@ static long raw_ctl_compat_ioctl(struct file *file, unsigned int cmd,
>  		return 0;
>  	}
>  
> -	return -EINVAL;
> +	return -ENOTTY;
>  }
>  #endif

Ok

>  
> 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. The original code also has bugs too (wrong error off
copy_*_user() again)

>  
>  	switch (cmd) {
>  	case MTIOCTOP:
> 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.

>  }
>  
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c2ac599..b93c76d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4773,7 +4773,7 @@ static int dev_ifsioc_locked(struct net *net, struct ifreq *ifr, unsigned int cm
>  		 * is never reached
>  		 */
>  		WARN_ON(1);
> -		err = -EINVAL;
> +		err = -ENOTTY;

This case doesn't really matter - it can't happen anyway so might as well
change
>  		break;
>  
>  	}
> @@ -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.
>  
>  	/*
>  	 *	Unknown or private ioctl.
> @@ -5062,7 +5062,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
>  		/* Take care of Wireless Extensions */
>  		if (cmd >= SIOCIWFIRST && cmd <= SIOCIWLAST)
>  			return wext_handle_ioctl(net, &ifr, cmd, arg);
> -		return -EINVAL;
> +		return -ENOTTY;

and this one looks right.

Alan

Re: [PATCH] Applying inappropriate ioctl operation on socket should return ENOTTY

[Lifeng Sun]

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

-- 

[PATCH 1/5] networking: inappropriate ioctl operation should return ENOTTY

[Lifeng Sun]

ioctl() calls against a socket with an inappropriate ioctl operation
are incorrectly returning EINVAL rather than ENOTTY:

  [ENOTTY]
      Inappropriate I/O control operation.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=33992

Signed-off-by: Lifeng Sun <lifongsun@gmail.com>
---
 net/core/dev.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index c2ac599..856b6ee 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4773,7 +4773,7 @@ static int dev_ifsioc_locked(struct net *net, struct ifreq *ifr, unsigned int cm
 		 * is never reached
 		 */
 		WARN_ON(1);
-		err = -EINVAL;
+		err = -ENOTTY;
 		break;
 
 	}
@@ -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 -ENOTTY;
 
 	/*
 	 *	Unknown or private ioctl.
@@ -5062,7 +5062,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
 		/* Take care of Wireless Extensions */
 		if (cmd >= SIOCIWFIRST && cmd <= SIOCIWLAST)
 			return wext_handle_ioctl(net, &ifr, cmd, arg);
-		return -EINVAL;
+		return -ENOTTY;
 	}
 }
 
-- 
1.7.5.rc1

Re: [PATCH 1/5] networking: inappropriate ioctl operation should return ENOTTY

[David Miller]

From: Lifeng Sun <lifongsun@gmail.com>
Date: Thu, 28 Apr 2011 16:04:51 +0800

> ioctl() calls against a socket with an inappropriate ioctl operation
> are incorrectly returning EINVAL rather than ENOTTY:
> 
>   [ENOTTY]
>       Inappropriate I/O control operation.
> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=33992
> 
> Signed-off-by: Lifeng Sun <lifongsun@gmail.com>

Applied, thanks.

Re: [PATCH] Applying inappropriate ioctl operation on socket should return ENOTTY

[Lifeng Sun]

On Wed, Apr 27, 2011 at 8:09 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> 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.

> The original code also has bugs too (wrong error off
> copy_*_user() again)

It seems alright.

- Lifeng