Re: [Devel] [PATCH vz10] selftests: binderfs: skip the stress test without binderfs

[Konstantin Khorenko <khorenko@virtuozzo.com>]

> binderfs_test_stress mounts binderfs 32 times and asserts on the first
> mount, so on a kernel without CONFIG_ANDROID_BINDERFS the case fails
> rather than reporting that the filesystem is not there.
> 
> Probe once with a mount into a temporary directory and skip when the
> kernel answers ENODEV. Only ENODEV means the filesystem is missing;
> anything else is a real failure and still fails, as does a temporary
> directory we cannot create.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-142449
> Feature: fix selftests
> Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
>
> diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> index 319567f0fae14..5d47e27e9dab8 100644
> --- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> +++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> @@ -379,6 +379,17 @@ static void *binder_version_thread(void *data)
>   */
>  TEST(binderfs_stress)
>  {
> +	char probe[] = P_tmpdir "/binderfs_probe_XXXXXX";
> +	int probe_ret;
> +
> +	ASSERT_NE(NULL, mkdtemp(probe));
> +	probe_ret = mount(NULL, probe, "binder", 0, 0);
> +	if (!probe_ret)
> +		umount2(probe, MNT_DETACH);
> +	rmdir(probe);
> +	if (probe_ret && errno == ENODEV)

errno is checked after rmdir() => original errno has been already lost.

> +		SKIP(return, "The Android binderfs filesystem is not available");
> +

The block is inserted before other declarations, the style is strange.

>  	int fds[1000];
>  	int syncfds[2];
>  	pid_t pid;

Another angle of view:

  The probe cannot tell "no binderfs" from "no permission", and the
  unprivileged case is the whole point.

  path_mount() rejects an unprivileged caller in may_mount()
  (fs/namespace.c:4194, -EPERM) before do_new_mount() looks up the filesystem
  type and returns -ENODEV (fs/namespace.c:3887). A non-root probe therefore
  always gets EPERM, binderfs present or not.

  And binderfs_stress is meant to run unprivileged: change_idmaps() writes
  "0 <getuid()> 1" (binderfs_test.c:338), a single-id identity mapping an
  unprivileged process may write for its own child; change_userns() then gives
  it setid_userns_root(), and FS_USERNS_MOUNT on binder_fs_type permits the
  mount inside the userns. So for a non-root run without
  CONFIG_ANDROID_BINDERFS:

  - the probe in the parent gets EPERM, errno != ENODEV, no skip;
  - the child reaches the real mount() inside the userns, gets ENODEV, and
    ASSERT_EQ(ret, 0) fails - the original problem is untouched.



All-in-all, i would suggest another way of fixing it - check /proc/filesystems
instead:

  - tools/testing/selftests/landlock/fs_test.c:131
      supports_filesystem(), searches "nodev\t<fs>\n";
  - tools/testing/selftests/resctrl/resctrlfs.c:756
      check_resctrlfs_support(), searches "nodev\tresctrl\n";
  - tools/testing/selftests/mm/run_vmtests.sh:440
      same for xfs from shell.

A helper along with change_mountns():

static bool binderfs_supported(void)
{
      char line[128];
      bool ret = false;
      FILE *f;

      f = fopen("/proc/filesystems", "r");
      if (!f)
              return true;    /* Cannot tell - let the test run and report. */

      while (fgets(line, sizeof(line), f)) {
              /* binderfs has no backing device, hence the "nodev" prefix. */
              if (!strcmp(line, "nodev\tbinder\n")) {
                      ret = true;
                      break;
              }
      }

      fclose(f);
      return ret;
}

and in TEST(binderfs_stress) right after all declarations:

      if (!binderfs_supported())
              SKIP(return, "The Android binderfs filesystem is not available");

-- 
Konstantin Khorenko <khorenko@virtuozzo.com>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel