diff mbox series

[6/6] linux: Use fchmodat2 on fchmod for flags different than 0 (BZ 26401)

Message ID 20231030213245.2626895-7-adhemerval.zanella@linaro.org
State New
Headers show
Series Update from Linux 6.6 | expand

Commit Message

Adhemerval Zanella Netto Oct. 30, 2023, 9:32 p.m. UTC 
Linux 6.6 (09da082b07bbae1c) added support for fchmodat2, which is
has similar semantic of fchmodat with an extra flag argument.  This
allow fchmodat to implement AT_SYMLINK_NOFOLLOW and AT_EMPTY_PATH
without the need to procfs.

The syscall is registered on all architectures (with value of 452
except on alpha which is 562, commit 78252deb023cf087).

Checked on x86_64-linux-gnu on a 6.6 kernel.

PS: setting it as RFC because there is no Linux 6.6 release yet.
---
 io/tst-lchmod.c                           |   4 +-
 sysdeps/unix/sysv/linux/fchmodat.c        | 120 ++++++++++++----------
 sysdeps/unix/sysv/linux/kernel-features.h |   8 ++
 3 files changed, 77 insertions(+), 55 deletions(-)

Comments

Florian Weimer Oct. 31, 2023, 8:28 a.m. UTC | #1 
* Adhemerval Zanella:

