[PATCH 0/1] readlinkat() error code change for empty pathname

[PATCH 0/1] readlinkat() error code change for empty pathname

[Andy Whitcroft]

In the 3.0 kernel we seem to have a semantic change in the error
code returned by the readlink/readlinkat calls for the empty pathname.
Specifically is is now returning EINVAL whereas it used to return ENOENT.
This appears to be contrary to our documentation on the expected return
code for the empty pathname (path_resolution(7)):

   Empty pathname
       In the original Unix, the empty pathname referred to the current direc-
       tory.  Nowadays POSIX decrees  that  an  empty  pathname  must  not  be
       resolved successfully.  Linux returns ENOENT in this case.

It is also possible we are moving away from posix compliance if the
comments in the source are to believed (from the comments on do_getname
in fs/namei.c):

 * POSIX.1 2.4: an empty pathname is invalid (ENOENT).

This is causing a number of applications to fail to build in Ubuntu
(at least those including gnulib), mostly with test suite failures.
It seems that application writers are relying on this behaviour.

Doing a quick review of a selection of pathname related calls we also
seem now to be inconsistant in our return for readlink/readlinkat:

  open("", O_RDONLY) = -1 ENOENT (No such file or directory)
  chmod("", 0777) = -1 ENOENT (No such file or directory)
  readlink("", 0x7fff794533c0, 2048) = -1 EINVAL (Invalid argument)
  stat("", 0x7fff79453330) = -1 ENOENT (No such file or directory)
  rename("", "") = -1 ENOENT (No such file or directory)
  unlink("") = -1 ENOENT (No such file or directory)

This change was introduced in the commit below as part of the introduction
of O_PATH support, and occurs because the readlinkat() call does not have
a flags field to allow this to be an opt-in behaviour:

  commit 65cfc6722361570bfe255698d9cd4dccaf47570d
  Author: Al Viro <viro@zeniv.linux.org.uk>
  Date: Tue, 2 Aug 2011 15:16:23 +0100

    readlinkat(), fchownat() and fstatat() with empty relative pathnames
    
    For readlinkat() we simply allow empty pathname; it will fail unless
    we have dfd equal to O_PATH-opened symlink, so we are outside of
    POSIX scope here.  For fchownat() and fstatat() we allow AT_EMPTY_PATH;
    let the caller explicitly ask for such behaviour.
    
    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Following this email is a patch which attempts to paper over this
inconsistancy.  I am not very happy with it as it does expose some of
the internals of the getname processing.

Comments/thoughts/better ideas?

-apw

Andy Whitcroft (1):
  readlinkat: ensure we return ENOENT for the empty pathname for normal
    lookups

 fs/namei.c            |   24 +++++++++++++++++++-----
 fs/stat.c             |    5 +++--
 include/linux/namei.h |    1 +
 3 files changed, 23 insertions(+), 7 deletions(-)

-- 
1.7.4.1

[PATCH 1/1] readlinkat: ensure we return ENOENT for the empty pathname for normal lookups

[Andy Whitcroft]

Since the commit below which added O_PATH support to the *at() calls,
the error return for readlink/readlinkat for the empty pathname has
switched from ENOENT to EINVAL:

  commit 65cfc6722361570bfe255698d9cd4dccaf47570d
  Author: Al Viro <viro@zeniv.linux.org.uk>
  Date:   Sun Mar 13 15:56:26 2011 -0400

    readlinkat(), fchownat() and fstatat() with empty relative pathnames

This is both unexpected for userspace and makes readlink/readlinkat
inconsistant with all other interfaces; and inconsistant with our stated
return for these pathnames.

As the readlinkat call does not have a flags parameter we cannot use
the AT_EMPTY_PATH approach used in the other calls.  Therefore expose
whether the original path is infact entry via a new user_path_at_empty()
path lookup function.  Use this to determine whether to default to EINVAL
or ENOENT for failures.

BugLink: http://bugs.launchpad.net/bugs/817187
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 fs/namei.c            |   24 +++++++++++++++++++-----
 fs/stat.c             |    5 +++--
 include/linux/namei.h |    1 +
 3 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 445fd5d..9e013b8 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -137,7 +137,8 @@ static int do_getname(const char __user *filename, char *page)
 	return retval;
 }
 
