[6/6] linux: Use fchmodat2 on fchmod for flags different than 0 (BZ 26401)
Checks
Context |
Check |
Description |
redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
redhat-pt-bot/TryBot-32bit |
success
|
Build for i686
|
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
fail
|
Testing failed
|
linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-arm |
fail
|
Testing failed
|
Commit Message
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
* 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?
* Adhemerval Zanella:
> PS: setting it as RFC because there is no Linux 6.6 release yet.
Please remove this from the commit message.
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.
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.
@@ -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);
@@ -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)
@@ -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 */