Re: [PATCHv2 2/3] selftests: Add test of O_BENEATH & openat(2)

Re: [PATCHv2 2/3] selftests: Add test of O_BENEATH & openat(2)

[Dave Chinner]

[cc fstests@vger.kernel.org]

On Tue, Nov 04, 2014 at 09:54:43AM +0000, David Drysdale wrote:
> Add simple tests of openat(2) variations, including examples that
> check the new O_BENEATH flag.
> 
> Signed-off-by: David Drysdale <drysdale@google.com>

Wouldn't this be better added to fstests? That's the regression
test suite used by filesystem developers and most distro QA
organisations and where the fs developers aggregate all their new
regression tests.

IMO, the fewer places we aggregate VFS/filesystem tests the better.
I really don't think the kernel tree is the best place for adding
VFS behavioural tests because it has none of the infrastructure
around it to test arbitrary filesystems and configurations and hence
is not particularly useful to the people whoa re likely to notice
and care about fs regression tests suddenly breaking.

As an example, the recent renameat() syscall additions (e.g.
RENAME_EXCHANGE, RENAME_NOREPLACE) have unit tests in fstests, so
these new O_BENEATH tests should really follow the same model...

Cheers,

Dave.

> ---
>  tools/testing/selftests/Makefile          |   1 +
>  tools/testing/selftests/openat/.gitignore |   4 +
>  tools/testing/selftests/openat/Makefile   |  28 +++++
>  tools/testing/selftests/openat/openat.c   | 180 ++++++++++++++++++++++++++++++
>  4 files changed, 213 insertions(+)
>  create mode 100644 tools/testing/selftests/openat/.gitignore
>  create mode 100644 tools/testing/selftests/openat/Makefile
>  create mode 100644 tools/testing/selftests/openat/openat.c
> 
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index 36ff2e4c7b6f..812e973233d2 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -14,6 +14,7 @@ TARGETS += powerpc
>  TARGETS += user
>  TARGETS += sysctl
>  TARGETS += firmware
> +TARGETS += openat
> 
>  TARGETS_HOTPLUG = cpu-hotplug
>  TARGETS_HOTPLUG += memory-hotplug
> diff --git a/tools/testing/selftests/openat/.gitignore b/tools/testing/selftests/openat/.gitignore
> new file mode 100644
> index 000000000000..835b2dcd8678
> --- /dev/null
> +++ b/tools/testing/selftests/openat/.gitignore
> @@ -0,0 +1,4 @@
> +openat
> +subdir
> +topfile
> +symlinkdown
> \ No newline at end of file
> diff --git a/tools/testing/selftests/openat/Makefile b/tools/testing/selftests/openat/Makefile
> new file mode 100644
> index 000000000000..84cd06e7ee82
> --- /dev/null
> +++ b/tools/testing/selftests/openat/Makefile
> @@ -0,0 +1,28 @@
> +CC = $(CROSS_COMPILE)gcc
> +CFLAGS = -Wall
> +BINARIES = openat
> +DEPS = subdir topfile symlinkdown subdir/bottomfile subdir/symlinkup subdir/symlinkout subdir/symlinkin
> +all: $(BINARIES) $(DEPS)
> +
> +subdir:
> +	mkdir -p subdir
> +topfile:
> +	echo 0123456789 > $@
> +subdir/bottomfile: | subdir
> +	echo 0123456789 > $@
> +subdir/symlinkup: | subdir
> +	ln -s ../topfile $@
> +subdir/symlinkout: | subdir
> +	ln -s /etc/passwd $@
> +subdir/symlinkin: | subdir
> +	ln -s bottomfile $@
> +symlinkdown:
> +	ln -s subdir/bottomfile $@
> +%: %.c
> +	$(CC) $(CFLAGS) -o $@ $^
> +
> +run_tests: all
> +	./openat
> +
> +clean:
> +	rm -rf $(BINARIES) $(DEPS)
> diff --git a/tools/testing/selftests/openat/openat.c b/tools/testing/selftests/openat/openat.c
> new file mode 100644
> index 000000000000..0c030cbd10dc
> --- /dev/null
> +++ b/tools/testing/selftests/openat/openat.c
> @@ -0,0 +1,180 @@
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/syscall.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <errno.h>
> +
> +#include <linux/fcntl.h>
> +
> +/* Bypass glibc */
> +static int openat_(int dirfd, const char *pathname, int flags)
> +{
> +	return syscall(__NR_openat, dirfd, pathname, flags);
> +}
> +
> +static int openat_or_die(int dfd, const char *path, int flags)
> +{
> +	int fd = openat_(dfd, path, flags);
> +
> +	if (fd < 0) {
> +		printf("Failed to openat(%d, '%s'); "
> +			"check prerequisites are available\n", dfd, path);
> +		exit(1);
> +	}
> +	return fd;
> +}
> +
> +static int check_openat(int dfd, const char *path, int flags)
> +{
> +	int rc;
> +	int fd;
> +	char buffer[4];
> +
> +	errno = 0;
> +	printf("Check success of openat(%d, '%s', %x)... ",
> +	       dfd, path?:"(null)", flags);
> +	fd = openat_(dfd, path, flags);
> +	if (fd < 0) {
> +		printf("[FAIL]: openat() failed, rc=%d errno=%d (%s)\n",
> +			fd, errno, strerror(errno));
> +		return 1;
> +	}
> +	errno = 0;
> +	rc = read(fd, buffer, sizeof(buffer));
> +	if (rc < 0) {
> +		printf("[FAIL]: read() failed, rc=%d errno=%d (%s)\n",
> +			rc, errno, strerror(errno));
> +		return 1;
> +	}
> +	close(fd);
> +	printf("[OK]\n");
> +	return 0;
> +}
> +
> +#define check_openat_fail(dfd, path, flags, errno)	\
> +	_check_openat_fail(dfd, path, flags, errno, #errno)
> +static int _check_openat_fail(int dfd, const char *path, int flags,
> +			      int expected_errno, const char *errno_str)
> +{
> +	int rc;
> +
> +	errno = 0;
> +	printf("Check failure of openat(%d, '%s', %x) with %s... ",
> +		dfd, path?:"(null)", flags, errno_str);
> +	rc = openat_(dfd, path, flags);
> +	if (rc > 0) {
> +		printf("[FAIL] (unexpected success from openat(2))\n");
> +		close(rc);
> +		return 1;
> +	}
> +	if (errno != expected_errno) {
> +		printf("[FAIL] (expected errno %d (%s) not %d (%s)\n",
> +			expected_errno, strerror(expected_errno),
> +			errno, strerror(errno));
> +		return 1;
> +	}
> +	printf("[OK]\n");
> +	return 0;
> +}
> +
> +int check_proc(void)
> +{
> +	int proc_dfd = openat_(AT_FDCWD, "/proc/self", O_RDONLY);
> +	int fail = 0;
> +
> +	if (proc_dfd < 0) {
> +		printf("'/proc/self' unavailable (errno=%d '%s'), skipping\n",
> +			errno, strerror(errno));
> +		return 1;
> +	}
> +	fail |= check_openat(proc_dfd, "root/etc/passwd", O_RDONLY);
> +#ifdef O_BENEATH
> +	fail |= check_openat_fail(proc_dfd, "root/etc/passwd",
> +				  O_RDONLY|O_BENEATH, EPERM);
> +#endif
> +	return fail;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	int fail = 0;
> +	int dot_dfd = openat_or_die(AT_FDCWD, ".", O_RDONLY);
> +	int subdir_dfd = openat_or_die(AT_FDCWD, "subdir", O_RDONLY);
> +	int file_fd = openat_or_die(AT_FDCWD, "topfile", O_RDONLY);
> +
> +	/* Sanity check normal behavior */
> +	fail |= check_openat(AT_FDCWD, "topfile", O_RDONLY);
> +	fail |= check_openat(AT_FDCWD, "subdir/bottomfile", O_RDONLY);
> +
> +	fail |= check_openat(dot_dfd, "topfile", O_RDONLY);
> +	fail |= check_openat(dot_dfd, "subdir/bottomfile", O_RDONLY);
> +	fail |= check_openat(dot_dfd, "subdir/../topfile", O_RDONLY);
> +
> +	fail |= check_openat(subdir_dfd, "../topfile", O_RDONLY);
> +	fail |= check_openat(subdir_dfd, "bottomfile", O_RDONLY);
> +	fail |= check_openat(subdir_dfd, "../subdir/bottomfile", O_RDONLY);
> +	fail |= check_openat(subdir_dfd, "symlinkup", O_RDONLY);
> +	fail |= check_openat(subdir_dfd, "symlinkout", O_RDONLY);
> +
> +	fail |= check_openat(AT_FDCWD, "/etc/passwd", O_RDONLY);
> +	fail |= check_openat(dot_dfd, "/etc/passwd", O_RDONLY);
> +	fail |= check_openat(subdir_dfd, "/etc/passwd", O_RDONLY);
> +
> +	fail |= check_openat_fail(AT_FDCWD, "bogus", O_RDONLY, ENOENT);
> +	fail |= check_openat_fail(dot_dfd, "bogus", O_RDONLY, ENOENT);
> +	fail |= check_openat_fail(999, "bogus", O_RDONLY, EBADF);
> +	fail |= check_openat_fail(file_fd, "bogus", O_RDONLY, ENOTDIR);
> +
> +#ifdef O_BENEATH
> +	/* Test out O_BENEATH */
> +	fail |= check_openat(AT_FDCWD, "topfile", O_RDONLY|O_BENEATH);
> +	fail |= check_openat(AT_FDCWD, "subdir/bottomfile",
> +			     O_RDONLY|O_BENEATH);
> +
> +	fail |= check_openat(dot_dfd, "topfile", O_RDONLY|O_BENEATH);
> +	fail |= check_openat(dot_dfd, "subdir/bottomfile",
> +			     O_RDONLY|O_BENEATH);
> +	fail |= check_openat(subdir_dfd, "bottomfile", O_RDONLY|O_BENEATH);
> +
> +	/* Symlinks without .. or leading / are OK */
> +	fail |= check_openat(dot_dfd, "symlinkdown", O_RDONLY|O_BENEATH);
> +	fail |= check_openat(dot_dfd, "subdir/symlinkin", O_RDONLY|O_BENEATH);
> +	fail |= check_openat(subdir_dfd, "symlinkin", O_RDONLY|O_BENEATH);
> +	/* ... unless of course we specify O_NOFOLLOW */
> +	fail |= check_openat_fail(dot_dfd, "symlinkdown",
> +				  O_RDONLY|O_BENEATH|O_NOFOLLOW, ELOOP);
> +	fail |= check_openat_fail(dot_dfd, "subdir/symlinkin",
> +				  O_RDONLY|O_BENEATH|O_NOFOLLOW, ELOOP);
> +	fail |= check_openat_fail(subdir_dfd, "symlinkin",
> +				  O_RDONLY|O_BENEATH|O_NOFOLLOW, ELOOP);
> +
> +	/* Can't open paths with ".." in them */
> +	fail |= check_openat_fail(dot_dfd, "subdir/../topfile",
> +				O_RDONLY|O_BENEATH, EPERM);
> +	fail |= check_openat_fail(subdir_dfd, "../topfile",
> +				  O_RDONLY|O_BENEATH, EPERM);
> +	fail |= check_openat_fail(subdir_dfd, "../subdir/bottomfile",
> +				O_RDONLY|O_BENEATH, EPERM);
> +
> +	/* Can't open paths starting with "/" */
> +	fail |= check_openat_fail(AT_FDCWD, "/etc/passwd",
> +				  O_RDONLY|O_BENEATH, EPERM);
> +	fail |= check_openat_fail(dot_dfd, "/etc/passwd",
> +				  O_RDONLY|O_BENEATH, EPERM);
> +	fail |= check_openat_fail(subdir_dfd, "/etc/passwd",
> +				  O_RDONLY|O_BENEATH, EPERM);
> +	/* Can't sneak around constraints with symlinks */
> +	fail |= check_openat_fail(subdir_dfd, "symlinkup",
> +				  O_RDONLY|O_BENEATH, EPERM);
> +	fail |= check_openat_fail(subdir_dfd, "symlinkout",
> +				  O_RDONLY|O_BENEATH, EPERM);
> +#else
> +	printf("Skipping O_BENEATH tests due to missing #define\n");
> +#endif
> +	fail |= check_proc();
> +
> +	return fail ? -1 : 0;
> +}
> --
> 2.1.0.rc2.206.gedb03e5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCHv2 2/3] selftests: Add test of O_BENEATH & openat(2)

[David Drysdale]

On Tue, Nov 11, 2014 at 5:36 AM, Dave Chinner <david@fromorbit.com> wrote:
> [cc fstests@vger.kernel.org]
>
> On Tue, Nov 04, 2014 at 09:54:43AM +0000, David Drysdale wrote:
>> Add simple tests of openat(2) variations, including examples that
>> check the new O_BENEATH flag.
>>
>> Signed-off-by: David Drysdale <drysdale@google.com>
>
> Wouldn't this be better added to fstests? That's the regression
> test suite used by filesystem developers and most distro QA
> organisations and where the fs developers aggregate all their new
> regression tests.
>
> IMO, the fewer places we aggregate VFS/filesystem tests the better.
> I really don't think the kernel tree is the best place for adding
> VFS behavioural tests because it has none of the infrastructure
> around it to test arbitrary filesystems and configurations and hence
> is not particularly useful to the people whoa re likely to notice
> and care about fs regression tests suddenly breaking.
>
> As an example, the recent renameat() syscall additions (e.g.
> RENAME_EXCHANGE, RENAME_NOREPLACE) have unit tests in fstests, so
> these new O_BENEATH tests should really follow the same model...
>
> Cheers,
>
> Dave.

Fair enough, that makes sense -- I've now got a version of the selftest
running within xfstests (git://oss.sgi.com/xfs/cmds/xfstests.git is the
master repo, right?).

