[PATCH] Linux: Work around kernel bugs in chmod on /proc/self/fd paths

[PATCH] Linux: Work around kernel bugs in chmod on /proc/self/fd paths

[Florian Weimer]

It appears that the ability to change symbolic link modes through such
paths is unintended.  On several file systems, the operation fails with
EOPNOTSUPP, even though the symbolic link permissions are updated.
The expected behavior is a failure to update the permissions, without
file system changes.

-----
 io/tst-lchmod.c                    | 83 ++++++++++++--------------------------
 sysdeps/unix/sysv/linux/fchmodat.c | 28 +++++++++++--
 2 files changed, 49 insertions(+), 62 deletions(-)

diff --git a/io/tst-lchmod.c b/io/tst-lchmod.c
index 73e45549af..59873f130d 100644
--- a/io/tst-lchmod.c
+++ b/io/tst-lchmod.c
@@ -102,68 +102,44 @@ test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t,
   TEST_VERIFY ((st.st_mode & 0777) != 2);
   mode_t original_symlink_mode = st.st_mode;
 
-  /* Set to true if AT_SYMLINK_NOFOLLOW is supported.  */
-  bool nofollow;
-
   /* We should be able to change the mode of a file, including through
      the symbolic link to-file.  */
   const char *arg = select_path (do_relative_path, path_file, "file");
   TEST_COMPARE (chmod_func (fd, arg, 1, 0), 0);
   xstat (path_file, &st);
   TEST_COMPARE (st.st_mode & 0777, 1);
-  int ret = chmod_func (fd, path_file, 2, AT_SYMLINK_NOFOLLOW);
-  if (ret == 0)
-    {
-      printf ("info: AT_SYMLINK_NOFOLLOW support in %s\n", tempdir);
-      nofollow = true;
-    }
-  else
-    {
-      printf ("info: no AT_SYMLINK_NOFOLLOW support in %s\n", tempdir);
-      nofollow = false;
-
-      /* Set up things for the code below.  */
-      TEST_COMPARE (chmod_func (fd, path_file, 2, 0), 0);
-    }
+  arg = select_path (do_relative_path, path_to_file, "to-file");
+  TEST_COMPARE (chmod_func (fd, path_to_file, 2, 0), 0);
   xstat (path_file, &st);
   TEST_COMPARE (st.st_mode & 0777, 2);
+  xlstat (path_to_file, &st);
+  TEST_COMPARE (original_symlink_mode, st.st_mode);
+  arg = select_path (do_relative_path, path_file, "file");
+  TEST_COMPARE (chmod_func (fd, arg, 1, 0), 0);
+  xstat (path_file, &st);
+  TEST_COMPARE (st.st_mode & 0777, 1);
+  xlstat (path_to_file, &st);
+  TEST_COMPARE (original_symlink_mode, st.st_mode);
+
+  /* Changing the mode of a symbolic link should fail fail.  */
   arg = select_path (do_relative_path, path_to_file, "to-file");
-  TEST_COMPARE (chmod_func (fd, path_to_file, 1, 0), 0);
+  int ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW);
+  TEST_COMPARE (ret, -1);
+  TEST_COMPARE (errno, EOPNOTSUPP);
+
+  /* The modes should remain unchanged.  */
   xstat (path_file, &st);
   TEST_COMPARE (st.st_mode & 0777, 1);
   xlstat (path_to_file, &st);
   TEST_COMPARE (original_symlink_mode, st.st_mode);
 
-  /* Changing the mode of a symbolic link may fail.  */
   arg = select_path (do_relative_path, path_to_file, "to-file");
   ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW);
-  if (nofollow)
-    {
-      TEST_COMPARE (ret, 0);
+  TEST_COMPARE (ret, -1);
+  TEST_COMPARE (errno, EOPNOTSUPP);
 
-      /* The mode of the link changed.  */
-      xlstat (path_to_file, &st);
-      TEST_COMPARE (st.st_mode & 0777, 2);
-
-      /* But the mode of the file is unchanged.  */
-      xstat (path_file, &st);
-      TEST_COMPARE (st.st_mode & 0777, 1);
-
-    }
-  else
-    {
-      TEST_COMPARE (ret, -1);
-      TEST_COMPARE (errno, EOPNOTSUPP);
-
-      /* The modes should remain unchanged.  */
-      xstat (path_file, &st);
-      TEST_COMPARE (st.st_mode & 0777, 1);
-      xlstat (path_to_file, &st);
-      TEST_COMPARE (original_symlink_mode, st.st_mode);
-    }
-
-  /* If we have NOFOLLOW support, we should be able to change the mode
-     of a dangling symbolic link or a symbolic link loop.  */
+  /* Likewise, changing dangling and looping symbolic links must
+     fail.  */
   const char *paths[] = { path_dangling, path_loop };
   for (size_t i = 0; i < array_length (paths); ++i)
     {
@@ -178,19 +154,10 @@ test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t,
       original_symlink_mode = st.st_mode;
       arg = select_path (do_relative_path, path, filename);
       ret = chmod_func (fd, arg, new_mode, AT_SYMLINK_NOFOLLOW);
-      if (nofollow)
-        {
-          TEST_COMPARE (ret, 0);
-          xlstat (path, &st);
-          TEST_COMPARE (st.st_mode & 0777, new_mode);
-        }
-      else /* !nofollow.  */
-        {
-          TEST_COMPARE (ret, -1);
-          TEST_COMPARE (errno, EOPNOTSUPP);
-          xlstat (path, &st);
-          TEST_COMPARE (st.st_mode, original_symlink_mode);
-        }
+      TEST_COMPARE (ret, -1);
+      TEST_COMPARE (errno, EOPNOTSUPP);
+      xlstat (path, &st);
+      TEST_COMPARE (st.st_mode, original_symlink_mode);
     }
 
    /* A missing file should always result in ENOENT.  The presence of
diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c
index 719053b333..17eca54051 100644
--- a/sysdeps/unix/sysv/linux/fchmodat.c
+++ b/sysdeps/unix/sysv/linux/fchmodat.c
@@ -45,6 +45,30 @@ fchmodat (int fd, const char *file, mode_t mode, int flag)
 	   caller can treat them as temporary if necessary.  */
 	return pathfd;
 
+      /* Use fstatat because fstat does not work on O_PATH descriptors
+	 before Linux 3.6.  */
+      struct stat64 st;
+      if (fstatat64 (pathfd, "", &st, AT_EMPTY_PATH) != 0)
+	{
+	  __close_nocancel (pathfd);
+	  return -1;
+	}
+
+      /* Some Linux versions with some file systems can actually
+	 change symbolic link permissions via /proc, but this is not
+	 intentional, and it gives inconsistent results (e.g., error
+	 return despite mode change).  The expected behavior is that
+	 symbolic link modes cannot be changed at all, and this check
+	 enforces that.  */
+      if (S_ISLNK (st.st_mode))
+	{
+	  __close_nocancel (pathfd);
+	  __set_errno (EOPNOTSUPP);
+	  return -1;
+	}
+
+      /* For most file systems, fchmod does not operate on O_PATH
+	 descriptors, so go through /proc.  */
       char buf[32];
       if (__snprintf (buf, sizeof (buf), "/proc/self/fd/%d", pathfd) < 0)
 	{
@@ -54,10 +78,6 @@ fchmodat (int fd, const char *file, mode_t mode, int flag)
 	  return -1;
 	}
 
-      /* This operates directly on the symbolic link if it is one.
-	 /proc/self/fd files look like symbolic links, but they are
-	 not.  (fchmod and fchmodat do not work on O_PATH descriptors,
-	 similar to fstat before Linux 3.6.)  */
       int ret = __chmod (buf, mode);
       if (ret != 0)
 	{

Re: [PATCH] Linux: Work around kernel bugs in chmod on /proc/self/fd paths

[Florian Weimer]

* Florian Weimer:

> It appears that the ability to change symbolic link modes through such
> paths is unintended.  On several file systems, the operation fails with
> EOPNOTSUPP, even though the symbolic link permissions are updated.
> The expected behavior is a failure to update the permissions, without
> file system changes.

Adhemerval, you wanted to see this fixed quickly?

Thanks,
Florian

Re: [PATCH] Linux: Work around kernel bugs in chmod on /proc/self/fd paths

[Adhemerval Zanella]

On 13/02/2020 13:48, Florian Weimer wrote:
> * Florian Weimer:
> 
>> It appears that the ability to change symbolic link modes through such
>> paths is unintended.  On several file systems, the operation fails with
>> EOPNOTSUPP, even though the symbolic link permissions are updated.
>> The expected behavior is a failure to update the permissions, without
>> file system changes.
> 
> Adhemerval, you wanted to see this fixed quickly?

I will try to fully review this today, but skimming it seems fine.

Re: [PATCH] Linux: Work around kernel bugs in chmod on /proc/self/fd paths

[Matheus Castanho]

On 2/13/20 1:54 PM, Adhemerval Zanella wrote:
> 
> 
> On 13/02/2020 13:48, Florian Weimer wrote:
>> * Florian Weimer:
>>
>>> It appears that the ability to change symbolic link modes through such
>>> paths is unintended.  On several file systems, the operation fails with
>>> EOPNOTSUPP, even though the symbolic link permissions are updated.
>>> The expected behavior is a failure to update the permissions, without
>>> file system changes.
>>
>> Adhemerval, you wanted to see this fixed quickly?
> 
> I will try to fully review this today, but skimming it seems fine.
> 

I am seeing the same issue as Adhemerval on powerpc, powerpc64 and
powerpc64le.

I still haven't reviewed the whole patch, but I can at least verify that
it fixes the failing test on ext4 on powerpc (5.4.0-3) and powerpc64le
(4.15.0).

PASS: io/tst-lchmod
original exit status 0
info: testing lchmod
info: testing fchmodat with AT_FDCWD
info: testing fchmodat with relative path
info: re-running tests (after trying to empty /proc)
info: testing lchmod
info: testing fchmodat with AT_FDCWD
info: testing fchmodat with relative path

--
Matheus Castanho

Re: [PATCH] Linux: Work around kernel bugs in chmod on /proc/self/fd paths

[Paul Eggert]

The patch to sysdeps/unix/sysv/linux/fchmodat.c looks good; thanks.

I followed up in Gnulib by installing a similar patch there. See:

https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=d17d3a3cc55500709c22c7d0ade24a0845aa17b9

Re: [PATCH] Linux: Work around kernel bugs in chmod on /proc/self/fd paths

[Florian Weimer]

* Florian Weimer:

> It appears that the ability to change symbolic link modes through such
> paths is unintended.  On several file systems, the operation fails with
> EOPNOTSUPP, even though the symbolic link permissions are updated.
> The expected behavior is a failure to update the permissions, without
> file system changes.

Does somebody want to review the test part of this patch?

  <https://www.sourceware.org/ml/libc-alpha/2020-02/msg00534.html>

Thanks,
Florian

Re: [PATCH] Linux: Work around kernel bugs in chmod on /proc/self/fd paths

[Matheus Castanho]

Hi Florian,

Some considerations on the test code below.

On 2/12/20 5:24 PM, Florian Weimer wrote:
> It appears that the ability to change symbolic link modes through such
> paths is unintended.  On several file systems, the operation fails with
> EOPNOTSUPP, even though the symbolic link permissions are updated.
> The expected behavior is a failure to update the permissions, without
> file system changes.
> 
> -----
>  io/tst-lchmod.c                    | 83 ++++++++++++--------------------------
>  sysdeps/unix/sysv/linux/fchmodat.c | 28 +++++++++++--
>  2 files changed, 49 insertions(+), 62 deletions(-)
> 
> diff --git a/io/tst-lchmod.c b/io/tst-lchmod.c
> index 73e45549af..59873f130d 100644
> --- a/io/tst-lchmod.c
> +++ b/io/tst-lchmod.c
> @@ -102,68 +102,44 @@ test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t,
>    TEST_VERIFY ((st.st_mode & 0777) != 2);
>    mode_t original_symlink_mode = st.st_mode;
>  
> -  /* Set to true if AT_SYMLINK_NOFOLLOW is supported.  */
> -  bool nofollow;
> -
>    /* We should be able to change the mode of a file, including through
>       the symbolic link to-file.  */
>    const char *arg = select_path (do_relative_path, path_file, "file");
>    TEST_COMPARE (chmod_func (fd, arg, 1, 0), 0);
>    xstat (path_file, &st);
>    TEST_COMPARE (st.st_mode & 0777, 1);
> -  int ret = chmod_func (fd, path_file, 2, AT_SYMLINK_NOFOLLOW);
> -  if (ret == 0)
> -    {
> -      printf ("info: AT_SYMLINK_NOFOLLOW support in %s\n", tempdir);
> -      nofollow = true;
> -    }
> -  else
> -    {
> -      printf ("info: no AT_SYMLINK_NOFOLLOW support in %s\n", tempdir);
> -      nofollow = false;
> -
> -      /* Set up things for the code below.  */
> -      TEST_COMPARE (chmod_func (fd, path_file, 2, 0), 0);
> -    }
> +  arg = select_path (do_relative_path, path_to_file, "to-file");
> +  TEST_COMPARE (chmod_func (fd, path_to_file, 2, 0), 0);

Shouldn't you use arg instead of path_to_file here? Like
 TEST_COMPARE (chmod_func (fd, arg, 2, 0), 0);

Otherwise the select_path call is not needed.

>    xstat (path_file, &st);
>    TEST_COMPARE (st.st_mode & 0777, 2);
> +  xlstat (path_to_file, &st);
> +  TEST_COMPARE (original_symlink_mode, st.st_mode);

Ok.

> +  arg = select_path (do_relative_path, path_file, "file");
> +  TEST_COMPARE (chmod_func (fd, arg, 1, 0), 0);
> +  xstat (path_file, &st);
> +  TEST_COMPARE (st.st_mode & 0777, 1);
> +  xlstat (path_to_file, &st);
> +  TEST_COMPARE (original_symlink_mode, st.st_mode);

Ok.

> +
> +  /* Changing the mode of a symbolic link should fail fail.  */

Duplicated 'fail'.

>    arg = select_path (do_relative_path, path_to_file, "to-file");
> -  TEST_COMPARE (chmod_func (fd, path_to_file, 1, 0), 0);
> +  int ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW);
> +  TEST_COMPARE (ret, -1);
> +  TEST_COMPARE (errno, EOPNOTSUPP);

Ok.

> +
> +  /* The modes should remain unchanged.  */
>    xstat (path_file, &st);
>    TEST_COMPARE (st.st_mode & 0777, 1);
>    xlstat (path_to_file, &st);
>    TEST_COMPARE (original_symlink_mode, st.st_mod
Ok.

>  
> -  /* Changing the mode of a symbolic link may fail.  */
>    arg = select_path (do_relative_path, path_to_file, "to-file");
>    ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW);
> -  if (nofollow)
> -    {
> -      TEST_COMPARE (ret, 0);
> +  TEST_COMPARE (ret, -1);
> +  TEST_COMPARE (errno, EOPNOTSUPP);

This is exactly the same test as above. Is it really needed?

>  
> -      /* The mode of the link changed.  */
> -      xlstat (path_to_file, &st);
> -      TEST_COMPARE (st.st_mode & 0777, 2);
> -
> -      /* But the mode of the file is unchanged.  */
> -      xstat (path_file, &st);
> -      TEST_COMPARE (st.st_mode & 0777, 1);
> -
> -    }
> -  else
> -    {
> -      TEST_COMPARE (ret, -1);
> -      TEST_COMPARE (errno, EOPNOTSUPP);
> -
> -      /* The modes should remain unchanged.  */
> -      xstat (path_file, &st);
> -      TEST_COMPARE (st.st_mode & 0777, 1);
> -      xlstat (path_to_file, &st);
> -      TEST_COMPARE (original_symlink_mode, st.st_mode);
> -    }
> -
> -  /* If we have NOFOLLOW support, we should be able to change the mode
> -     of a dangling symbolic linqk or a symbolic link loop.  */
> +  /* Likewise, changing dangling and looping symbolic links must
> +     fail.  */
>    const char *paths[] = { path_dangling, path_loop };
>    for (size_t i = 0; i < array_length (paths); ++i)
>      {
> @@ -178,19 +154,10 @@ test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t,
>        original_symlink_mode = st.st_mode;
>        arg = select_path (do_relative_path, path, filename);
>        ret = chmod_func (fd, arg, new_mode, AT_SYMLINK_NOFOLLOW);
> -      if (nofollow)
> -        {
> -          TEST_COMPARE (ret, 0);
> -          xlstat (path, &st);
> -          TEST_COMPARE (st.st_mode & 0777, new_mode);
> -        }
> -      else /* !nofollow.  */
> -        {
> -          TEST_COMPARE (ret, -1);
> -          TEST_COMPARE (errno, EOPNOTSUPP);
> -          xlstat (path, &st);
> -          TEST_COMPARE (st.st_mode, original_symlink_mode);
> -        }
> +      TEST_COMPARE (ret, -1);
> +      TEST_COMPARE (errno, EOPNOTSUPP);
> +      xlstat (path, &st);
> +      TEST_COMPARE (st.st_mode, original_symlink_mode);

Ok.

>      }
>  
>     /* A missing file should always result in ENOENT.  The presence of
> diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c
> index 719053b333..17eca54051 100644
> --- a/sysdeps/unix/sysv/linux/fchmodat.c
> +++ b/sysdeps/unix/sysv/linux/fchmodat.c
> @@ -45,6 +45,30 @@ fchmodat (int fd, const char *file, mode_t mode, int flag)
>  	   caller can treat them as temporary if necessary.  */
>  	return pathfd;
>  
> +      /* Use fstatat because fstat does not work on O_PATH descriptors
> +	 before Linux 3.6.  */
> +      struct stat64 st;
> +      if (fstatat64 (pathfd, "", &st, AT_EMPTY_PATH) != 0)
> +	{
> +	  __close_nocancel (pathfd);
> +	  return -1;
> +	}
> +
> +      /* Some Linux versions with some file systems can actually
> +	 change symbolic link permissions via /proc, but this is not
> +	 intentional, and it gives inconsistent results (e.g., error
> +	 return despite mode change).  The expected behavior is that
> +	 symbolic link modes cannot be changed at all, and this check
> +	 enforces that.  */
> +      if (S_ISLNK (st.st_mode))
> +	{
> +	  __close_nocancel (pathfd);
> +	  __set_errno (EOPNOTSUPP);
> +	  return -1;
> +	}
> +
> +      /* For most file systems, fchmod does not operate on O_PATH
> +	 descriptors, so go through /proc.  */
>        char buf[32];
>        if (__snprintf (buf, sizeof (buf), "/proc/self/fd/%d", pathfd) < 0)
>  	{
> @@ -54,10 +78,6 @@ fchmodat (int fd, const char *file, mode_t mode, int flag)
>  	  return -1;
>  	}
>  
> -      /* This operates directly on the symbolic link if it is one.
> -	 /proc/self/fd files look like symbolic links, but they are
> -	 not.  (fchmod and fchmodat do not work on O_PATH descriptors,
> -	 similar to fstat before Linux 3.6.)  */
>        int ret = __chmod (buf, mode);
>        if (ret != 0)
>  	{
> 

--
Matheus Castanho

Re: [PATCH] Linux: Work around kernel bugs in chmod on /proc/self/fd paths

[Florian Weimer]

* Matheus Castanho:

> Some considerations on the test code below.

Thanks.

>> +  arg = select_path (do_relative_path, path_to_file, "to-file");
>> +  TEST_COMPARE (chmod_func (fd, path_to_file, 2, 0), 0);
>
> Shouldn't you use arg instead of path_to_file here? Like
>  TEST_COMPARE (chmod_func (fd, arg, 2, 0), 0);
>
> Otherwise the select_path call is not needed.

Fixed to use arg.

>> +
>> +  /* Changing the mode of a symbolic link should fail fail.  */
>
> Duplicated 'fail'.

Fixed.

>> -  /* Changing the mode of a symbolic link may fail.  */
>>    arg = select_path (do_relative_path, path_to_file, "to-file");
>>    ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW);
>> -  if (nofollow)
>> -    {
>> -      TEST_COMPARE (ret, 0);
>> +  TEST_COMPARE (ret, -1);
>> +  TEST_COMPARE (errno, EOPNOTSUPP);
>
> This is exactly the same test as above. Is it really needed?

Agreed, I've dropped it.

New patch below.

Florian

8<------------------------------------------------------------------8<
It appears that the ability to change symbolic link modes through such
paths is unintended.  On several file systems, the operation fails with
EOPNOTSUPP, even though the symbolic link permissions are updated.
The expected behavior is a failure to update the permissions, without
file system changes.

-----
 io/tst-lchmod.c                    | 80 ++++++++++----------------------------
 sysdeps/unix/sysv/linux/fchmodat.c | 28 +++++++++++--
 2 files changed, 45 insertions(+), 63 deletions(-)

diff --git a/io/tst-lchmod.c b/io/tst-lchmod.c
index 73e45549af..33ca474b50 100644
--- a/io/tst-lchmod.c
+++ b/io/tst-lchmod.c
@@ -102,68 +102,39 @@ test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t,
   TEST_VERIFY ((st.st_mode & 0777) != 2);
   mode_t original_symlink_mode = st.st_mode;
 
-  /* Set to true if AT_SYMLINK_NOFOLLOW is supported.  */
-  bool nofollow;
-
   /* We should be able to change the mode of a file, including through
      the symbolic link to-file.  */
   const char *arg = select_path (do_relative_path, path_file, "file");
   TEST_COMPARE (chmod_func (fd, arg, 1, 0), 0);
   xstat (path_file, &st);
   TEST_COMPARE (st.st_mode & 0777, 1);
-  int ret = chmod_func (fd, path_file, 2, AT_SYMLINK_NOFOLLOW);
-  if (ret == 0)
-    {
-      printf ("info: AT_SYMLINK_NOFOLLOW support in %s\n", tempdir);
-      nofollow = true;
-    }
-  else
-    {
-      printf ("info: no AT_SYMLINK_NOFOLLOW support in %s\n", tempdir);
-      nofollow = false;
-
-      /* Set up things for the code below.  */
-      TEST_COMPARE (chmod_func (fd, path_file, 2, 0), 0);
-    }
+  arg = select_path (do_relative_path, path_to_file, "to-file");
+  TEST_COMPARE (chmod_func (fd, arg, 2, 0), 0);
   xstat (path_file, &st);
   TEST_COMPARE (st.st_mode & 0777, 2);
-  arg = select_path (do_relative_path, path_to_file, "to-file");
-  TEST_COMPARE (chmod_func (fd, path_to_file, 1, 0), 0);
+  xlstat (path_to_file, &st);
+  TEST_COMPARE (original_symlink_mode, st.st_mode);
+  arg = select_path (do_relative_path, path_file, "file");
+  TEST_COMPARE (chmod_func (fd, arg, 1, 0), 0);
   xstat (path_file, &st);
   TEST_COMPARE (st.st_mode & 0777, 1);
   xlstat (path_to_file, &st);
   TEST_COMPARE (original_symlink_mode, st.st_mode);
 
-  /* Changing the mode of a symbolic link may fail.  */
+  /* Changing the mode of a symbolic link should fail.  */
   arg = select_path (do_relative_path, path_to_file, "to-file");
-  ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW);
-  if (nofollow)
-    {
-      TEST_COMPARE (ret, 0);
-
-      /* The mode of the link changed.  */
-      xlstat (path_to_file, &st);
-      TEST_COMPARE (st.st_mode & 0777, 2);
-
-      /* But the mode of the file is unchanged.  */
-      xstat (path_file, &st);
-      TEST_COMPARE (st.st_mode & 0777, 1);
+  int ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW);
+  TEST_COMPARE (ret, -1);
+  TEST_COMPARE (errno, EOPNOTSUPP);
 
