[PATCH 0/4] Tests for overlayfs immutable/append-only files

[PATCH 0/4] Tests for overlayfs immutable/append-only files

[Amir Goldstein]

Eryu,

Overlayfs never had full support for immutable/append-only files.
Whatever works is covered by generic/079 and overlay/030 tests.
Both tests cover only upper files and directories.
generic/079 is notrun on kernel < 5.10 and passes on >= 5.10.

This series improves the t_immutable test program and adds a new test
to cover lower files and directories - the test fails on upstream kernel.
Fixing this requires some VFS API changes that Miklos has proposed [1].

The new test covers two reported bug, one of them is a deadlock.
The deadlock trigger is commented out until we have a fix upstream.

Thanks,
Amir. 

[1] https://lore.kernel.org/linux-unionfs/20201123141207.GC327006@miu.piliscsaba.redhat.com/

Amir Goldstein (4):
  overlay/030: Update comment w.r.t upstream kernel
  src/t_immutable: factor out some helpers
  src/t_immutable: Allow setting flags on existing files
  overlay: Test lost immutable/append-only flags on copy-up

 src/t_immutable.c     | 241 +++++++++++++++++++++---------------------
 tests/overlay/030     |   7 +-
 tests/overlay/075     |  97 +++++++++++++++++
 tests/overlay/075.out |  11 ++
 tests/overlay/group   |   1 +
 5 files changed, 237 insertions(+), 120 deletions(-)
 create mode 100755 tests/overlay/075
 create mode 100644 tests/overlay/075.out

-- 
2.25.1

[PATCH 1/4] overlay/030: Update comment w.r.t upstream kernel

[Amir Goldstein]

commit 61536bed2149 ("ovl: support [S|G]ETFLAGS and FS[S|G]ETXATTR
ioctls for directories") makes the comment in test header inaccurate.
Fix the comment to include this information.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/030 | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/overlay/030 b/tests/overlay/030
index 3ef206b6..c461e502 100755
--- a/tests/overlay/030
+++ b/tests/overlay/030
@@ -8,8 +8,11 @@
 # and directories in an overlayfs upper directory.
 #
 # This test is similar and was derived from generic/079, but
-# the original test is _notrun on overlay mount because FS_IOC_GETFLAGS
-# FS_IOC_SETFLAGS ioctls fail on overlay directory inodes.
+# the original test is _notrun with FSTYP=overlay on kernel < v5.10
+# because prior to commit 61536bed2149 ("ovl: support [S|G]ETFLAGS
+# and FS[S|G]ETXATTR ioctls for directories"), t_immutable -c would
+# fail to prepare immutable/append-only directories on the overlay
+# mount path.
 #
 seq=`basename $0`
 seqres=$RESULT_DIR/$seq
-- 
2.25.1

[PATCH 2/4] src/t_immutable: factor out some helpers

[Amir Goldstein]

Reduce boilerplate code.
define _GNU_SOURCE needed for asprintf.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 src/t_immutable.c | 221 ++++++++++++++++++++++------------------------
 1 file changed, 104 insertions(+), 117 deletions(-)

diff --git a/src/t_immutable.c b/src/t_immutable.c
index 86c567ed..b6a76af0 100644
--- a/src/t_immutable.c
+++ b/src/t_immutable.c
@@ -8,6 +8,9 @@
 
 #define TEST_UTIME
 
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -1895,13 +1898,66 @@ static int check_test_area(const char *dir)
      return 0;
 }
 
+static int create_dir(char **ppath, const char *fmt, const char *dir)
+{
+     const char *path;
+     struct stat st;
+
+     if (asprintf(ppath, fmt, dir) == -1) {
+	  return -1;
+     }
+     path = *ppath;
+     if (stat(path, &st) == 0) {
+	  fprintf(stderr, "%s: Test area directory %s must not exist for test area creation.\n",
+		  __progname, path);
+	  return 1;
+     }
+     if (mkdir(path, 0777) != 0) {
+	  fprintf(stderr, "%s: error creating directory %s: %s\n", __progname, path, strerror(errno));
+	  return -1;
+     }
+     return 0;
+}
+
+static int create_file(char **ppath, const char *fmt, const char *dir)
+{
+     const char *path;
+     int fd;
+
+     if (asprintf(ppath, fmt, dir) == -1) {
+	  return -1;
+     }
+     path = *ppath;
+     if ((fd = open(path, O_WRONLY|O_CREAT|O_EXCL, 0666)) == -1) {
+	  fprintf(stderr, "%s: error creating file %s: %s\n", __progname, path, strerror(errno));
+          return -1;
+     }
+     return fd;
+}
+
+static int create_xattrs(int fd)
+{
+     if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
+	  if (errno != EOPNOTSUPP) {
+	       perror("setxattr");
+	       return 1;
+	  }
+     }
+     if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
+	  if (errno != EOPNOTSUPP) {
+	       perror("setxattr");
+	       return 1;
+	  }
+     }
+     return 0;
+}
+
 static int create_test_area(const char *dir)
 {
      int fd;
      char *path;
      static const char *acl_u_text = "u::rw-,g::rw-,o::rw-,u:nobody:rw-,m::rw-";
      static const char *acl_u_text_d = "u::rwx,g::rwx,o::rwx,u:nobody:rwx,m::rwx";
-     struct stat st;
      static const char *immutable = "This is an immutable file.\nIts contents cannot be altered.\n";
      static const char *append_only = "This is an append-only file.\nIts contents cannot be altered.\n"
 	  "Data can only be appended.\n---\n";
@@ -1911,79 +1967,45 @@ static int create_test_area(const char *dir)
 	  return 1;
      }
 
-     if (stat(dir, &st) == 0) {
-	  fprintf(stderr, "%s: Test area directory %s must not exist for test area creation.\n",
-		  __progname, dir);
-	  return 1;
-     }
-
      umask(0000);