Given that xfstests is independent of the kernel, what's the expected
way to deal with flags (or syscalls) that are only in specific kernel
versions?   At the moment I've just got a primitive override at
compile time (#ifndef O_BENEATH #define O_BENEATH ...), and
then the test will fail at run-time against an older kernel -- is there a
need for anything more sophisticated?  (And if so, are there any
examples I can crib from?)

Also, is there an archive of the fstests@ mailing list somewhere?

Thanks,
David

Re: [PATCHv2 2/3] selftests: Add test of O_BENEATH & openat(2)

[Dave Chinner]

On Fri, Nov 21, 2014 at 02:19:41PM +0000, David Drysdale wrote:
> On Tue, Nov 11, 2014 at 5:36 AM, Dave Chinner <david@fromorbit.com> wrote:
> > [cc fstests@vger.kernel.org]
> >
> > On Tue, Nov 04, 2014 at 09:54:43AM +0000, David Drysdale wrote:
> >> Add simple tests of openat(2) variations, including examples that
> >> check the new O_BENEATH flag.
> >>
> >> Signed-off-by: David Drysdale <drysdale@google.com>
> >
> > Wouldn't this be better added to fstests? That's the regression
> > test suite used by filesystem developers and most distro QA
> > organisations and where the fs developers aggregate all their new
> > regression tests.
> >
> > IMO, the fewer places we aggregate VFS/filesystem tests the better.
> > I really don't think the kernel tree is the best place for adding
> > VFS behavioural tests because it has none of the infrastructure
> > around it to test arbitrary filesystems and configurations and hence
> > is not particularly useful to the people whoa re likely to notice
> > and care about fs regression tests suddenly breaking.
> >
> > As an example, the recent renameat() syscall additions (e.g.
> > RENAME_EXCHANGE, RENAME_NOREPLACE) have unit tests in fstests, so
> > these new O_BENEATH tests should really follow the same model...
> 
> Fair enough, that makes sense -- I've now got a version of the selftest
> running within xfstests (git://oss.sgi.com/xfs/cmds/xfstests.git is the
> master repo, right?).

Or git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git, which is
where I typically update first and push dev branches to.

> Given that xfstests is independent of the kernel, what's the expected
> way to deal with flags (or syscalls) that are only in specific kernel
> versions?  At the moment I've just got a primitive override at
> compile time (#ifndef O_BENEATH #define O_BENEATH ...), and
> then the test will fail at run-time against an older kernel -- is there a
> need for anything more sophisticated?  (And if so, are there any
> examples I can crib from?)

See the code in the src/renameat2.c for an example of how syscalls
and their flags are added prior to their being kernel and userspace
header support.

Also, note the "-t" CLI option for the "test" option in that little
program.  This is used by the _require_renameat2 function that the
tests that make use of this functionality call to determine if th
etest shoul dbe run on this kernel or not.

> Also, is there an archive of the fstests@ mailing list somewhere?

I wish. I've tried to get it archived on all the major sites like
marc, spinics, etc repeatedly since the list was created and not a
single one of them have responded to any of my requests, let alone
started to archive the list....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com