-    }
-  else
-    {
-      TEST_COMPARE (ret, -1);
-      TEST_COMPARE (errno, EOPNOTSUPP);
-
-      /* The modes should remain unchanged.  */
-      xstat (path_file, &st);
-      TEST_COMPARE (st.st_mode & 0777, 1);
-      xlstat (path_to_file, &st);
-      TEST_COMPARE (original_symlink_mode, st.st_mode);
-    }
+  /* The modes should remain unchanged.  */
+  xstat (path_file, &st);
+  TEST_COMPARE (st.st_mode & 0777, 1);
+  xlstat (path_to_file, &st);
+  TEST_COMPARE (original_symlink_mode, st.st_mode);
 
-  /* If we have NOFOLLOW support, we should be able to change the mode
-     of a dangling symbolic link or a symbolic link loop.  */
+  /* Likewise, changing dangling and looping symbolic links must
+     fail.  */
   const char *paths[] = { path_dangling, path_loop };
   for (size_t i = 0; i < array_length (paths); ++i)
     {
@@ -178,19 +149,10 @@ test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t,
       original_symlink_mode = st.st_mode;
       arg = select_path (do_relative_path, path, filename);
       ret = chmod_func (fd, arg, new_mode, AT_SYMLINK_NOFOLLOW);
-      if (nofollow)
-        {
-          TEST_COMPARE (ret, 0);
-          xlstat (path, &st);
-          TEST_COMPARE (st.st_mode & 0777, new_mode);
-        }
-      else /* !nofollow.  */
-        {
-          TEST_COMPARE (ret, -1);
-          TEST_COMPARE (errno, EOPNOTSUPP);
-          xlstat (path, &st);
-          TEST_COMPARE (st.st_mode, original_symlink_mode);
-        }
+      TEST_COMPARE (ret, -1);
+      TEST_COMPARE (errno, EOPNOTSUPP);
+      xlstat (path, &st);
+      TEST_COMPARE (st.st_mode, original_symlink_mode);
     }
 
    /* A missing file should always result in ENOENT.  The presence of
diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c
index 719053b333..17eca54051 100644
--- a/sysdeps/unix/sysv/linux/fchmodat.c
+++ b/sysdeps/unix/sysv/linux/fchmodat.c
@@ -45,6 +45,30 @@ fchmodat (int fd, const char *file, mode_t mode, int flag)
 	   caller can treat them as temporary if necessary.  */
 	return pathfd;
 