-static char *getname_flags(const char __user * filename, int flags)
+static char *getname_flags_empty(const char __user * filename,
+							int flags, int *empty)
 {
 	char *tmp, *result;
 
@@ -148,6 +149,8 @@ static char *getname_flags(const char __user * filename, int flags)
 
 		result = tmp;
 		if (retval < 0) {
+			if (retval == -ENOENT && empty)
+				*empty = 1;
 			if (retval != -ENOENT || !(flags & LOOKUP_EMPTY)) {
 				__putname(tmp);
 				result = ERR_PTR(retval);
@@ -158,9 +161,14 @@ static char *getname_flags(const char __user * filename, int flags)
 	return result;
 }
 
+static char *getname_flags(const char __user * filename, int flags)
+{
+	return getname_flags_empty(filename, flags, 0);
+}
+
 char *getname(const char __user * filename)
 {
-	return getname_flags(filename, 0);
+	return getname_flags_empty(filename, 0, 0);
 }
 
 #ifdef CONFIG_AUDITSYSCALL
@@ -1756,11 +1764,11 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
 	return __lookup_hash(&this, base, NULL);
 }
 
-int user_path_at(int dfd, const char __user *name, unsigned flags,
-		 struct path *path)
+int user_path_at_empty(int dfd, const char __user *name, unsigned flags,
+		 struct path *path, int *empty)
 {
 	struct nameidata nd;
-	char *tmp = getname_flags(name, flags);
+	char *tmp = getname_flags_empty(name, flags, empty);
 	int err = PTR_ERR(tmp);
 	if (!IS_ERR(tmp)) {
 
@@ -1774,6 +1782,12 @@ int user_path_at(int dfd, const char __user *name, unsigned flags,
 	return err;
 }
 
+int user_path_at(int dfd, const char __user *name, unsigned flags,
+		 struct path *path)
+{
+	return user_path_at_empty(dfd, name, flags, path, 0);
+}
+
 static int user_path_parent(int dfd, const char __user *path,
 			struct nameidata *nd, char **name)
 {
diff --git a/fs/stat.c b/fs/stat.c
index 9610391..4c814bb 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -296,15 +296,16 @@ SYSCALL_DEFINE4(readlinkat, int, dfd, const char __user *, pathname,
 {
 	struct path path;
 	int error;
+	int empty = 0;
 
 	if (bufsiz <= 0)
 		return -EINVAL;
 
-	error = user_path_at(dfd, pathname, LOOKUP_EMPTY, &path);
+	error = user_path_at_empty(dfd, pathname, LOOKUP_EMPTY, &path, &empty);
 	if (!error) {
 		struct inode *inode = path.dentry->d_inode;
 
-		error = -EINVAL;
+		error = (empty) ? -ENOENT : -EINVAL;
 		if (inode->i_op->readlink) {
 			error = security_inode_readlink(path.dentry);
 			if (!error) {
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 76fe2c6..c300127 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -66,6 +66,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
 #define LOOKUP_EMPTY		0x4000
 
 extern int user_path_at(int, const char __user *, unsigned, struct path *);
+extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);
 
 #define user_path(name, path) user_path_at(AT_FDCWD, name, LOOKUP_FOLLOW, path)
 #define user_lpath(name, path) user_path_at(AT_FDCWD, name, 0, path)
-- 
1.7.4.1

Re: [PATCH 1/1] readlinkat: ensure we return ENOENT for the empty pathname for normal lookups

[Wanlong Gao]

Hi Greg:

I see that this patch was queued to stable 3.0 and 3.1, and I wanna know
will it be merged to v3.0.9, v3.1.1?

The return value of readlink/readlinkat for the empty pathname had switched
from ENOENT to EINVAL since V2.6.39, and now switched back again....

It is important for LTP to check the return value for this, so can you tell
me, thanks a lot Greg.

Thanks
-Wanlong Gao

> Since the commit below which added O_PATH support to the *at() calls,
> the error return for readlink/readlinkat for the empty pathname has
> switched from ENOENT to EINVAL:
> 
>   commit 65cfc6722361570bfe255698d9cd4dccaf47570d
>   Author: Al Viro <viro@zeniv.linux.org.uk>
>   Date:   Sun Mar 13 15:56:26 2011 -0400
> 
>     readlinkat(), fchownat() and fstatat() with empty relative pathnames
> 
> This is both unexpected for userspace and makes readlink/readlinkat
> inconsistant with all other interfaces; and inconsistant with our stated
> return for these pathnames.
> 
> As the readlinkat call does not have a flags parameter we cannot use
> the AT_EMPTY_PATH approach used in the other calls.  Therefore expose
> whether the original path is infact entry via a new user_path_at_empty()
> path lookup function.  Use this to determine whether to default to EINVAL
> or ENOENT for failures.
> 
> BugLink: http://bugs.launchpad.net/bugs/817187
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  fs/namei.c            |   24 +++++++++++++++++++-----
>  fs/stat.c             |    5 +++--
>  include/linux/namei.h |    1 +
>  3 files changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 445fd5d..9e013b8 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -137,7 +137,8 @@ static int do_getname(const char __user *filename, char *page)
>  	return retval;
>  }
>  
> -static char *getname_flags(const char __user * filename, int flags)
> +static char *getname_flags_empty(const char __user * filename,
> +							int flags, int *empty)
>  {
>  	char *tmp, *result;
>  
> @@ -148,6 +149,8 @@ static char *getname_flags(const char __user * filename, int flags)
>  
>  		result = tmp;
>  		if (retval < 0) {
> +			if (retval == -ENOENT && empty)
> +				*empty = 1;
>  			if (retval != -ENOENT || !(flags & LOOKUP_EMPTY)) {
>  				__putname(tmp);
>  				result = ERR_PTR(retval);
> @@ -158,9 +161,14 @@ static char *getname_flags(const char __user * filename, int flags)
>  	return result;
>  }
>  
> +static char *getname_flags(const char __user * filename, int flags)
> +{
> +	return getname_flags_empty(filename, flags, 0);
> +}
> +
>  char *getname(const char __user * filename)
>  {
> -	return getname_flags(filename, 0);
> +	return getname_flags_empty(filename, 0, 0);
>  }
>  
>  #ifdef CONFIG_AUDITSYSCALL
> @@ -1756,11 +1764,11 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
>  	return __lookup_hash(&this, base, NULL);
>  }
>  
> -int user_path_at(int dfd, const char __user *name, unsigned flags,
> -		 struct path *path)
> +int user_path_at_empty(int dfd, const char __user *name, unsigned flags,
> +		 struct path *path, int *empty)
>  {
>  	struct nameidata nd;
> -	char *tmp = getname_flags(name, flags);
> +	char *tmp = getname_flags_empty(name, flags, empty);
>  	int err = PTR_ERR(tmp);
>  	if (!IS_ERR(tmp)) {
>  
> @@ -1774,6 +1782,12 @@ int user_path_at(int dfd, const char __user *name, unsigned flags,
>  	return err;
>  }
>  
> +int user_path_at(int dfd, const char __user *name, unsigned flags,
> +		 struct path *path)
> +{
> +	return user_path_at_empty(dfd, name, flags, path, 0);
> +}
> +
>  static int user_path_parent(int dfd, const char __user *path,
>  			struct nameidata *nd, char **name)
>  {
> diff --git a/fs/stat.c b/fs/stat.c
> index 9610391..4c814bb 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -296,15 +296,16 @@ SYSCALL_DEFINE4(readlinkat, int, dfd, const char __user *, pathname,
>  {
>  	struct path path;
>  	int error;
> +	int empty = 0;
>  
>  	if (bufsiz <= 0)
>  		return -EINVAL;
>  
> -	error = user_path_at(dfd, pathname, LOOKUP_EMPTY, &path);
> +	error = user_path_at_empty(dfd, pathname, LOOKUP_EMPTY, &path, &empty);
>  	if (!error) {
>  		struct inode *inode = path.dentry->d_inode;
>  
> -		error = -EINVAL;
> +		error = (empty) ? -ENOENT : -EINVAL;
>  		if (inode->i_op->readlink) {
>  			error = security_inode_readlink(path.dentry);
>  			if (!error) {
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 76fe2c6..c300127 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -66,6 +66,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
>  #define LOOKUP_EMPTY		0x4000
>  
>  extern int user_path_at(int, const char __user *, unsigned, struct path *);
> +extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);
>  
>  #define user_path(name, path) user_path_at(AT_FDCWD, name, LOOKUP_FOLLOW, path)
>  #define user_lpath(name, path) user_path_at(AT_FDCWD, name, 0, path)

Re: [PATCH 1/1] readlinkat: ensure we return ENOENT for the empty pathname for normal lookups

[Greg KH]

On Mon, Nov 07, 2011 at 11:03:50AM +0800, Wanlong Gao wrote:
> Hi Greg:
> 
> I see that this patch was queued to stable 3.0 and 3.1, and I wanna know
> will it be merged to v3.0.9, v3.1.1?

That is exactly what being queued for the next stable release means, so
yes, it will be included in those next releases, when they happen,
hopefully sometime later this week.

greg k-h

Re: [PATCH 0/1] readlinkat() error code change for empty pathname

[Randy Dunlap]

On Thu,  4 Aug 2011 12:00:33 +0100 Andy Whitcroft wrote:

> In the 3.0 kernel we seem to have a semantic change in the error

Meta-comment:  Please drop the use of patch 0/1 for only one patch.
Just use the canonical patch format as described in Documentation/SubmittingPatches:


<quote>

15) The canonical patch format

The canonical patch subject line is:

    Subject: [PATCH 001/123] subsystem: summary phrase

The canonical patch message body contains the following:

  - A "from" line specifying the patch author.

  - An empty line.

  - The body of the explanation, which will be copied to the
    permanent changelog to describe this patch.

  - The "Signed-off-by:" lines, described above, which will
    also go in the changelog.

  - A marker line containing simply "---".

  - Any additional comments not suitable for the changelog.

  - The actual patch (diff output).

[...]

The "---" marker line serves the essential purpose of marking for patch
handling tools where the changelog message ends.

One good use for the additional comments after the "---" marker is for
a diffstat, to show what files have changed, and the number of
inserted and deleted lines per file.  A diffstat is especially useful
on bigger patches.  Other comments relevant only to the moment or the
maintainer, not suitable for the permanent changelog, should also go
here.  A good example of such comments might be "patch changelogs"
which describe what has changed between the v1 and v2 version of the
patch.

If you are going to include a diffstat after the "---" marker, please
use diffstat options "-p 1 -w 70" so that filenames are listed from
the top of the kernel source tree and don't use too much horizontal
space (easily fit in 80 columns, maybe with some indentation).

</quote>


Thanks.
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***