> diff --git a/io/tst-lchmod.c b/io/tst-lchmod.c
> index 2bf4835b05..6496dc61e0 100644
> --- a/io/tst-lchmod.c
> +++ b/io/tst-lchmod.c
> @@ -219,9 +219,9 @@ test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t,
>           /* The error code from the openat fallback leaks out.  */
>           if (errno != ENFILE && errno != EMFILE)
>             TEST_COMPARE (errno, EOPNOTSUPP);
> +	 xstat (path_file, &st);
> +	 TEST_COMPARE (st.st_mode & 0777, 3);
>         }
> -     xstat (path_file, &st);
> -     TEST_COMPARE (st.st_mode & 0777, 3);
>  
>       /* Close the descriptors.  */
>       for (int *pfd = fd_list_begin (&fd_list); pfd < fd_list_end (&fd_list);

Why this test change?
Florian Weimer Oct. 31, 2023, 8:30 a.m. UTC | #2 
* Adhemerval Zanella:

> PS: setting it as RFC because there is no Linux 6.6 release yet.

Please remove this from the commit message.
Gabriel Ravier Oct. 31, 2023, 10:27 a.m. UTC | #3 
On 10/31/23 08:28, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> diff --git a/io/tst-lchmod.c b/io/tst-lchmod.c
>> index 2bf4835b05..6496dc61e0 100644
>> --- a/io/tst-lchmod.c
>> +++ b/io/tst-lchmod.c
>> @@ -219,9 +219,9 @@ test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t,
>>            /* The error code from the openat fallback leaks out.  */
>>            if (errno != ENFILE && errno != EMFILE)
>>              TEST_COMPARE (errno, EOPNOTSUPP);
>> +	 xstat (path_file, &st);
>> +	 TEST_COMPARE (st.st_mode & 0777, 3);
>>          }
>> -     xstat (path_file, &st);
>> -     TEST_COMPARE (st.st_mode & 0777, 3);
>>   
>>        /* Close the descriptors.  */
>>        for (int *pfd = fd_list_begin (&fd_list); pfd < fd_list_end (&fd_list);
> Why this test change?

Unless I'm misreading the original, it seems like the test would always 
fail if the call to `chmod_func` right before this occurs succeeds - any 
success is followed by two contradictory assertions that `(st.st_mode & 
0777) == 2` and `(st.st_mode & 0777) == 3` (and that last test seems 
like its testing that the value of `st.st_mode & 0777` does not change 
when `chmod_func` fails.
Adhemerval Zanella Netto Oct. 31, 2023, 2:14 p.m. UTC | #4 
On 31/10/23 07:27, Gabriel Ravier wrote:
> On 10/31/23 08:28, Florian Weimer wrote:
>> * Adhemerval Zanella:
>>
>>> diff --git a/io/tst-lchmod.c b/io/tst-lchmod.c
>>> index 2bf4835b05..6496dc61e0 100644
>>> --- a/io/tst-lchmod.c
>>> +++ b/io/tst-lchmod.c
>>> @@ -219,9 +219,9 @@ test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t,
>>>            /* The error code from the openat fallback leaks out.  */
>>>            if (errno != ENFILE && errno != EMFILE)
>>>              TEST_COMPARE (errno, EOPNOTSUPP);
>>> +     xstat (path_file, &st);
>>> +     TEST_COMPARE (st.st_mode & 0777, 3);
>>>          }
>>> -     xstat (path_file, &st);
>>> -     TEST_COMPARE (st.st_mode & 0777, 3);
>>>          /* Close the descriptors.  */
>>>        for (int *pfd = fd_list_begin (&fd_list); pfd < fd_list_end (&fd_list);
>> Why this test change?
> 
> Unless I'm misreading the original, it seems like the test would always fail if the call to `chmod_func` right before this occurs succeeds - any success is followed by two contradictory assertions that `(st.st_mode & 0777) == 2` and `(st.st_mode & 0777) == 3` (and that last test seems like its testing that the value of `st.st_mode & 0777` does not change when `chmod_func` fails.
> 

Yes, I found it when testing on a 6.6 kernel with fchmodat2.
diff mbox series

Patch

diff --git a/io/tst-lchmod.c b/io/tst-lchmod.c
index 2bf4835b05..6496dc61e0 100644
--- a/io/tst-lchmod.c
+++ b/io/tst-lchmod.c
@@ -219,9 +219,9 @@  test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t,
          /* The error code from the openat fallback leaks out.  */
          if (errno != ENFILE && errno != EMFILE)
            TEST_COMPARE (errno, EOPNOTSUPP);
+	 xstat (path_file, &st);
+	 TEST_COMPARE (st.st_mode & 0777, 3);
        }
-     xstat (path_file, &st);
-     TEST_COMPARE (st.st_mode & 0777, 3);
 
      /* Close the descriptors.  */
      for (int *pfd = fd_list_begin (&fd_list); pfd < fd_list_end (&fd_list);
diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c
index 99527a3727..99d3df6440 100644
--- a/sysdeps/unix/sysv/linux/fchmodat.c
+++ b/sysdeps/unix/sysv/linux/fchmodat.c
@@ -26,66 +26,80 @@ 
 #include <sysdep.h>
 #include <unistd.h>
 
-int
-fchmodat (int fd, const char *file, mode_t mode, int flag)
+#if !__ASSUME_FCHMODAT2
+static int
+fchmodat_fallback (int fd, const char *file, mode_t mode, int flag)
 {
-  if (flag == 0)
-    return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
-  else if (flag != AT_SYMLINK_NOFOLLOW)
+  if (flag != AT_SYMLINK_NOFOLLOW)
     return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
-  else
-    {
-      /* The kernel system call does not have a mode argument.
-	 However, we can create an O_PATH descriptor and change that
-	 via /proc (which does not resolve symbolic links).  */
 
-      int pathfd = __openat_nocancel (fd, file,
-				      O_PATH | O_NOFOLLOW | O_CLOEXEC);
-      if (pathfd < 0)
-	/* This may report errors such as ENFILE and EMFILE.  The
-	   caller can treat them as temporary if necessary.  */
-	return pathfd;
+  /* The kernel system call does not have a mode argument.
+     However, we can create an O_PATH descriptor and change that
+     via /proc (which does not resolve symbolic links).  */
 
-      /* Use fstatat because fstat does not work on O_PATH descriptors
-	 before Linux 3.6.  */
-      struct __stat64_t64 st;
-      if (__fstatat64_time64 (pathfd, "", &st, AT_EMPTY_PATH) != 0)
-	{
-	  __close_nocancel (pathfd);
-	  return -1;
-	}
+  int pathfd = __openat_nocancel (fd, file,
+				  O_PATH | O_NOFOLLOW | O_CLOEXEC);
+  if (pathfd < 0)
+    /* This may report errors such as ENFILE and EMFILE.  The
+       caller can treat them as temporary if necessary.  */
+    return pathfd;
 
-      /* 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;
-	}
+  /* Use fstatat because fstat does not work on O_PATH descriptors
+     before Linux 3.6.  */
+  struct __stat64_t64 st;
+  if (__fstatat64_time64 (pathfd, "", &st, AT_EMPTY_PATH) != 0)
+    {
+      __close_nocancel (pathfd);
+      return -1;
+    }
 
-      /* For most file systems, fchmod does not operate on O_PATH
-	 descriptors, so go through /proc.  */
-      struct fd_to_filename filename;
-      int ret = __chmod (__fd_to_filename (pathfd, &filename), mode);
-      if (ret != 0)
-	{
-	  if (errno == ENOENT)
-	    /* /proc has not been mounted.  Without /proc, there is no
-	       way to upgrade the O_PATH descriptor to a full
-	       descriptor.  It is also not possible to re-open the
-	       file without O_PATH because the file name may refer to
-	       another file, and opening that without O_PATH may have
-	       side effects (such as blocking, device rewinding, or
-	       releasing POSIX locks).  */
-	    __set_errno (EOPNOTSUPP);
-	}
+  /* 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);
-      return ret;
+      __set_errno (EOPNOTSUPP);
+      return -1;
+    }
+
+  /* For most file systems, fchmod does not operate on O_PATH
+     descriptors, so go through /proc.  */
+  struct fd_to_filename filename;
+  int ret = __chmod (__fd_to_filename (pathfd, &filename), mode);
+  if (ret != 0)
+    {
+      if (errno == ENOENT)
+	/* /proc has not been mounted.  Without /proc, there is no
+	   way to upgrade the O_PATH descriptor to a full
+	   descriptor.  It is also not possible to re-open the
+	   file without O_PATH because the file name may refer to
+	   another file, and opening that without O_PATH may have
+	   side effects (such as blocking, device rewinding, or
+	   releasing POSIX locks).  */
+	__set_errno (EOPNOTSUPP);
     }
+  __close_nocancel (pathfd);
+  return ret;
+}
+#endif
+
+int
+fchmodat (int fd, const char *file, mode_t mode, int flag)
+{
+#if __ASSUME_FCHMODAT2
+  return INLINE_SYSCALL_CALL (fchmodat2, fd, file, mode, flag);
+#else
+  if (flag == 0)
+    return INLINE_SYSCALL_CALL (fchmodat, fd, file, mode);
+
+  int r = INLINE_SYSCALL_CALL (fchmodat2, fd, file, mode, flag);
+  if (r != 0 && errno == ENOSYS)
+    return fchmodat_fallback (fd, file, mode, flag);
+  return r;
+#endif
 }
 libc_hidden_def (fchmodat)
diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
index 07b440f4ee..670d2604d2 100644
--- a/sysdeps/unix/sysv/linux/kernel-features.h
+++ b/sysdeps/unix/sysv/linux/kernel-features.h
@@ -252,4 +252,12 @@ 
 # define __ASSUME_CLONE3 0
 #endif
 
+/* The fchmodat2 system call was introduced across all architectures
+   in Linux 6.6.  */
+#if __LINUX_KERNEL_VERSION >= 0x060600
+# define __ASSUME_FCHMODAT2 1
+#else
+# define __ASSUME_FCHMODAT2 0
+#endif
+
 #endif /* kernel-features.h */