-     if (mkdir(dir, 0777) != 0) {
-	  fprintf(stderr, "%s: error creating directory %s: %s\n", __progname, dir, strerror(errno));
+     if (create_dir(&path, "%s", dir)) {
 	  return 1;
      }
-
-     asprintf(&path, "%s/immutable.d", dir);
-     if (mkdir(path, 0777) != 0) {
-          fprintf(stderr, "%s: error creating directory %s: %s\n", __progname, path, strerror(errno));
-          return 1;
-     }
      free(path);
 
-     asprintf(&path, "%s/empty-immutable.d", dir);
-     if (mkdir(path, 0777) != 0) {
-          fprintf(stderr, "%s: error creating directory %s: %s\n", __progname, path, strerror(errno));
-          return 1;
+     if (create_dir(&path, "%s/append-only.d", dir)) {
+	  return 1;
      }
      free(path);
 
-     asprintf(&path, "%s/append-only.d", dir);
-     if (mkdir(path, 0777) != 0) {
-          fprintf(stderr, "%s: error creating directory %s: %s\n", __progname, path, strerror(errno));
-          return 1;
+     if (create_dir(&path, "%s/append-only.d/dir", dir)) {
+	  return 1;
      }
      free(path);
 
-     asprintf(&path, "%s/empty-append-only.d", dir);
-     if (mkdir(path, 0777) != 0) {
-          fprintf(stderr, "%s: error creating directory %s: %s\n", __progname, path, strerror(errno));
+     if ((fd = create_file(&path, "%s/append-only.d/file", dir)) == -1) {
           return 1;
      }
+     close(fd);
      free(path);
 
-     asprintf(&path, "%s/immutable.d/dir", dir);
-     if (mkdir(path, 0777) != 0) {
-          fprintf(stderr, "%s: error creating directory %s: %s\n", __progname, path, strerror(errno));
-          return 1;
+     if (create_dir(&path, "%s/immutable.d", dir)) {
+	  return 1;
      }
      free(path);
 
-     asprintf(&path, "%s/append-only.d/dir", dir);
-     if (mkdir(path, 0777) != 0) {
-          fprintf(stderr, "%s: error creating directory %s: %s\n", __progname, path, strerror(errno));
-          return 1;
+     if (create_dir(&path, "%s/immutable.d/dir", dir)) {
+	  return 1;
      }
      free(path);
 
-     asprintf(&path, "%s/append-only.d/file", dir);
-     if ((fd = open(path, O_WRONLY|O_CREAT|O_EXCL, 0666)) == -1) {
-	  fprintf(stderr, "%s: error creating file %s: %s\n", __progname, path, strerror(errno));
+     if ((fd = create_file(&path, "%s/immutable.d/file", dir)) == -1) {
           return 1;
      }
      close(fd);
      free(path);
 
-     asprintf(&path, "%s/immutable.d/file", dir);
-     if ((fd = open(path, O_WRONLY|O_CREAT|O_EXCL, 0666)) == -1) {
-          fprintf(stderr, "%s: error creating file %s: %s\n", __progname, path, strerror(errno));
-          return 1;
-     }
-     close(fd);
-     free(path);
-
-     asprintf(&path, "%s/immutable.f", dir);
-     if ((fd = open(path, O_WRONLY|O_CREAT|O_EXCL, 0666)) == -1) {
-          fprintf(stderr, "%s: error creating file %s: %s\n", __progname, path, strerror(errno));
+     if ((fd = create_file(&path, "%s/immutable.f", dir)) == -1) {
           return 1;
      }
      if (write(fd, immutable, strlen(immutable)) != strlen(immutable)) {
@@ -1994,17 +2016,8 @@ static int create_test_area(const char *dir)
 	  perror("acl");
 	  return 1;
      }
-     if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
-	  if (errno != EOPNOTSUPP) {
-	       perror("setxattr");
-	       return 1;
-	  }
-     }
-     if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
-	  if (errno != EOPNOTSUPP) {
-	       perror("setxattr");
-	       return 1;
-	  }
+     if (create_xattrs(fd)) {
+	  return 1;
      }
      if (fsetflag(path, fd, 1, 1)) {
           perror("fsetflag");
@@ -2014,8 +2027,7 @@ static int create_test_area(const char *dir)
      close(fd);
      free(path);
 
-     asprintf(&path, "%s/append-only.f", dir);
-     if ((fd = open(path, O_WRONLY|O_CREAT|O_EXCL, 0666)) == -1) {
+     if ((fd = create_file(&path, "%s/append-only.f", dir)) == -1) {
           fprintf(stderr, "%s: error creating file %s: %s\n", __progname, path, strerror(errno));
           return 1;
      }
@@ -2027,17 +2039,8 @@ static int create_test_area(const char *dir)
           perror("acl");
           return 1;
      }
-     if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
-	  if (errno != EOPNOTSUPP) {
-	       perror("setxattr");
-	       return 1;
-	  }
-     }
-     if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
-	  if (errno != EOPNOTSUPP) {
-	       perror("setxattr");
-	       return 1;
-	  }
+     if (create_xattrs(fd)) {
+	  return 1;
      }
      if (fsetflag(path, fd, 1, 0)) {
           perror("fsetflag");
@@ -2056,17 +2059,8 @@ static int create_test_area(const char *dir)
           perror("acl");
           return 1;
      }
-     if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
-	  if (errno != EOPNOTSUPP) {
-	       perror("setxattr");
-	       return 1;
-	  }
-     }
-     if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
-	  if (errno != EOPNOTSUPP) {
-	       perror("setxattr");
-	       return 1;
-	  }
+     if (create_xattrs(fd)) {
+	  return 1;
      }
      if (fsetflag(path, fd, 1, 1)) {
           perror("fsetflag");
@@ -2076,7 +2070,9 @@ static int create_test_area(const char *dir)
      close(fd);
      free(path);
 
-     asprintf(&path, "%s/empty-immutable.d", dir);
+     if (create_dir(&path, "%s/empty-immutable.d", dir)) {
+	  return 1;
+     }
      if ((fd = open(path, O_RDONLY)) == -1) {
           fprintf(stderr, "%s: error opening %s: %s\n", __progname, path, strerror(errno));
           return 1;
@@ -2098,17 +2094,8 @@ static int create_test_area(const char *dir)
           perror("acl");
           return 1;
      }
-     if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
-	  if (errno != EOPNOTSUPP) {
-	       perror("setxattr");
-	       return 1;
-	  }
-     }
-     if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
-	  if (errno != EOPNOTSUPP) {
-	       perror("setxattr");
-	       return 1;
-	  }
+     if (create_xattrs(fd)) {
+	  return 1;
      }
      if (fsetflag(path, fd, 1, 0)) {
           perror("fsetflag");
@@ -2118,7 +2105,9 @@ static int create_test_area(const char *dir)
      close(fd);
      free(path);
 
-     asprintf(&path, "%s/empty-append-only.d", dir);
+     if (create_dir(&path, "%s/empty-append-only.d", dir)) {
+	  return 1;
+     }
      if ((fd = open(path, O_RDONLY)) == -1) {
           fprintf(stderr, "%s: error opening %s: %s\n", __progname, path, strerror(errno));
           return 1;
@@ -2242,6 +2231,7 @@ int main(int argc, char **argv)
 {
      int ret;
      int failed = 0;
+     int runtest = 1, create = 0, remove = 0;
 
 /* this arg parsing is gross, but who cares, its a test program */
 
@@ -2251,32 +2241,29 @@ int main(int argc, char **argv)
      }
 
      if (!strcmp(argv[1], "-c")) {
-	  if (argc == 3) {
-	       if ((ret = create_test_area(argv[argc-1])))
-		    return ret;
-	  } else {
-	       fprintf(stderr, "usage: t_immutable -c test_area_dir\n");
-	       return 1;
-	  }
+	  create = 1;
      } else if (!strcmp(argv[1], "-C")) {
-          if (argc == 3) {
-               return create_test_area(argv[argc-1]);
-          } else {
-               fprintf(stderr, "usage: t_immutable -C test_area_dir\n");
-               return 1;
-          }
+	  /* Prepare test area without running tests */
+	  create = 1;
+	  runtest = 0;
      } else if (!strcmp(argv[1], "-r")) {
-	  if (argc == 3)
-	       return remove_test_area(argv[argc-1]);
-	  else {
-	       fprintf(stderr, "usage: t_immutable -r test_area_dir\n");
-	       return 1;
-	  }
-     } else if (argc != 2) {
-	  fprintf(stderr, "usage: t_immutable [-c|-r] test_area_dir\n");
+	  remove = 1;
+     }
+
+     if (argc != 2 + (create | remove)) {
+	  fprintf(stderr, "usage: t_immutable [-C|-c|-r] test_area_dir\n");
 	  return 1;
      }
 
+     if (create) {
+	  ret = create_test_area(argv[argc-1]);
+	  if (ret || !runtest) {
+               return ret;
+	  }
+     } else if (remove) {
+	  return remove_test_area(argv[argc-1]);
+     }
+
      umask(0000);
 
      if (check_test_area(argv[argc-1]))
-- 
2.25.1

[PATCH 3/4] src/t_immutable: Allow setting flags on existing files

[Amir Goldstein]

For overlayfs tests we need to be able to setflags on existing
(lower) files.

t_immutable -C test_dir

Creates the test area and sets flags, but it also allows setting flags
on an existing test area.

t_immutable -R test_dir

Removes the flags from existing test area, but does not remove the files
in the test area.

To setup a test area with file without flags, need to run the -C and -R
commands.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 src/t_immutable.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/src/t_immutable.c b/src/t_immutable.c
index b6a76af0..a2e6796d 100644
--- a/src/t_immutable.c
+++ b/src/t_immutable.c
@@ -1898,6 +1898,8 @@ static int check_test_area(const char *dir)
      return 0;
 }
 
+static int allow_existing;
+
 static int create_dir(char **ppath, const char *fmt, const char *dir)
 {
      const char *path;
@@ -1908,6 +1910,9 @@ static int create_dir(char **ppath, const char *fmt, const char *dir)
      }
      path = *ppath;
      if (stat(path, &st) == 0) {
+	  if (allow_existing && S_ISDIR(st.st_mode)) {
+	       return 0;
+	  }
 	  fprintf(stderr, "%s: Test area directory %s must not exist for test area creation.\n",
 		  __progname, path);
 	  return 1;
@@ -1921,6 +1926,7 @@ static int create_dir(char **ppath, const char *fmt, const char *dir)
 
 static int create_file(char **ppath, const char *fmt, const char *dir)
 {
+     int flags = O_WRONLY|O_CREAT | (allow_existing ? 0 : O_EXCL);
      const char *path;
      int fd;
 
@@ -1928,7 +1934,7 @@ static int create_file(char **ppath, const char *fmt, const char *dir)
 	  return -1;
      }
      path = *ppath;
-     if ((fd = open(path, O_WRONLY|O_CREAT|O_EXCL, 0666)) == -1) {
+     if ((fd = open(path, flags, 0666)) == -1) {
 	  fprintf(stderr, "%s: error creating file %s: %s\n", __progname, path, strerror(errno));
           return -1;
      }
@@ -1937,13 +1943,15 @@ static int create_file(char **ppath, const char *fmt, const char *dir)
 
 static int create_xattrs(int fd)
 {
-     if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
+     int flags = allow_existing ? 0 : XATTR_CREATE;
+
+     if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), flags) != 0) {
 	  if (errno != EOPNOTSUPP) {
 	       perror("setxattr");
 	       return 1;
 	  }
      }
-     if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
+     if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), flags) != 0) {
 	  if (errno != EOPNOTSUPP) {
 	       perror("setxattr");
 	       return 1;
@@ -2214,6 +2222,10 @@ static int remove_test_area(const char *dir)
 	  return 1;
      }
 
+     if (allow_existing) {
+	     return 0;
+     }
+
      pid = fork();
      if (!pid) {
 	  execl("/bin/rm", "rm", "-rf", dir, NULL);
@@ -2236,7 +2248,7 @@ int main(int argc, char **argv)
 /* this arg parsing is gross, but who cares, its a test program */
 
      if (argc < 2) {
-	  fprintf(stderr, "usage: t_immutable [-C|-c|-r] test_area_dir\n");
+	  fprintf(stderr, "usage: t_immutable [-C|-c|-R|-r] test_area_dir\n");
 	  return 1;
      }
 
@@ -2246,18 +2258,24 @@ int main(int argc, char **argv)
 	  /* Prepare test area without running tests */
 	  create = 1;
 	  runtest = 0;
+	  /* With existing test area, only setflags */
+	  allow_existing = 1;
      } else if (!strcmp(argv[1], "-r")) {
 	  remove = 1;
+     } else if (!strcmp(argv[1], "-R")) {
+	  /* Cleanup flags on test area but leave the files */
+	  remove = 1;
+	  allow_existing = 1;
      }
 
      if (argc != 2 + (create | remove)) {
-	  fprintf(stderr, "usage: t_immutable [-C|-c|-r] test_area_dir\n");
+	  fprintf(stderr, "usage: t_immutable [-C|-c|-R|-r] test_area_dir\n");
 	  return 1;
      }
 
      if (create) {
 	  ret = create_test_area(argv[argc-1]);
-	  if (ret || !runtest) {
+	  if (ret || allow_existing) {
                return ret;
 	  }
      } else if (remove) {
-- 
2.25.1

[PATCH 4/4] overlay: Test lost immutable/append-only flags on copy-up

[Amir Goldstein]

Chengguang Xu reported [1] that append-only flag is lost on copy-up.
I had noticed that for directories, immutable flag can also be lost on
copy up (when parent is copied up). That's an old overlayfs bug.

Overlayfs added the ability to set inode flags (e.g. chattr +i) in
kernel 5.10 by commit 61536bed2149 ("ovl: support [S|G]ETFLAGS and
FS[S|G]ETXATTR ioctls for directories").
Icenowy Zheng reported [2] a regression in that commit that causes
a deadlock when setting inode flags on lower dir.

There is a commented line in the test that triggers this deadlock,
but it has been left commented out until a fix is merged upstream.

[1] https://lore.kernel.org/linux-unionfs/20201226104618.239739-1-cgxu519@mykernel.net/
[2] https://lore.kernel.org/linux-unionfs/20210101201230.768653-1-icenowy@aosc.io/

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/overlay/075     | 97 +++++++++++++++++++++++++++++++++++++++++++
 tests/overlay/075.out | 11 +++++
 tests/overlay/group   |  1 +
 3 files changed, 109 insertions(+)
 create mode 100755 tests/overlay/075
 create mode 100644 tests/overlay/075.out

diff --git a/tests/overlay/075 b/tests/overlay/075
new file mode 100755
index 00000000..bcdc8d4e
--- /dev/null
+++ b/tests/overlay/075
@@ -0,0 +1,97 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2021 CTERA Networks. All Rights Reserved.
+#
+# FS QA Test No. 075
+#
+# Run the t_immutable test program for immutable/append-only files
+# and directories that exist in overlayfs lower layer.
+#
+# This test is similar and was derived from generic/079, but instead
+# of creating new files which are created in upper layer, prepare
+# the test area in lower layer before running the t_immutable test on
+# the overlayfs mount.
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+timmutable=$here/src/t_immutable
+lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
+upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	# -r will fail to remove test dirs, because we added subdirs
+	# we just need to remove the flags so use -R
+	$timmutable -R $upperdir/testdir &> /dev/null
+	$timmutable -R $lowerdir/testdir &> /dev/null
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+_supported_fs overlay
+
+_require_chattr iaA
+_require_test_program "t_immutable"
+_require_scratch
+
+_scratch_mkfs
+
+# Preparing test area files in lower dir and check chattr support of base fs
+mkdir -p $lowerdir
+mkdir -p $upperdir
+$timmutable -C $lowerdir/testdir >$tmp.out 2>&1
+if grep -q -e 'Operation not supported' -e "Inappropriate ioctl" $tmp.out; then
+	_notrun "Setting immutable/append flag not supported"
+fi
+# Remove the immutable/append-only flags and create subdirs
+$timmutable -R $lowerdir/testdir >$tmp.out 2>&1
+for dir in $lowerdir/testdir/*.d; do
+	mkdir $dir/subdir
+done
+# Restore the immutable/append-only flags
+$timmutable -C $lowerdir/testdir >$tmp.out 2>&1
+
+_scratch_mount
+
+# Test immutability of files in overlay
+echo "Before directories copy up"
+$timmutable $SCRATCH_MNT/testdir 2>&1
+
+# Trigger copy-up of immutable/append-only dirs by touching their subdirs
+# inode flags are not copied-up, so immutable/append-only flags are lost
+for dir in $SCRATCH_MNT/testdir/*.d; do
+	# chattr on dir fails (not supported) on kernel < 5.10.
+	# chattr on lower dir will deadlock on kernel 5.10 with commit 61536bed2149
+	# ("ovl: support [S|G]ETFLAGS and FS[S|G]ETXATTR ioctls for directories"),
+	# so this line is commented out until a fix is merged
+	# $CHATTR_PROG +A $dir/subdir > /dev/null 2>&1
+	touch $dir/subdir
+done
+
+# Trigger copy-up of append-only files by touching them
+# inode flags are not copied-up, so append-only flags are lost
+# touch on the immutable files is expected to fail, so immutable
+# flags will not be lost
+for file in $SCRATCH_MNT/testdir/*.f; do
+	touch $file > /dev/null 2>&1
+done
+
+# immutable/append-only flags still exist on the overlay in-core inode
+# After mount cycle, flags are forever lost
+_scratch_cycle_mount
+
+# Test immutability of files in overlay after directories copy-up
+echo "After directories copy up"
+$timmutable $SCRATCH_MNT/testdir 2>&1
+
+status=$?
+exit
diff --git a/tests/overlay/075.out b/tests/overlay/075.out
new file mode 100644
index 00000000..ab39c6b8
--- /dev/null
+++ b/tests/overlay/075.out
@@ -0,0 +1,11 @@
+QA output created by 075
+Before directories copy up
+testing immutable...PASS.
+testing append-only...PASS.
+testing immutable as non-root...PASS.
+testing append-only as non-root...PASS.
+After directories copy up
+testing immutable...PASS.
+testing append-only...PASS.
+testing immutable as non-root...PASS.
+testing append-only as non-root...PASS.
diff --git a/tests/overlay/group b/tests/overlay/group
index 047ea046..cfc75bb1 100644
--- a/tests/overlay/group
+++ b/tests/overlay/group
@@ -77,6 +77,7 @@
 072 auto quick copyup hardlink
 073 auto quick whiteout
 074 auto quick exportfs dangerous
+075 auto quick perms
 100 auto quick union samefs
 101 auto quick union nonsamefs
 102 auto quick union nonsamefs xino
-- 
2.25.1

Re: [PATCH 2/4] src/t_immutable: factor out some helpers

[Eryu Guan]

On Sat, Jan 16, 2021 at 06:56:17PM +0200, Amir Goldstein wrote:
> Reduce boilerplate code.
> define _GNU_SOURCE needed for asprintf.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  src/t_immutable.c | 221 ++++++++++++++++++++++------------------------
>  1 file changed, 104 insertions(+), 117 deletions(-)
> 
> diff --git a/src/t_immutable.c b/src/t_immutable.c
> index 86c567ed..b6a76af0 100644
> --- a/src/t_immutable.c
> +++ b/src/t_immutable.c
> @@ -8,6 +8,9 @@
>  
>  #define TEST_UTIME
>  
> +#ifndef _GNU_SOURCE
> +#define _GNU_SOURCE
> +#endif
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -1895,13 +1898,66 @@ static int check_test_area(const char *dir)
>       return 0;
>  }
>  
> +static int create_dir(char **ppath, const char *fmt, const char *dir)
> +{
> +     const char *path;
> +     struct stat st;
> +
> +     if (asprintf(ppath, fmt, dir) == -1) {
> +	  return -1;
> +     }
> +     path = *ppath;
> +     if (stat(path, &st) == 0) {
> +	  fprintf(stderr, "%s: Test area directory %s must not exist for test area creation.\n",
> +		  __progname, path);
> +	  return 1;

Other places return -1 but 1 is returned here, should be -1 as well?

Thanks,
Eryu

> +     }
> +     if (mkdir(path, 0777) != 0) {
> +	  fprintf(stderr, "%s: error creating directory %s: %s\n", __progname, path, strerror(errno));
> +	  return -1;
> +     }
> +     return 0;
> +}
> +
> +static int create_file(char **ppath, const char *fmt, const char *dir)
> +{
> +     const char *path;
> +     int fd;
> +
> +     if (asprintf(ppath, fmt, dir) == -1) {
> +	  return -1;
> +     }
> +     path = *ppath;
> +     if ((fd = open(path, O_WRONLY|O_CREAT|O_EXCL, 0666)) == -1) {
> +	  fprintf(stderr, "%s: error creating file %s: %s\n", __progname, path, strerror(errno));
> +          return -1;
> +     }
> +     return fd;
> +}
> +
> +static int create_xattrs(int fd)
> +{
> +     if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
> +	  if (errno != EOPNOTSUPP) {
> +	       perror("setxattr");
> +	       return 1;
> +	  }
> +     }
> +     if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
> +	  if (errno != EOPNOTSUPP) {
> +	       perror("setxattr");
> +	       return 1;
> +	  }
> +     }
> +     return 0;
> +}
> +
>  static int create_test_area(const char *dir)
>  {
>       int fd;
>       char *path;
>       static const char *acl_u_text = "u::rw-,g::rw-,o::rw-,u:nobody:rw-,m::rw-";
>       static const char *acl_u_text_d = "u::rwx,g::rwx,o::rwx,u:nobody:rwx,m::rwx";
> -     struct stat st;
>       static const char *immutable = "This is an immutable file.\nIts contents cannot be altered.\n";
>       static const char *append_only = "This is an append-only file.\nIts contents cannot be altered.\n"
>  	  "Data can only be appended.\n---\n";
> @@ -1911,79 +1967,45 @@ static int create_test_area(const char *dir)
>  	  return 1;
>       }
>  
> -     if (stat(dir, &st) == 0) {
> -	  fprintf(stderr, "%s: Test area directory %s must not exist for test area creation.\n",
> -		  __progname, dir);
> -	  return 1;
> -     }
> -
>       umask(0000);
> -     if (mkdir(dir, 0777) != 0) {
> -	  fprintf(stderr, "%s: error creating directory %s: %s\n", __progname, dir, strerror(errno));
> +     if (create_dir(&path, "%s", dir)) {
>  	  return 1;
>       }
> -
> -     asprintf(&path, "%s/immutable.d", dir);
> -     if (mkdir(path, 0777) != 0) {
> -          fprintf(stderr, "%s: error creating directory %s: %s\n", __progname, path, strerror(errno));
> -          return 1;
> -     }
>       free(path);
>  
> -     asprintf(&path, "%s/empty-immutable.d", dir);
> -     if (mkdir(path, 0777) != 0) {
> -          fprintf(stderr, "%s: error creating directory %s: %s\n", __progname, path, strerror(errno));
> -          return 1;
> +     if (create_dir(&path, "%s/append-only.d", dir)) {
> +	  return 1;
>       }
>       free(path);
>  
> -     asprintf(&path, "%s/append-only.d", dir);
> -     if (mkdir(path, 0777) != 0) {
> -          fprintf(stderr, "%s: error creating directory %s: %s\n", __progname, path, strerror(errno));
> -          return 1;
> +     if (create_dir(&path, "%s/append-only.d/dir", dir)) {
> +	  return 1;
>       }
>       free(path);
>  
> -     asprintf(&path, "%s/empty-append-only.d", dir);
> -     if (mkdir(path, 0777) != 0) {
> -          fprintf(stderr, "%s: error creating directory %s: %s\n", __progname, path, strerror(errno));
> +     if ((fd = create_file(&path, "%s/append-only.d/file", dir)) == -1) {
>            return 1;
>       }
> +     close(fd);
>       free(path);
>  
> -     asprintf(&path, "%s/immutable.d/dir", dir);
> -     if (mkdir(path, 0777) != 0) {
> -          fprintf(stderr, "%s: error creating directory %s: %s\n", __progname, path, strerror(errno));
> -          return 1;
> +     if (create_dir(&path, "%s/immutable.d", dir)) {
> +	  return 1;
>       }
>       free(path);
>  
> -     asprintf(&path, "%s/append-only.d/dir", dir);
> -     if (mkdir(path, 0777) != 0) {
> -          fprintf(stderr, "%s: error creating directory %s: %s\n", __progname, path, strerror(errno));
> -          return 1;
> +     if (create_dir(&path, "%s/immutable.d/dir", dir)) {
> +	  return 1;
>       }
>       free(path);
>  
> -     asprintf(&path, "%s/append-only.d/file", dir);
> -     if ((fd = open(path, O_WRONLY|O_CREAT|O_EXCL, 0666)) == -1) {
> -	  fprintf(stderr, "%s: error creating file %s: %s\n", __progname, path, strerror(errno));
> +     if ((fd = create_file(&path, "%s/immutable.d/file", dir)) == -1) {
>            return 1;
>       }
>       close(fd);
>       free(path);
>  
> -     asprintf(&path, "%s/immutable.d/file", dir);
> -     if ((fd = open(path, O_WRONLY|O_CREAT|O_EXCL, 0666)) == -1) {
> -          fprintf(stderr, "%s: error creating file %s: %s\n", __progname, path, strerror(errno));
> -          return 1;
> -     }
> -     close(fd);
> -     free(path);
> -
> -     asprintf(&path, "%s/immutable.f", dir);
> -     if ((fd = open(path, O_WRONLY|O_CREAT|O_EXCL, 0666)) == -1) {
> -          fprintf(stderr, "%s: error creating file %s: %s\n", __progname, path, strerror(errno));
> +     if ((fd = create_file(&path, "%s/immutable.f", dir)) == -1) {
>            return 1;
>       }
>       if (write(fd, immutable, strlen(immutable)) != strlen(immutable)) {
> @@ -1994,17 +2016,8 @@ static int create_test_area(const char *dir)
>  	  perror("acl");
>  	  return 1;
>       }
> -     if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
> -	  if (errno != EOPNOTSUPP) {
> -	       perror("setxattr");
> -	       return 1;
> -	  }
> -     }
> -     if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
> -	  if (errno != EOPNOTSUPP) {
> -	       perror("setxattr");
> -	       return 1;
> -	  }
> +     if (create_xattrs(fd)) {
> +	  return 1;
>       }
>       if (fsetflag(path, fd, 1, 1)) {
>            perror("fsetflag");
> @@ -2014,8 +2027,7 @@ static int create_test_area(const char *dir)
>       close(fd);
>       free(path);
>  
> -     asprintf(&path, "%s/append-only.f", dir);
> -     if ((fd = open(path, O_WRONLY|O_CREAT|O_EXCL, 0666)) == -1) {
> +     if ((fd = create_file(&path, "%s/append-only.f", dir)) == -1) {
>            fprintf(stderr, "%s: error creating file %s: %s\n", __progname, path, strerror(errno));
>            return 1;
>       }
> @@ -2027,17 +2039,8 @@ static int create_test_area(const char *dir)
>            perror("acl");
>            return 1;
>       }
> -     if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
> -	  if (errno != EOPNOTSUPP) {
> -	       perror("setxattr");
> -	       return 1;
> -	  }
> -     }
> -     if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
> -	  if (errno != EOPNOTSUPP) {
> -	       perror("setxattr");
> -	       return 1;
> -	  }
> +     if (create_xattrs(fd)) {
> +	  return 1;
>       }
>       if (fsetflag(path, fd, 1, 0)) {
>            perror("fsetflag");
> @@ -2056,17 +2059,8 @@ static int create_test_area(const char *dir)
>            perror("acl");
>            return 1;
>       }
> -     if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
> -	  if (errno != EOPNOTSUPP) {
> -	       perror("setxattr");
> -	       return 1;
> -	  }
> -     }
> -     if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
> -	  if (errno != EOPNOTSUPP) {
> -	       perror("setxattr");
> -	       return 1;
> -	  }
> +     if (create_xattrs(fd)) {
> +	  return 1;
>       }
>       if (fsetflag(path, fd, 1, 1)) {
>            perror("fsetflag");
> @@ -2076,7 +2070,9 @@ static int create_test_area(const char *dir)
>       close(fd);
>       free(path);
>  
> -     asprintf(&path, "%s/empty-immutable.d", dir);
> +     if (create_dir(&path, "%s/empty-immutable.d", dir)) {
> +	  return 1;
> +     }
>       if ((fd = open(path, O_RDONLY)) == -1) {
>            fprintf(stderr, "%s: error opening %s: %s\n", __progname, path, strerror(errno));
>            return 1;
> @@ -2098,17 +2094,8 @@ static int create_test_area(const char *dir)
>            perror("acl");
>            return 1;
>       }
> -     if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
> -	  if (errno != EOPNOTSUPP) {
> -	       perror("setxattr");
> -	       return 1;
> -	  }
> -     }
> -     if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
> -	  if (errno != EOPNOTSUPP) {
> -	       perror("setxattr");
> -	       return 1;
> -	  }
> +     if (create_xattrs(fd)) {
> +	  return 1;
>       }
>       if (fsetflag(path, fd, 1, 0)) {
>            perror("fsetflag");
> @@ -2118,7 +2105,9 @@ static int create_test_area(const char *dir)
>       close(fd);
>       free(path);
>  
> -     asprintf(&path, "%s/empty-append-only.d", dir);
> +     if (create_dir(&path, "%s/empty-append-only.d", dir)) {
> +	  return 1;
> +     }
>       if ((fd = open(path, O_RDONLY)) == -1) {
>            fprintf(stderr, "%s: error opening %s: %s\n", __progname, path, strerror(errno));
>            return 1;
> @@ -2242,6 +2231,7 @@ int main(int argc, char **argv)
>  {
>       int ret;
>       int failed = 0;
> +     int runtest = 1, create = 0, remove = 0;
>  
>  /* this arg parsing is gross, but who cares, its a test program */
>  
> @@ -2251,32 +2241,29 @@ int main(int argc, char **argv)
>       }
>  
>       if (!strcmp(argv[1], "-c")) {
> -	  if (argc == 3) {
> -	       if ((ret = create_test_area(argv[argc-1])))
> -		    return ret;
> -	  } else {
> -	       fprintf(stderr, "usage: t_immutable -c test_area_dir\n");
> -	       return 1;
> -	  }
> +	  create = 1;
>       } else if (!strcmp(argv[1], "-C")) {
> -          if (argc == 3) {
> -               return create_test_area(argv[argc-1]);
> -          } else {
> -               fprintf(stderr, "usage: t_immutable -C test_area_dir\n");
> -               return 1;
> -          }
> +	  /* Prepare test area without running tests */
> +	  create = 1;
> +	  runtest = 0;
>       } else if (!strcmp(argv[1], "-r")) {
> -	  if (argc == 3)
> -	       return remove_test_area(argv[argc-1]);
> -	  else {
> -	       fprintf(stderr, "usage: t_immutable -r test_area_dir\n");
> -	       return 1;
> -	  }
> -     } else if (argc != 2) {
> -	  fprintf(stderr, "usage: t_immutable [-c|-r] test_area_dir\n");
> +	  remove = 1;
> +     }
> +
> +     if (argc != 2 + (create | remove)) {
> +	  fprintf(stderr, "usage: t_immutable [-C|-c|-r] test_area_dir\n");
>  	  return 1;
>       }
>  
> +     if (create) {
> +	  ret = create_test_area(argv[argc-1]);
> +	  if (ret || !runtest) {
> +               return ret;
> +	  }
> +     } else if (remove) {
> +	  return remove_test_area(argv[argc-1]);
> +     }
> +
>       umask(0000);
>  
>       if (check_test_area(argv[argc-1]))
> -- 
> 2.25.1

Re: [PATCH 3/4] src/t_immutable: Allow setting flags on existing files

[Eryu Guan]

On Sat, Jan 16, 2021 at 06:56:18PM +0200, Amir Goldstein wrote:
> For overlayfs tests we need to be able to setflags on existing
> (lower) files.
> 
> t_immutable -C test_dir
> 
> Creates the test area and sets flags, but it also allows setting flags
> on an existing test area.
> 
> t_immutable -R test_dir
> 
> Removes the flags from existing test area, but does not remove the files
> in the test area.
> 
> To setup a test area with file without flags, need to run the -C and -R
> commands.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  src/t_immutable.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/src/t_immutable.c b/src/t_immutable.c
> index b6a76af0..a2e6796d 100644
> --- a/src/t_immutable.c
> +++ b/src/t_immutable.c
> @@ -1898,6 +1898,8 @@ static int check_test_area(const char *dir)
>       return 0;
>  }
>  
> +static int allow_existing;
> +
>  static int create_dir(char **ppath, const char *fmt, const char *dir)
>  {
>       const char *path;
> @@ -1908,6 +1910,9 @@ static int create_dir(char **ppath, const char *fmt, const char *dir)
>       }
>       path = *ppath;
>       if (stat(path, &st) == 0) {
> +	  if (allow_existing && S_ISDIR(st.st_mode)) {
> +	       return 0;
> +	  }
>  	  fprintf(stderr, "%s: Test area directory %s must not exist for test area creation.\n",
>  		  __progname, path);
>  	  return 1;
> @@ -1921,6 +1926,7 @@ static int create_dir(char **ppath, const char *fmt, const char *dir)
>  
>  static int create_file(char **ppath, const char *fmt, const char *dir)
>  {
> +     int flags = O_WRONLY|O_CREAT | (allow_existing ? 0 : O_EXCL);
>       const char *path;
>       int fd;
>  
> @@ -1928,7 +1934,7 @@ static int create_file(char **ppath, const char *fmt, const char *dir)
>  	  return -1;
>       }
>       path = *ppath;
> -     if ((fd = open(path, O_WRONLY|O_CREAT|O_EXCL, 0666)) == -1) {
> +     if ((fd = open(path, flags, 0666)) == -1) {
>  	  fprintf(stderr, "%s: error creating file %s: %s\n", __progname, path, strerror(errno));
>            return -1;
>       }
> @@ -1937,13 +1943,15 @@ static int create_file(char **ppath, const char *fmt, const char *dir)
>  
>  static int create_xattrs(int fd)
>  {
> -     if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
> +     int flags = allow_existing ? 0 : XATTR_CREATE;
> +
> +     if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), flags) != 0) {
>  	  if (errno != EOPNOTSUPP) {
>  	       perror("setxattr");
>  	       return 1;
>  	  }
>       }
> -     if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
> +     if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), flags) != 0) {
>  	  if (errno != EOPNOTSUPP) {
>  	       perror("setxattr");
>  	       return 1;
> @@ -2214,6 +2222,10 @@ static int remove_test_area(const char *dir)
>  	  return 1;
>       }
>  
> +     if (allow_existing) {
> +	     return 0;
> +     }
> +
>       pid = fork();
>       if (!pid) {
>  	  execl("/bin/rm", "rm", "-rf", dir, NULL);
> @@ -2236,7 +2248,7 @@ int main(int argc, char **argv)
>  /* this arg parsing is gross, but who cares, its a test program */
>  
>       if (argc < 2) {
> -	  fprintf(stderr, "usage: t_immutable [-C|-c|-r] test_area_dir\n");
> +	  fprintf(stderr, "usage: t_immutable [-C|-c|-R|-r] test_area_dir\n");
>  	  return 1;
>       }
>  
> @@ -2246,18 +2258,24 @@ int main(int argc, char **argv)
>  	  /* Prepare test area without running tests */
>  	  create = 1;
>  	  runtest = 0;
> +	  /* With existing test area, only setflags */
> +	  allow_existing = 1;
>       } else if (!strcmp(argv[1], "-r")) {
>  	  remove = 1;
> +     } else if (!strcmp(argv[1], "-R")) {
> +	  /* Cleanup flags on test area but leave the files */
> +	  remove = 1;
> +	  allow_existing = 1;
>       }
>  
>       if (argc != 2 + (create | remove)) {
> -	  fprintf(stderr, "usage: t_immutable [-C|-c|-r] test_area_dir\n");
> +	  fprintf(stderr, "usage: t_immutable [-C|-c|-R|-r] test_area_dir\n");
>  	  return 1;
>       }
>  
>       if (create) {
>  	  ret = create_test_area(argv[argc-1]);
> -	  if (ret || !runtest) {
> +	  if (ret || allow_existing) {

With this change, compiler warns about 'runtest' is set but not used,
and 'allow_existing' now indicates '!runtest' implicitly, which seems
subtle. I think it's better to keep 'runtest' as the indicator to
actually run the test?

Thanks,
Eryu

>                 return ret;
>  	  }
>       } else if (remove) {
> -- 
> 2.25.1

Re: [PATCH 2/4] src/t_immutable: factor out some helpers

[Amir Goldstein]

On Sun, Jan 24, 2021 at 5:09 PM Eryu Guan <guan@eryu.me> wrote:
>
> On Sat, Jan 16, 2021 at 06:56:17PM +0200, Amir Goldstein wrote:
> > Reduce boilerplate code.
> > define _GNU_SOURCE needed for asprintf.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  src/t_immutable.c | 221 ++++++++++++++++++++++------------------------
> >  1 file changed, 104 insertions(+), 117 deletions(-)
> >
> > diff --git a/src/t_immutable.c b/src/t_immutable.c
> > index 86c567ed..b6a76af0 100644
> > --- a/src/t_immutable.c
> > +++ b/src/t_immutable.c
> > @@ -8,6 +8,9 @@
> >
> >  #define TEST_UTIME
> >
> > +#ifndef _GNU_SOURCE
> > +#define _GNU_SOURCE
> > +#endif
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > @@ -1895,13 +1898,66 @@ static int check_test_area(const char *dir)
> >       return 0;
> >  }
> >
> > +static int create_dir(char **ppath, const char *fmt, const char *dir)
> > +{
> > +     const char *path;
> > +     struct stat st;
> > +
> > +     if (asprintf(ppath, fmt, dir) == -1) {
> > +       return -1;
> > +     }
> > +     path = *ppath;
> > +     if (stat(path, &st) == 0) {
> > +       fprintf(stderr, "%s: Test area directory %s must not exist for test area creation.\n",
> > +               __progname, path);
> > +       return 1;
>
> Other places return -1 but 1 is returned here, should be -1 as well?
>

It is a semantically different return value.

-1 are error cases, 1 means already existing, so the caller that requested to
create the dir could treat this as success.
I did not end up implementing the 'allow_existing' feature in this way, but I
see no reason to change the return value, because future implementation
could make use of this distinction. Unless you insist.

Thanks,
Amir.

Re: [PATCH 3/4] src/t_immutable: Allow setting flags on existing files

[Amir Goldstein]

On Sun, Jan 24, 2021 at 5:14 PM Eryu Guan <guan@eryu.me> wrote:
>
> On Sat, Jan 16, 2021 at 06:56:18PM +0200, Amir Goldstein wrote:
> > For overlayfs tests we need to be able to setflags on existing
> > (lower) files.
> >
> > t_immutable -C test_dir
> >
> > Creates the test area and sets flags, but it also allows setting flags
> > on an existing test area.
> >
> > t_immutable -R test_dir
> >
> > Removes the flags from existing test area, but does not remove the files
> > in the test area.
> >
> > To setup a test area with file without flags, need to run the -C and -R
> > commands.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  src/t_immutable.c | 30 ++++++++++++++++++++++++------
> >  1 file changed, 24 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/t_immutable.c b/src/t_immutable.c
> > index b6a76af0..a2e6796d 100644
> > --- a/src/t_immutable.c
> > +++ b/src/t_immutable.c
> > @@ -1898,6 +1898,8 @@ static int check_test_area(const char *dir)
> >       return 0;
> >  }
> >
> > +static int allow_existing;
> > +
> >  static int create_dir(char **ppath, const char *fmt, const char *dir)
> >  {
> >       const char *path;
> > @@ -1908,6 +1910,9 @@ static int create_dir(char **ppath, const char *fmt, const char *dir)
> >       }
> >       path = *ppath;
> >       if (stat(path, &st) == 0) {
> > +       if (allow_existing && S_ISDIR(st.st_mode)) {
> > +            return 0;
> > +       }
> >         fprintf(stderr, "%s: Test area directory %s must not exist for test area creation.\n",
> >                 __progname, path);
> >         return 1;
> > @@ -1921,6 +1926,7 @@ static int create_dir(char **ppath, const char *fmt, const char *dir)
> >
> >  static int create_file(char **ppath, const char *fmt, const char *dir)
> >  {
> > +     int flags = O_WRONLY|O_CREAT | (allow_existing ? 0 : O_EXCL);
> >       const char *path;
> >       int fd;
> >
> > @@ -1928,7 +1934,7 @@ static int create_file(char **ppath, const char *fmt, const char *dir)
> >         return -1;
> >       }
> >       path = *ppath;
> > -     if ((fd = open(path, O_WRONLY|O_CREAT|O_EXCL, 0666)) == -1) {
> > +     if ((fd = open(path, flags, 0666)) == -1) {
> >         fprintf(stderr, "%s: error creating file %s: %s\n", __progname, path, strerror(errno));
> >            return -1;
> >       }
> > @@ -1937,13 +1943,15 @@ static int create_file(char **ppath, const char *fmt, const char *dir)
> >
> >  static int create_xattrs(int fd)
> >  {
> > -     if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
> > +     int flags = allow_existing ? 0 : XATTR_CREATE;
> > +
> > +     if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), flags) != 0) {
> >         if (errno != EOPNOTSUPP) {
> >              perror("setxattr");
> >              return 1;
> >         }
> >       }
> > -     if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) {
> > +     if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), flags) != 0) {
> >         if (errno != EOPNOTSUPP) {
> >              perror("setxattr");
> >              return 1;
> > @@ -2214,6 +2222,10 @@ static int remove_test_area(const char *dir)
> >         return 1;
> >       }
> >
> > +     if (allow_existing) {
> > +          return 0;
> > +     }
> > +
> >       pid = fork();
> >       if (!pid) {
> >         execl("/bin/rm", "rm", "-rf", dir, NULL);
> > @@ -2236,7 +2248,7 @@ int main(int argc, char **argv)
> >  /* this arg parsing is gross, but who cares, its a test program */
> >
> >       if (argc < 2) {
> > -       fprintf(stderr, "usage: t_immutable [-C|-c|-r] test_area_dir\n");
> > +       fprintf(stderr, "usage: t_immutable [-C|-c|-R|-r] test_area_dir\n");
> >         return 1;
> >       }
> >
> > @@ -2246,18 +2258,24 @@ int main(int argc, char **argv)
> >         /* Prepare test area without running tests */
> >         create = 1;
> >         runtest = 0;
> > +       /* With existing test area, only setflags */
> > +       allow_existing = 1;
> >       } else if (!strcmp(argv[1], "-r")) {
> >         remove = 1;
> > +     } else if (!strcmp(argv[1], "-R")) {
> > +       /* Cleanup flags on test area but leave the files */
> > +       remove = 1;
> > +       allow_existing = 1;
> >       }
> >
> >       if (argc != 2 + (create | remove)) {
> > -       fprintf(stderr, "usage: t_immutable [-C|-c|-r] test_area_dir\n");
> > +       fprintf(stderr, "usage: t_immutable [-C|-c|-R|-r] test_area_dir\n");
> >         return 1;
> >       }
> >
> >       if (create) {
> >         ret = create_test_area(argv[argc-1]);
> > -       if (ret || !runtest) {
> > +       if (ret || allow_existing) {
>
> With this change, compiler warns about 'runtest' is set but not used,
> and 'allow_existing' now indicates '!runtest' implicitly, which seems
> subtle. I think it's better to keep 'runtest' as the indicator to
> actually run the test?
>

Sure, I removed it by mistake.

Thanks,
Amir.

Re: [PATCH 2/4] src/t_immutable: factor out some helpers

[Eryu Guan]

On Sun, Jan 24, 2021 at 05:29:33PM +0200, Amir Goldstein wrote:
> On Sun, Jan 24, 2021 at 5:09 PM Eryu Guan <guan@eryu.me> wrote:
> >
> > On Sat, Jan 16, 2021 at 06:56:17PM +0200, Amir Goldstein wrote:
> > > Reduce boilerplate code.
> > > define _GNU_SOURCE needed for asprintf.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >  src/t_immutable.c | 221 ++++++++++++++++++++++------------------------
> > >  1 file changed, 104 insertions(+), 117 deletions(-)
> > >
> > > diff --git a/src/t_immutable.c b/src/t_immutable.c
> > > index 86c567ed..b6a76af0 100644
> > > --- a/src/t_immutable.c
> > > +++ b/src/t_immutable.c
> > > @@ -8,6 +8,9 @@
> > >
> > >  #define TEST_UTIME
> > >
> > > +#ifndef _GNU_SOURCE
> > > +#define _GNU_SOURCE
> > > +#endif
> > >  #include <stdio.h>
> > >  #include <stdlib.h>
> > >  #include <string.h>
> > > @@ -1895,13 +1898,66 @@ static int check_test_area(const char *dir)
> > >       return 0;
> > >  }
> > >
> > > +static int create_dir(char **ppath, const char *fmt, const char *dir)
> > > +{
> > > +     const char *path;
> > > +     struct stat st;
> > > +
> > > +     if (asprintf(ppath, fmt, dir) == -1) {
> > > +       return -1;
> > > +     }
> > > +     path = *ppath;
> > > +     if (stat(path, &st) == 0) {
> > > +       fprintf(stderr, "%s: Test area directory %s must not exist for test area creation.\n",
> > > +               __progname, path);
> > > +       return 1;
> >
> > Other places return -1 but 1 is returned here, should be -1 as well?
> >
> 
> It is a semantically different return value.
> 
> -1 are error cases, 1 means already existing, so the caller that requested to
> create the dir could treat this as success.
> I did not end up implementing the 'allow_existing' feature in this way, but I
> see no reason to change the return value, because future implementation
> could make use of this distinction. Unless you insist.

I can live with it :) I'm just curious why the return values are
different, as I didn't see different values in original code.

Thanks,
Eryu

Re: [PATCH 3/4] src/t_immutable: Allow setting flags on existing files

[Eryu Guan]

On Sun, Jan 24, 2021 at 05:32:15PM +0200, Amir Goldstein wrote:
> On Sun, Jan 24, 2021 at 5:14 PM Eryu Guan <guan@eryu.me> wrote:
> >
[snap]
> > >
> > >       if (create) {
> > >         ret = create_test_area(argv[argc-1]);
> > > -       if (ret || !runtest) {
> > > +       if (ret || allow_existing) {
> >
> > With this change, compiler warns about 'runtest' is set but not used,
> > and 'allow_existing' now indicates '!runtest' implicitly, which seems
> > subtle. I think it's better to keep 'runtest' as the indicator to
> > actually run the test?
> >
> 
> Sure, I removed it by mistake.

Then this is the only place that needs update. I can fix it on commit,
no need to resend then.

Thanks,
Eryu

Re: [PATCH 3/4] src/t_immutable: Allow setting flags on existing files

[Amir Goldstein]

On Mon, Jan 25, 2021 at 2:46 PM Eryu Guan <eguan@linux.alibaba.com> wrote:
>
> On Sun, Jan 24, 2021 at 05:32:15PM +0200, Amir Goldstein wrote:
> > On Sun, Jan 24, 2021 at 5:14 PM Eryu Guan <guan@eryu.me> wrote:
> > >
> [snap]
> > > >
> > > >       if (create) {
> > > >         ret = create_test_area(argv[argc-1]);
> > > > -       if (ret || !runtest) {
> > > > +       if (ret || allow_existing) {
> > >
> > > With this change, compiler warns about 'runtest' is set but not used,
> > > and 'allow_existing' now indicates '!runtest' implicitly, which seems
> > > subtle. I think it's better to keep 'runtest' as the indicator to
> > > actually run the test?
> > >
> >
> > Sure, I removed it by mistake.
>
> Then this is the only place that needs update. I can fix it on commit,
> no need to resend then.
>

Excellent.

Now, about that kernel deadlock mentioned in is commented out line in
test overlay/075. The fix for that is in overlayfs-next:

147ec02b8705 - ovl: avoid deadlock on directory ioctl

But I am not so happy about adding a test that crashes stable/old kernels.
If you like I can post another test that is "dangerous" just for the deadlock
but after the fix is merged to master and 5.10.y so at least people who tests
the latest kernels will not crash.

Let me know your preference.

Thanks,
Amir.

Re: [PATCH 4/4] overlay: Test lost immutable/append-only flags on copy-up

[Amir Goldstein]

On Sat, Jan 16, 2021 at 6:56 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Chengguang Xu reported [1] that append-only flag is lost on copy-up.
> I had noticed that for directories, immutable flag can also be lost on
> copy up (when parent is copied up). That's an old overlayfs bug.
>
> Overlayfs added the ability to set inode flags (e.g. chattr +i) in
> kernel 5.10 by commit 61536bed2149 ("ovl: support [S|G]ETFLAGS and
> FS[S|G]ETXATTR ioctls for directories").
> Icenowy Zheng reported [2] a regression in that commit that causes
> a deadlock when setting inode flags on lower dir.
>
> There is a commented line in the test that triggers this deadlock,
> but it has been left commented out until a fix is merged upstream.
>

Re-iterate in correct thread:

The fix for above is in overlayfs-next:

* 147ec02b8705 - ovl: avoid deadlock on directory ioctl

But I wouldn't uncomment that line in the test just yet.

To be clear, this test does not pass in master and there is no clear
estimation on when the reported issues will be fixed.

Miklos,

Do you plan to followup on your VFS implementation for the
{s,g}etxflags methods?

Thanks,
Amir.

> [1] https://lore.kernel.org/linux-unionfs/20201226104618.239739-1-cgxu519@mykernel.net/
> [2] https://lore.kernel.org/linux-unionfs/20210101201230.768653-1-icenowy@aosc.io/
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  tests/overlay/075     | 97 +++++++++++++++++++++++++++++++++++++++++++
>  tests/overlay/075.out | 11 +++++
>  tests/overlay/group   |  1 +
>  3 files changed, 109 insertions(+)
>  create mode 100755 tests/overlay/075
>  create mode 100644 tests/overlay/075.out
>
> diff --git a/tests/overlay/075 b/tests/overlay/075
> new file mode 100755
> index 00000000..bcdc8d4e
> --- /dev/null
> +++ b/tests/overlay/075
> @@ -0,0 +1,97 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2021 CTERA Networks. All Rights Reserved.
> +#
> +# FS QA Test No. 075
> +#
> +# Run the t_immutable test program for immutable/append-only files
> +# and directories that exist in overlayfs lower layer.
> +#
> +# This test is similar and was derived from generic/079, but instead
> +# of creating new files which are created in upper layer, prepare
> +# the test area in lower layer before running the t_immutable test on
> +# the overlayfs mount.
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +timmutable=$here/src/t_immutable
> +lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
> +upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
> +tmp=/tmp/$$
> +status=1       # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +       # -r will fail to remove test dirs, because we added subdirs
> +       # we just need to remove the flags so use -R
> +       $timmutable -R $upperdir/testdir &> /dev/null
> +       $timmutable -R $lowerdir/testdir &> /dev/null
> +       rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +_supported_fs overlay
> +
> +_require_chattr iaA
> +_require_test_program "t_immutable"
> +_require_scratch
> +
> +_scratch_mkfs
> +
> +# Preparing test area files in lower dir and check chattr support of base fs
> +mkdir -p $lowerdir
> +mkdir -p $upperdir
> +$timmutable -C $lowerdir/testdir >$tmp.out 2>&1
> +if grep -q -e 'Operation not supported' -e "Inappropriate ioctl" $tmp.out; then
> +       _notrun "Setting immutable/append flag not supported"
> +fi
> +# Remove the immutable/append-only flags and create subdirs
> +$timmutable -R $lowerdir/testdir >$tmp.out 2>&1
> +for dir in $lowerdir/testdir/*.d; do
> +       mkdir $dir/subdir
> +done
> +# Restore the immutable/append-only flags
> +$timmutable -C $lowerdir/testdir >$tmp.out 2>&1
> +
> +_scratch_mount
> +
> +# Test immutability of files in overlay
> +echo "Before directories copy up"
> +$timmutable $SCRATCH_MNT/testdir 2>&1
> +
> +# Trigger copy-up of immutable/append-only dirs by touching their subdirs
> +# inode flags are not copied-up, so immutable/append-only flags are lost
> +for dir in $SCRATCH_MNT/testdir/*.d; do
> +       # chattr on dir fails (not supported) on kernel < 5.10.
> +       # chattr on lower dir will deadlock on kernel 5.10 with commit 61536bed2149
> +       # ("ovl: support [S|G]ETFLAGS and FS[S|G]ETXATTR ioctls for directories"),
> +       # so this line is commented out until a fix is merged
> +       # $CHATTR_PROG +A $dir/subdir > /dev/null 2>&1
> +       touch $dir/subdir
> +done
> +
> +# Trigger copy-up of append-only files by touching them
> +# inode flags are not copied-up, so append-only flags are lost
> +# touch on the immutable files is expected to fail, so immutable
> +# flags will not be lost
> +for file in $SCRATCH_MNT/testdir/*.f; do
> +       touch $file > /dev/null 2>&1
> +done
> +
> +# immutable/append-only flags still exist on the overlay in-core inode
> +# After mount cycle, flags are forever lost
> +_scratch_cycle_mount
> +
> +# Test immutability of files in overlay after directories copy-up
> +echo "After directories copy up"
> +$timmutable $SCRATCH_MNT/testdir 2>&1
> +
> +status=$?
> +exit
> diff --git a/tests/overlay/075.out b/tests/overlay/075.out
> new file mode 100644
> index 00000000..ab39c6b8
> --- /dev/null
> +++ b/tests/overlay/075.out
> @@ -0,0 +1,11 @@
> +QA output created by 075
> +Before directories copy up
> +testing immutable...PASS.
> +testing append-only...PASS.
> +testing immutable as non-root...PASS.
> +testing append-only as non-root...PASS.
> +After directories copy up
> +testing immutable...PASS.
> +testing append-only...PASS.
> +testing immutable as non-root...PASS.
> +testing append-only as non-root...PASS.
> diff --git a/tests/overlay/group b/tests/overlay/group
> index 047ea046..cfc75bb1 100644
> --- a/tests/overlay/group
> +++ b/tests/overlay/group
> @@ -77,6 +77,7 @@
>  072 auto quick copyup hardlink
>  073 auto quick whiteout
>  074 auto quick exportfs dangerous
> +075 auto quick perms
>  100 auto quick union samefs
>  101 auto quick union nonsamefs
>  102 auto quick union nonsamefs xino
> --
> 2.25.1
>

Re: [PATCH 4/4] overlay: Test lost immutable/append-only flags on copy-up

[Miklos Szeredi]

On Mon, Jan 25, 2021 at 2:24 PM Amir Goldstein <amir73il@gmail.com> wrote:

> Miklos,
>
> Do you plan to followup on your VFS implementation for the
> {s,g}etxflags methods?

Definitely.  Will try for 5.12, but no garantees.

Thanks,
Miklos

Re: [PATCH 4/4] overlay: Test lost immutable/append-only flags on copy-up

[Eryu Guan]

On Mon, Jan 25, 2021 at 03:24:07PM +0200, Amir Goldstein wrote:
> On Sat, Jan 16, 2021 at 6:56 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Chengguang Xu reported [1] that append-only flag is lost on copy-up.
> > I had noticed that for directories, immutable flag can also be lost on
> > copy up (when parent is copied up). That's an old overlayfs bug.
> >
> > Overlayfs added the ability to set inode flags (e.g. chattr +i) in
> > kernel 5.10 by commit 61536bed2149 ("ovl: support [S|G]ETFLAGS and
> > FS[S|G]ETXATTR ioctls for directories").
> > Icenowy Zheng reported [2] a regression in that commit that causes
> > a deadlock when setting inode flags on lower dir.
> >
> > There is a commented line in the test that triggers this deadlock,
> > but it has been left commented out until a fix is merged upstream.
> >
> 
> Re-iterate in correct thread:
> 
> The fix for above is in overlayfs-next:
> 
> * 147ec02b8705 - ovl: avoid deadlock on directory ioctl
> 
> But I wouldn't uncomment that line in the test just yet.

Then I'd prefer wait for the deadlock fix land in upstream first, and
merge the test with the deadlock trigger in place.

Or as you mentioned in previous thread, we could seperate the deadlock
case as a new test (also remove it from current overlay/075), so we
could merge the [s,g]etxflags case first, then the deadlock case only
when the fix is upstreamd.

Either way works for me, I just want to avoid merging the test without
the deadlock trigger, then uncomment it when the fix is available.

Thanks,
Eryu

> 
> To be clear, this test does not pass in master and there is no clear
> estimation on when the reported issues will be fixed.
> 
> Miklos,
> 
> Do you plan to followup on your VFS implementation for the
> {s,g}etxflags methods?
> 
> Thanks,
> Amir.
> 
> > [1] https://lore.kernel.org/linux-unionfs/20201226104618.239739-1-cgxu519@mykernel.net/
> > [2] https://lore.kernel.org/linux-unionfs/20210101201230.768653-1-icenowy@aosc.io/
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  tests/overlay/075     | 97 +++++++++++++++++++++++++++++++++++++++++++
> >  tests/overlay/075.out | 11 +++++
> >  tests/overlay/group   |  1 +
> >  3 files changed, 109 insertions(+)
> >  create mode 100755 tests/overlay/075
> >  create mode 100644 tests/overlay/075.out
> >
> > diff --git a/tests/overlay/075 b/tests/overlay/075
> > new file mode 100755
> > index 00000000..bcdc8d4e
> > --- /dev/null
> > +++ b/tests/overlay/075
> > @@ -0,0 +1,97 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (C) 2021 CTERA Networks. All Rights Reserved.
> > +#
> > +# FS QA Test No. 075
> > +#
> > +# Run the t_immutable test program for immutable/append-only files
> > +# and directories that exist in overlayfs lower layer.
> > +#
> > +# This test is similar and was derived from generic/079, but instead
> > +# of creating new files which are created in upper layer, prepare
> > +# the test area in lower layer before running the t_immutable test on
> > +# the overlayfs mount.
> > +#
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +timmutable=$here/src/t_immutable
> > +lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
> > +upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
> > +tmp=/tmp/$$
> > +status=1       # failure is the default!
> > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > +
> > +_cleanup()
> > +{
> > +       # -r will fail to remove test dirs, because we added subdirs
> > +       # we just need to remove the flags so use -R
> > +       $timmutable -R $upperdir/testdir &> /dev/null
> > +       $timmutable -R $lowerdir/testdir &> /dev/null
> > +       rm -f $tmp.*
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +
> > +_supported_fs overlay
> > +
> > +_require_chattr iaA
> > +_require_test_program "t_immutable"
> > +_require_scratch
> > +
> > +_scratch_mkfs
> > +
> > +# Preparing test area files in lower dir and check chattr support of base fs
> > +mkdir -p $lowerdir
> > +mkdir -p $upperdir
> > +$timmutable -C $lowerdir/testdir >$tmp.out 2>&1
> > +if grep -q -e 'Operation not supported' -e "Inappropriate ioctl" $tmp.out; then
> > +       _notrun "Setting immutable/append flag not supported"
> > +fi
> > +# Remove the immutable/append-only flags and create subdirs
> > +$timmutable -R $lowerdir/testdir >$tmp.out 2>&1
> > +for dir in $lowerdir/testdir/*.d; do
> > +       mkdir $dir/subdir
> > +done
> > +# Restore the immutable/append-only flags
> > +$timmutable -C $lowerdir/testdir >$tmp.out 2>&1
> > +
> > +_scratch_mount
> > +
> > +# Test immutability of files in overlay
> > +echo "Before directories copy up"
> > +$timmutable $SCRATCH_MNT/testdir 2>&1
> > +
> > +# Trigger copy-up of immutable/append-only dirs by touching their subdirs
> > +# inode flags are not copied-up, so immutable/append-only flags are lost
> > +for dir in $SCRATCH_MNT/testdir/*.d; do
> > +       # chattr on dir fails (not supported) on kernel < 5.10.
> > +       # chattr on lower dir will deadlock on kernel 5.10 with commit 61536bed2149
> > +       # ("ovl: support [S|G]ETFLAGS and FS[S|G]ETXATTR ioctls for directories"),
> > +       # so this line is commented out until a fix is merged
> > +       # $CHATTR_PROG +A $dir/subdir > /dev/null 2>&1
> > +       touch $dir/subdir
> > +done
> > +
> > +# Trigger copy-up of append-only files by touching them
> > +# inode flags are not copied-up, so append-only flags are lost
> > +# touch on the immutable files is expected to fail, so immutable
> > +# flags will not be lost
> > +for file in $SCRATCH_MNT/testdir/*.f; do
> > +       touch $file > /dev/null 2>&1
> > +done
> > +
> > +# immutable/append-only flags still exist on the overlay in-core inode
> > +# After mount cycle, flags are forever lost
> > +_scratch_cycle_mount
> > +
> > +# Test immutability of files in overlay after directories copy-up
> > +echo "After directories copy up"
> > +$timmutable $SCRATCH_MNT/testdir 2>&1
> > +
> > +status=$?
> > +exit
> > diff --git a/tests/overlay/075.out b/tests/overlay/075.out
> > new file mode 100644
> > index 00000000..ab39c6b8
> > --- /dev/null
> > +++ b/tests/overlay/075.out
> > @@ -0,0 +1,11 @@
> > +QA output created by 075
> > +Before directories copy up
> > +testing immutable...PASS.
> > +testing append-only...PASS.
> > +testing immutable as non-root...PASS.
> > +testing append-only as non-root...PASS.
> > +After directories copy up
> > +testing immutable...PASS.
> > +testing append-only...PASS.
> > +testing immutable as non-root...PASS.
> > +testing append-only as non-root...PASS.
> > diff --git a/tests/overlay/group b/tests/overlay/group
> > index 047ea046..cfc75bb1 100644
> > --- a/tests/overlay/group
> > +++ b/tests/overlay/group
> > @@ -77,6 +77,7 @@
> >  072 auto quick copyup hardlink
> >  073 auto quick whiteout
> >  074 auto quick exportfs dangerous
> > +075 auto quick perms
> >  100 auto quick union samefs
> >  101 auto quick union nonsamefs
> >  102 auto quick union nonsamefs xino
> > --
> > 2.25.1
> >

Re: [PATCH 4/4] overlay: Test lost immutable/append-only flags on copy-up

[Amir Goldstein]

On Wed, Jan 27, 2021 at 4:58 AM Eryu Guan <eguan@linux.alibaba.com> wrote:
>
> On Mon, Jan 25, 2021 at 03:24:07PM +0200, Amir Goldstein wrote:
> > On Sat, Jan 16, 2021 at 6:56 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > Chengguang Xu reported [1] that append-only flag is lost on copy-up.
> > > I had noticed that for directories, immutable flag can also be lost on
> > > copy up (when parent is copied up). That's an old overlayfs bug.
> > >
> > > Overlayfs added the ability to set inode flags (e.g. chattr +i) in
> > > kernel 5.10 by commit 61536bed2149 ("ovl: support [S|G]ETFLAGS and
> > > FS[S|G]ETXATTR ioctls for directories").
> > > Icenowy Zheng reported [2] a regression in that commit that causes
> > > a deadlock when setting inode flags on lower dir.
> > >
> > > There is a commented line in the test that triggers this deadlock,
> > > but it has been left commented out until a fix is merged upstream.
> > >
> >
> > Re-iterate in correct thread:
> >
> > The fix for above is in overlayfs-next:
> >
> > * 147ec02b8705 - ovl: avoid deadlock on directory ioctl
> >
> > But I wouldn't uncomment that line in the test just yet.
>
> Then I'd prefer wait for the deadlock fix land in upstream first, and
> merge the test with the deadlock trigger in place.
>
> Or as you mentioned in previous thread, we could seperate the deadlock
> case as a new test (also remove it from current overlay/075), so we
> could merge the [s,g]etxflags case first, then the deadlock case only
> when the fix is upstreamd.
>
> Either way works for me, I just want to avoid merging the test without
> the deadlock trigger, then uncomment it when the fix is available.
>

So let's wait for the deadlock fix to land and I will re-post with split
tests and address your minor comment.

Thanks,
Amir.