Message-ID: <20240802054252.GA1582980@pevik>
Date: Fri, 2 Aug 2024 07:42:52 +0200
From: Petr Vorel <pvorel@...e.cz>
To: Aleksa Sarai <cyphar@...har.com>
Cc: Andrea Cervesato <andrea.cervesato@...e.de>, ltp@...ts.linux.it,
	Alexey Gladkov <legion@...nel.org>,
	Christian Brauner <brauner@...nel.org>,
	Cyril Hrubis <chrubis@...e.cz>,
	Adhemerval Zanella <adhemerval.zanella@...aro.org>,
	Gaël PORTAY <gael.portay@...ne.fr>,
	linux-kernel@...r.kernel.org
Subject: Re: [LTP] [PATCH v4 0/5] Add fchmodat2 testing suite

> On 2024-08-01, Petr Vorel <pvorel@...e.cz> wrote:
> > Hi all,

> > > This is a patch-set that implements fchmodat2() syscall coverage.
> > > fchmodat2() has been added in kernel 6.6 in order to support
> > > AT_SYMLINK_NOFOLLOW and AT_EMPTY_PATH in fchmodat().
> > > There's no man pages yet, so please take the following links as
> > > main documentation along with kernel source code:

> > I would hope that it'd be at least Christian's fork [1], but it's not there.
> > I suppose nobody is working on the man page.

> > > https://www.phoronix.com/news/fchmodat2-For-Linux-6.6
> > > https://lore.kernel.org/lkml/20230824-frohlocken-vorabend-725f6fdaad50@brauner/

> > > ***********
> > > * WARNING *
> > > ***********

> > > fchmodat2_02 fails with EOPNOTSUPP because of missing feature.

> > For a record, it's fchmodat2_01.c (from this patchset) which is failing (on
> > 6.10.1-1.g4c78d6f-default Tumbleweed and 6.6.21-0-lts Alpine, both x86_64 VMs).

> > Andrea, I would personally just skip test on EOPNOTSUPP (that's what we do in
> > LTP on EOPNOTSUPP). The question why is not supported and whether is going to be
> > fixed.

> > Looking into glibc change 65341f7bbe ("linux: Use fchmodat2 on fchmod for flags
> > different than 0 (BZ 26401)") one year old change from glibc-2.39 [2] it looks
> > just accepted behavior (glibc returns EOPNOTSUPP on symlink):

> > +  /* 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;
> > +    }

> > Also musl also behaves the same on his fallback on old kernels [3]
> > (it started 10 years ago on 0dc48244 ("work around linux's lack of flags
> > argument to fchmodat syscall") when SYS_fchmodat was used and kept when this
> > year SYS_fchmodat2 started to be used in d0ed307e):

> > 	int ret = __syscall(SYS_fchmodat2, fd, path, mode, flag);
> > 	if (ret != -ENOSYS) return __syscall_ret(ret);

> > 	if (flag != AT_SYMLINK_NOFOLLOW)
> > 		return __syscall_ret(-EINVAL);

> > 	struct stat st;
> > 	int fd2;
> > 	char proc[15+3*sizeof(int)];

> > 	if (fstatat(fd, path, &st, flag))
> > 		return -1;
> > 	if (S_ISLNK(st.st_mode))
> > 		return __syscall_ret(-EOPNOTSUPP);


> > > According to documentation, the feature has been implemented in
> > > kernel 6.6, but __in reality__ AT_SYMLINK_NOFOLLOW is not working
> > > on symbolic files. Also kselftests, which are meant to test the
> > > functionality, are not working and they are treating fchmodat2()
> > > syscall failure as SKIP. Please take a look at the following code
> > > before reviewing:

> > > https://github.com/torvalds/linux/blob/8f6a15f095a63a83b096d9b29aaff4f0fbe6f6e6/tools/testing/selftests/fchmodat2/fchmodat2_test.c#L123

> > I see there is a kselftest workaround in 4859c257d295 ("selftests: Add fchmodat2
> > selftest") [4], where fchmodat2 failure on symlink is simply skipped.

> > Aleksa, you're probably aware of this fchmodat2() failure on symlinks. Does
> > anybody work or plan to work on fixing it? LTP has policy to not cover kernel
> > bugs, if it's not expected to be working we might just skip the test as well.

> If I understand the bug report, the issue is that fchmodat2() doesn't
> work on symlinks?

Yes.

> This is intentional -- Christian fixed a tree-wide bug a while ago[1]
> where some filesystems would change the mode of symlinks despite
> returning an error (usually EOPNOTSUPP) and IIRC a few others would
> happily change the mode of symlinks.

Ah, I've seen this in the past. Thanks a lot for reminding me.

> The current intended behaviour is to always return EOPNOTSUPP, and AFAIK
> there is no plan to re-enable the changing of symlink modes. EOPNOTSUPP
> was chosen because that's what filesystems were already returning.
> (While this is a little confusing, VFS syscalls return EINVAL for an
> unsupported flag, not EOPNOTSUPP.)

> The benefit of an AT_SYMLINK_NOFOLLOW flag is not just to to allow a
> syscall to operate on symlinks, it also allows programs to safely
> operate on path components without worrying about symlinks being
> followed (this is relevant for container runtimes, where we are
> operating on untrusted filesystem roots -- though in the case of
> fchmodat2(2) you would probably just use AT_EMPTY_PATH in practice). So
> an error here is actually what you want as a program that uses
> AT_SYMLINK_NOFOLLOW (since the actual operation is intentionally not
> supported by filesystems).

Thanks a lot for explaining the background!

> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5d1f903f75a80daa4dfb3d84e114ec8ecbf29956

> > I see a RFC UAPI related patchset [5] which touches include/uapi/linux/fcntl.h,
> > but AFAIK it's not related to this problem.

> Yeah this is unrelated, that patch is about clarifying how AT_* flags
> are allocated, not syscall behaviour.

Thanks!

> > Kind regards,
> > Petr

@Andrea, I guess we want something like this:

+++ testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
@@ -43,9 +43,10 @@ static void test_symbolic_link(void)
 	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
 	verify_mode(fd_dir, SNAME, S_IFLNK | 0777);
 
-	TST_EXP_PASS(fchmodat2(fd_dir, SNAME, 0640, AT_SYMLINK_NOFOLLOW));
-	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
-	verify_mode(fd_dir, SNAME, S_IFLNK | 0640);
+	if (tst_kvercmp(6, 6, 0) >= 0) {
+		TST_EXP_FAIL(tst_syscall(__NR_fchmodat2, fd_dir, SNAME, 0640,
+					 AT_SYMLINK_NOFOLLOW), EOPNOTSUPP);
+	}
 }
 
 static void test_empty_folder(void)

Powered by blists - more mailing lists