+      /* Use fstatat because fstat does not work on O_PATH descriptors
+	 before Linux 3.6.  */
+      struct stat64 st;
+      if (fstatat64 (pathfd, "", &st, AT_EMPTY_PATH) != 0)
+	{
+	  __close_nocancel (pathfd);
+	  return -1;
+	}
+
+      /* Some Linux versions with some file systems can actually
+	 change symbolic link permissions via /proc, but this is not
+	 intentional, and it gives inconsistent results (e.g., error
+	 return despite mode change).  The expected behavior is that
+	 symbolic link modes cannot be changed at all, and this check
+	 enforces that.  */
+      if (S_ISLNK (st.st_mode))
+	{
+	  __close_nocancel (pathfd);
+	  __set_errno (EOPNOTSUPP);
+	  return -1;
+	}
+
+      /* For most file systems, fchmod does not operate on O_PATH
+	 descriptors, so go through /proc.  */
       char buf[32];
       if (__snprintf (buf, sizeof (buf), "/proc/self/fd/%d", pathfd) < 0)
 	{
@@ -54,10 +78,6 @@ fchmodat (int fd, const char *file, mode_t mode, int flag)
 	  return -1;
 	}
 
-      /* This operates directly on the symbolic link if it is one.
-	 /proc/self/fd files look like symbolic links, but they are
-	 not.  (fchmod and fchmodat do not work on O_PATH descriptors,
-	 similar to fstat before Linux 3.6.)  */
       int ret = __chmod (buf, mode);
       if (ret != 0)
 	{

Re: [PATCH] Linux: Work around kernel bugs in chmod on /proc/self/fd paths

[Matheus Castanho]

On 2/18/20 11:50 AM, Florian Weimer wrote:
> * Matheus Castanho:
> 
>> Some considerations on the test code below.
> 
> Thanks.
> 
>>> +  arg = select_path (do_relative_path, path_to_file, "to-file");
>>> +  TEST_COMPARE (chmod_func (fd, path_to_file, 2, 0), 0);
>>
>> Shouldn't you use arg instead of path_to_file here? Like
>>  TEST_COMPARE (chmod_func (fd, arg, 2, 0), 0);
>>
>> Otherwise the select_path call is not needed.
> 
> Fixed to use arg.
> 
>>> +
>>> +  /* Changing the mode of a symbolic link should fail fail.  */
>>
>> Duplicated 'fail'.
> 
> Fixed.
> 
>>> -  /* Changing the mode of a symbolic link may fail.  */
>>>    arg = select_path (do_relative_path, path_to_file, "to-file");
>>>    ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW);
>>> -  if (nofollow)
>>> -    {
>>> -      TEST_COMPARE (ret, 0);
>>> +  TEST_COMPARE (ret, -1);
>>> +  TEST_COMPARE (errno, EOPNOTSUPP);
>>
>> This is exactly the same test as above. Is it really needed?
> 
> Agreed, I've dropped it.
> 
> New patch below.
> 
> Florian
> 
> 8<------------------------------------------------------------------8<
> It appears that the ability to change symbolic link modes through such
> paths is unintended.  On several file systems, the operation fails with
> EOPNOTSUPP, even though the symbolic link permissions are updated.
> The expected behavior is a failure to update the permissions, without
> file system changes.
> 
> -----
>  io/tst-lchmod.c                    | 80 ++++++++++----------------------------
>  sysdeps/unix/sysv/linux/fchmodat.c | 28 +++++++++++--
>  2 files changed, 45 insertions(+), 63 deletions(-)
> 
> diff --git a/io/tst-lchmod.c b/io/tst-lchmod.c
> index 73e45549af..33ca474b50 100644
> --- a/io/tst-lchmod.c
> +++ b/io/tst-lchmod.c
> @@ -102,68 +102,39 @@ test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t,
>    TEST_VERIFY ((st.st_mode & 0777) != 2);
>    mode_t original_symlink_mode = st.st_mode;
>  
> -  /* Set to true if AT_SYMLINK_NOFOLLOW is supported.  */
> -  bool nofollow;
> -
>    /* We should be able to change the mode of a file, including through
>       the symbolic link to-file.  */
>    const char *arg = select_path (do_relative_path, path_file, "file");
>    TEST_COMPARE (chmod_func (fd, arg, 1, 0), 0);
>    xstat (path_file, &st);
>    TEST_COMPARE (st.st_mode & 0777, 1);
> -  int ret = chmod_func (fd, path_file, 2, AT_SYMLINK_NOFOLLOW);
> -  if (ret == 0)
> -    {
> -      printf ("info: AT_SYMLINK_NOFOLLOW support in %s\n", tempdir);
> -      nofollow = true;
> -    }
> -  else
> -    {
> -      printf ("info: no AT_SYMLINK_NOFOLLOW support in %s\n", tempdir);
> -      nofollow = false;
> -
> -      /* Set up things for the code below.  */
> -      TEST_COMPARE (chmod_func (fd, path_file, 2, 0), 0);
> -    }
> +  arg = select_path (do_relative_path, path_to_file, "to-file");
> +  TEST_COMPARE (chmod_func (fd, arg, 2, 0), 0);
>    xstat (path_file, &st);
>    TEST_COMPARE (st.st_mode & 0777, 2);
> -  arg = select_path (do_relative_path, path_to_file, "to-file");
> -  TEST_COMPARE (chmod_func (fd, path_to_file, 1, 0), 0);
> +  xlstat (path_to_file, &st);
> +  TEST_COMPARE (original_symlink_mode, st.st_mode);
> +  arg = select_path (do_relative_path, path_file, "file");
> +  TEST_COMPARE (chmod_func (fd, arg, 1, 0), 0);
>    xstat (path_file, &st);
>    TEST_COMPARE (st.st_mode & 0777, 1);
>    xlstat (path_to_file, &st);
>    TEST_COMPARE (original_symlink_mode, st.st_mode);
>  
> -  /* Changing the mode of a symbolic link may fail.  */
> +  /* Changing the mode of a symbolic link should fail.  */
>    arg = select_path (do_relative_path, path_to_file, "to-file");
> -  ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW);
> -  if (nofollow)
> -    {
> -      TEST_COMPARE (ret, 0);
> -
> -      /* The mode of the link changed.  */
> -      xlstat (path_to_file, &st);
> -      TEST_COMPARE (st.st_mode & 0777, 2);
> -
> -      /* But the mode of the file is unchanged.  */
> -      xstat (path_file, &st);
> -      TEST_COMPARE (st.st_mode & 0777, 1);
> +  int ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW);
> +  TEST_COMPARE (ret, -1);
> +  TEST_COMPARE (errno, EOPNOTSUPP);
>  
> -    }
> -  else
> -    {
> -      TEST_COMPARE (ret, -1);
> -      TEST_COMPARE (errno, EOPNOTSUPP);
> -
> -      /* The modes should remain unchanged.  */
> -      xstat (path_file, &st);
> -      TEST_COMPARE (st.st_mode & 0777, 1);
> -      xlstat (path_to_file, &st);
> -      TEST_COMPARE (original_symlink_mode, st.st_mode);
> -    }
> +  /* The modes should remain unchanged.  */
> +  xstat (path_file, &st);
> +  TEST_COMPARE (st.st_mode & 0777, 1);
> +  xlstat (path_to_file, &st);
> +  TEST_COMPARE (original_symlink_mode, st.st_mode);
>  
> -  /* If we have NOFOLLOW support, we should be able to change the mode
> -     of a dangling symbolic link or a symbolic link loop.  */
> +  /* Likewise, changing dangling and looping symbolic links must
> +     fail.  */
>    const char *paths[] = { path_dangling, path_loop };
>    for (size_t i = 0; i < array_length (paths); ++i)
>      {
> @@ -178,19 +149,10 @@ test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t,
>        original_symlink_mode = st.st_mode;
>        arg = select_path (do_relative_path, path, filename);
>        ret = chmod_func (fd, arg, new_mode, AT_SYMLINK_NOFOLLOW);
> -      if (nofollow)
> -        {
> -          TEST_COMPARE (ret, 0);
> -          xlstat (path, &st);
> -          TEST_COMPARE (st.st_mode & 0777, new_mode);
> -        }
> -      else /* !nofollow.  */
> -        {
> -          TEST_COMPARE (ret, -1);
> -          TEST_COMPARE (errno, EOPNOTSUPP);
> -          xlstat (path, &st);
> -          TEST_COMPARE (st.st_mode, original_symlink_mode);
> -        }
> +      TEST_COMPARE (ret, -1);
> +      TEST_COMPARE (errno, EOPNOTSUPP);
> +      xlstat (path, &st);
> +      TEST_COMPARE (st.st_mode, original_symlink_mode);
>      }
>  
>     /* A missing file should always result in ENOENT.  The presence of
> diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c
> index 719053b333..17eca54051 100644
> --- a/sysdeps/unix/sysv/linux/fchmodat.c
> +++ b/sysdeps/unix/sysv/linux/fchmodat.c
> @@ -45,6 +45,30 @@ fchmodat (int fd, const char *file, mode_t mode, int flag)
>  	   caller can treat them as temporary if necessary.  */
>  	return pathfd;
>  
> +      /* Use fstatat because fstat does not work on O_PATH descriptors
> +	 before Linux 3.6.  */
> +      struct stat64 st;
> +      if (fstatat64 (pathfd, "", &st, AT_EMPTY_PATH) != 0)
> +	{
> +	  __close_nocancel (pathfd);
> +	  return -1;
> +	}
> +
> +      /* Some Linux versions with some file systems can actually
> +	 change symbolic link permissions via /proc, but this is not
> +	 intentional, and it gives inconsistent results (e.g., error
> +	 return despite mode change).  The expected behavior is that
> +	 symbolic link modes cannot be changed at all, and this check
> +	 enforces that.  */
> +      if (S_ISLNK (st.st_mode))
> +	{
> +	  __close_nocancel (pathfd);
> +	  __set_errno (EOPNOTSUPP);
> +	  return -1;
> +	}
> +
> +      /* For most file systems, fchmod does not operate on O_PATH
> +	 descriptors, so go through /proc.  */
>        char buf[32];
>        if (__snprintf (buf, sizeof (buf), "/proc/self/fd/%d", pathfd) < 0)
>  	{
> @@ -54,10 +78,6 @@ fchmodat (int fd, const char *file, mode_t mode, int flag)
>  	  return -1;
>  	}
>  
> -      /* This operates directly on the symbolic link if it is one.
> -	 /proc/self/fd files look like symbolic links, but they are
> -	 not.  (fchmod and fchmodat do not work on O_PATH descriptors,
> -	 similar to fstat before Linux 3.6.)  */
>        int ret = __chmod (buf, mode);
>        if (ret != 0)
>  	{
> 

LGTM.

Reviewed-by: Matheus Castanho <msc@linux.ibm.com>

--
Matheus Castanho

Re: [PATCH] Linux: Work around kernel bugs in chmod on /proc/self/fd paths

[Stefan Liebler]

On 2/18/20 5:50 PM, Matheus Castanho wrote:
...
> On 2/18/20 11:50 AM, Florian Weimer wrote:
>> Agreed, I've dropped it.
>>
>> New patch below.
>>
>> Florian
...
...
> LGTM.
> 
> Reviewed-by: Matheus Castanho <msc@linux.ibm.com>
> 
> --
> Matheus Castanho
> 

Hi Florian,

I've just recognized that building with -Os fails with
/usr/bin/ld: /path/to/glibc-build/libc_pic.os.clean: in function `fchmodat':
(.text+0xb1c36): undefined reference to `fstatat64'
collect2: error: ld returned 1 exit status

This happens at least on x86_64 and s390x.
I've bisected and this was the first bad commit.

Can you please have a look?

Bye
Stefan

Re: [PATCH] Linux: Work around kernel bugs in chmod on /proc/self/fd paths

[Florian Weimer]

* Stefan Liebler:

> On 2/18/20 5:50 PM, Matheus Castanho wrote:
> ...
>> On 2/18/20 11:50 AM, Florian Weimer wrote:
>>> Agreed, I've dropped it.
>>>
>>> New patch below.
>>>
>>> Florian
> ...
> ...
>> LGTM.
>>
>> Reviewed-by: Matheus Castanho <msc@linux.ibm.com>
>>
>> --
>> Matheus Castanho
>>
>
> Hi Florian,
>
> I've just recognized that building with -Os fails with
> /usr/bin/ld: /path/to/glibc-build/libc_pic.os.clean: in function `fchmodat':
> (.text+0xb1c36): undefined reference to `fstatat64'
> collect2: error: ld returned 1 exit status
>
> This happens at least on x86_64 and s390x.
> I've bisected and this was the first bad commit.

Thanks.  This patch should fix building with -Os.

Florian

8<------------------------------------------------------------------8<
Subject: Linux: Use __fstatat64 in fchmodat implementation

fstatat64 depends on inlining to produce the desired __fxstatat64
call, which does not happen with -Os, leading to a link failure
with an undefined reference to fstatat64.  __fxstatat64 has a macro
definition in include/sys/stat.h and thus avoids the problem.

-----
 sysdeps/unix/sysv/linux/fchmodat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c
index 17eca54051..5531f1aa6f 100644
--- a/sysdeps/unix/sysv/linux/fchmodat.c
+++ b/sysdeps/unix/sysv/linux/fchmodat.c
@@ -48,7 +48,7 @@ fchmodat (int fd, const char *file, mode_t mode, int flag)
       /* Use fstatat because fstat does not work on O_PATH descriptors
 	 before Linux 3.6.  */
       struct stat64 st;
-      if (fstatat64 (pathfd, "", &st, AT_EMPTY_PATH) != 0)
+      if (__fstatat64 (pathfd, "", &st, AT_EMPTY_PATH) != 0)
 	{
 	  __close_nocancel (pathfd);
 	  return -1;

Re: [PATCH] Linux: Work around kernel bugs in chmod on /proc/self/fd paths

[Stefan Liebler]

On 3/5/20 1:47 PM, Florian Weimer wrote:
> * Stefan Liebler:
> 
>> On 2/18/20 5:50 PM, Matheus Castanho wrote:
>> ...
>>> On 2/18/20 11:50 AM, Florian Weimer wrote:
>>>> Agreed, I've dropped it.
>>>>
>>>> New patch below.
>>>>
>>>> Florian
>> ...
>> ...
>>> LGTM.
>>>
>>> Reviewed-by: Matheus Castanho <msc@linux.ibm.com>
>>>
>>> --
>>> Matheus Castanho
>>>
>>
>> Hi Florian,
>>
>> I've just recognized that building with -Os fails with
>> /usr/bin/ld: /path/to/glibc-build/libc_pic.os.clean: in function `fchmodat':
>> (.text+0xb1c36): undefined reference to `fstatat64'
>> collect2: error: ld returned 1 exit status
>>
>> This happens at least on x86_64 and s390x.
>> I've bisected and this was the first bad commit.
> 
> Thanks.  This patch should fix building with -Os.
> 
> Florian
> 
> 8<------------------------------------------------------------------8<
> Subject: Linux: Use __fstatat64 in fchmodat implementation
> 
> fstatat64 depends on inlining to produce the desired __fxstatat64
> call, which does not happen with -Os, leading to a link failure
> with an undefined reference to fstatat64.  __fxstatat64 has a macro
> definition in include/sys/stat.h and thus avoids the problem.
> 
> -----
>   sysdeps/unix/sysv/linux/fchmodat.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c
> index 17eca54051..5531f1aa6f 100644
> --- a/sysdeps/unix/sysv/linux/fchmodat.c
> +++ b/sysdeps/unix/sysv/linux/fchmodat.c
> @@ -48,7 +48,7 @@ fchmodat (int fd, const char *file, mode_t mode, int flag)
>         /* Use fstatat because fstat does not work on O_PATH descriptors
>   	 before Linux 3.6.  */
>         struct stat64 st;
> -      if (fstatat64 (pathfd, "", &st, AT_EMPTY_PATH) != 0)
> +      if (__fstatat64 (pathfd, "", &st, AT_EMPTY_PATH) != 0)
>   	{
>   	  __close_nocancel (pathfd);
>   	  return -1;
> 

I've tested this and now glibc buids in my two environments (x86_64 and 
s390x) fine.

LGTM

Thanks,
Stefan