[PATCH v4 0/3] Restore the kernel v5.13 text attribute write behavior

[PATCH v4 0/3] Restore the kernel v5.13 text attribute write behavior

[Bart Van Assche]

Hi Christoph,

This patch series restores the v5.13 text attribute write behavior and also
adds unit tests for configfs. Please consider these patches for inclusion in
the Linux kernel.

Thanks,

Bart.

Changes compared to v3:
- Changed config CONFIGFS_KUNIT_TEST from tristate into bool. The unit test
  calls do_mount(). do_mount() has not been exported and hence is not available
  to kernel modules. This was detected by the kernel build robot.

Changes compared to v2:
- Modified description of patch 2/3.

Changes compared to v1:
- Instead of making the text attribute write behavior POSIX compliant, restore
  the v5.13 behavior.
- Added more unit tests.

Bart Van Assche (3):
  configfs: Restore the kernel v5.13 text attribute write behavior
  kunit: Add support for suite initialization and cleanup
  configfs: Add unit tests

 fs/configfs/Kconfig         |   8 +
 fs/configfs/Makefile        |   2 +
 fs/configfs/configfs-test.c | 429 ++++++++++++++++++++++++++++++++++++
 fs/configfs/file.c          |  18 +-
 include/kunit/test.h        |   4 +
 lib/kunit/test.c            |  14 ++
 6 files changed, 463 insertions(+), 12 deletions(-)
 create mode 100644 fs/configfs/configfs-test.c

[PATCH v4 1/3] configfs: Restore the kernel v5.13 text attribute write behavior

[Bart Van Assche]

Instead of appending new text attribute data at the offset specified by the
write() system call, only pass the newly written data to the .store()
callback.

Cc: Bodo Stroesser <bostroesser@gmail.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Yanko Kaneti <yaneti@declera.com>
Cc: Brendan Higgins <brendanhiggins@google.com>
Reported-by: Bodo Stroesser <bostroesser@gmail.com>
Tested-by: Bodo Stroesser <bostroesser@gmail.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 fs/configfs/file.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/fs/configfs/file.c b/fs/configfs/file.c
index 5a0be9985bae..0ad32150611e 100644
--- a/fs/configfs/file.c
+++ b/fs/configfs/file.c
@@ -177,28 +177,22 @@ static ssize_t configfs_bin_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	return retval;
 }
 
-/* Fill [buffer, buffer + pos) with data coming from @from. */
-static int fill_write_buffer(struct configfs_buffer *buffer, loff_t pos,
+/* Fill @buffer with data coming from @from. */
+static int fill_write_buffer(struct configfs_buffer *buffer,
 			     struct iov_iter *from)
 {
-	loff_t to_copy;
 	int copied;
-	u8 *to;
 
 	if (!buffer->page)
 		buffer->page = (char *)__get_free_pages(GFP_KERNEL, 0);
 	if (!buffer->page)
 		return -ENOMEM;
 
-	to_copy = SIMPLE_ATTR_SIZE - 1 - pos;
-	if (to_copy <= 0)
-		return 0;
-	to = buffer->page + pos;
-	copied = copy_from_iter(to, to_copy, from);
+	copied = copy_from_iter(buffer->page, SIMPLE_ATTR_SIZE - 1, from);
 	buffer->needs_read_fill = 1;
 	/* if buf is assumed to contain a string, terminate it by \0,
 	 * so e.g. sscanf() can scan the string easily */
-	to[copied] = 0;
+	buffer->page[copied] = 0;
 	return copied ? : -EFAULT;
 }
 
@@ -227,10 +221,10 @@ static ssize_t configfs_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
 	struct file *file = iocb->ki_filp;
 	struct configfs_buffer *buffer = file->private_data;
-	ssize_t len;
+	int len;
 
 	mutex_lock(&buffer->mutex);
-	len = fill_write_buffer(buffer, iocb->ki_pos, from);
+	len = fill_write_buffer(buffer, from);
 	if (len > 0)
 		len = flush_write_buffer(file, buffer, len);
 	if (len > 0)

[PATCH v4 2/3] kunit: Add support for suite initialization and cleanup

[Bart Van Assche]

A common feature of unit testing frameworks is support for sharing a test
configuration across multiple unit tests. Add this functionality to the
KUnit framework. This functionality will be used in the next patch in this
series.

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
Cc: David Gow <davidgow@google.com>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: kunit-dev@googlegroups.com
Cc: linux-kselftest@vger.kernel.org
Cc: Bodo Stroesser <bostroesser@gmail.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Yanko Kaneti <yaneti@declera.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/kunit/test.h |  4 ++++
 lib/kunit/test.c     | 14 ++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 24b40e5c160b..a6eef96a409c 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -215,6 +215,8 @@ static inline char *kunit_status_to_ok_not_ok(enum kunit_status status)
  * struct kunit_suite - describes a related collection of &struct kunit_case
  *
  * @name:	the name of the test. Purely informational.
+ * @init_suite:	called once per test suite before the test cases.
+ * @exit_suite:	called once per test suite after all test cases.
  * @init:	called before every test case.
  * @exit:	called after every test case.
  * @test_cases:	a null terminated array of test cases.
@@ -229,6 +231,8 @@ static inline char *kunit_status_to_ok_not_ok(enum kunit_status status)
  */
 struct kunit_suite {
 	const char name[256];
+	int (*init_suite)(void);
+	void (*exit_suite)(void);
 	int (*init)(struct kunit *test);
 	void (*exit)(struct kunit *test);
 	struct kunit_case *test_cases;
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index d79ecb86ea57..c271692ced93 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -397,9 +397,19 @@ int kunit_run_tests(struct kunit_suite *suite)
 {
 	char param_desc[KUNIT_PARAM_DESC_SIZE];
 	struct kunit_case *test_case;
+	int res = 0;
 
 	kunit_print_subtest_start(suite);
 
+	if (suite->init_suite)
+		res = suite->init_suite();
+
+	if (res < 0) {
+		kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT
+			  "# Suite initialization failed (%d)\n", res);
+		goto end;
+	}
+
 	kunit_suite_for_each_test_case(suite, test_case) {
 		struct kunit test = { .param_value = NULL, .param_index = 0 };
 		test_case->status = KUNIT_SKIPPED;
@@ -439,6 +449,10 @@ int kunit_run_tests(struct kunit_suite *suite)
 				      test.status_comment);
 	}
 
+	if (suite->exit_suite)
+		suite->exit_suite();
+
+end:
 	kunit_print_subtest_end(suite);
 
 	return 0;

[PATCH v4 3/3] configfs: Add unit tests

[Bart Van Assche]

Testing configfs read and write behavior is non-trivial and tedious.
Hence these configfs kunit tests that make it easier to test configfs
text and binary attribute support. This is how I run these tests:

set -e
if [ -e .config ]; then
    make ARCH=um mrproper
fi
if [ ! -e .kunit/.kunitconfig ]; then
    cat <<EOF >.kunit/.kunitconfig
CONFIG_CONFIGFS_FS=y
CONFIG_CONFIGFS_KUNIT_TEST=y
CONFIG_KUNIT=y
CONFIG_PROVE_LOCKING=y
CONFIG_SYSFS=y
CONFIG_UBSAN=y
EOF
    cp .kunit/.kunitconfig .kunit/.config
fi
./tools/testing/kunit/kunit.py run

Cc: Bodo Stroesser <bostroesser@gmail.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Yanko Kaneti <yaneti@declera.com>
Cc: Brendan Higgins <brendanhiggins@google.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 fs/configfs/Kconfig         |   8 +
 fs/configfs/Makefile        |   2 +
 fs/configfs/configfs-test.c | 429 ++++++++++++++++++++++++++++++++++++
 3 files changed, 439 insertions(+)
 create mode 100644 fs/configfs/configfs-test.c

diff --git a/fs/configfs/Kconfig b/fs/configfs/Kconfig
index 272b64456999..bbd7867407df 100644
--- a/fs/configfs/Kconfig
+++ b/fs/configfs/Kconfig
@@ -10,3 +10,11 @@ config CONFIGFS_FS
 
 	  Both sysfs and configfs can and should exist together on the
 	  same system. One is not a replacement for the other.
+
+config CONFIGFS_KUNIT_TEST
+	bool "Configfs Kunit test" if !KUNIT_ALL_TESTS
+	depends on CONFIGFS_FS && KUNIT=y
+	default KUNIT_ALL_TESTS
+	help
+	  Run the configfs unit tests at boot time. For more information, see
+	  also the Kunit documentation in Documentation/dev-tools/kunit/.
diff --git a/fs/configfs/Makefile b/fs/configfs/Makefile
index 0200498ede27..388003fa9f37 100644
--- a/fs/configfs/Makefile
+++ b/fs/configfs/Makefile
@@ -6,3 +6,5 @@
 obj-$(CONFIG_CONFIGFS_FS)	+= configfs.o
 
 configfs-objs	:= inode.o file.o dir.o symlink.o mount.o item.o
+
+obj-$(CONFIG_CONFIGFS_KUNIT_TEST) += configfs-test.o
diff --git a/fs/configfs/configfs-test.c b/fs/configfs/configfs-test.c
new file mode 100644
index 000000000000..46632d054fc8
--- /dev/null
+++ b/fs/configfs/configfs-test.c
@@ -0,0 +1,429 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <kunit/test.h>
+#include <linux/configfs.h>
+#include <linux/fs.h>
+#include <linux/namei.h>
+#include <linux/uio.h>
+
+/*
+ * Maximum number of bytes supported by the configfs attributes in this unit
+ * test.
+ */
+enum { ATTR_MAX_SIZE = 256 };
+
+static struct test_item {
+	uint32_t nbytes;
+	char data[ATTR_MAX_SIZE];
+} bin_attr, text_attr;
+
+static ssize_t attr_read(struct test_item *ti, void *buf, size_t len)
+{
+	size_t nbytes = min_t(size_t, len, ti->nbytes);
+
+	memcpy(buf, ti->data, nbytes);
+	return nbytes;
+}
+
+static ssize_t attr_write(struct test_item *ti, const void *buf, size_t len)
+{
+	if (len > ATTR_MAX_SIZE)
+		return -EINVAL;
+	ti->nbytes = len;
+	memcpy(ti->data, buf, len);
+	return len;
+}
+
+static DEFINE_SEMAPHORE(bin_attr_written);
+
+static ssize_t bin_attr_read(struct config_item *item, void *buf, size_t len)
+{
+	return buf ? attr_read(&bin_attr, buf, len) : bin_attr.nbytes;
+}
+
+static ssize_t bin_attr_write(struct config_item *item, const void *buf,
+			      size_t len)
+{
+	up(&bin_attr_written);
+	return attr_write(&bin_attr, buf, len);
+}
+
+CONFIGFS_BIN_ATTR(, bin_attr, NULL, ATTR_MAX_SIZE);
+
+static struct configfs_bin_attribute *bin_attrs[] = {
+	&attr_bin_attr,
+	NULL,
+};
+
+static ssize_t text_attr_show(struct config_item *item, char *buf)
+{
+	return attr_read(&text_attr, buf, strlen(buf));
+}
+
+static ssize_t text_attr_store(struct config_item *item, const char *buf,
+			       size_t size)
+{
+	return attr_write(&text_attr, buf, size);
+}
+
+CONFIGFS_ATTR(, text_attr);
+
+static struct configfs_attribute *text_attrs[] = {
+	&attr_text_attr,
+	NULL,
+};
+
+static const struct config_item_type test_configfs_type = {
+	.ct_owner	= THIS_MODULE,
+	.ct_bin_attrs	= bin_attrs,
+	.ct_attrs	= text_attrs,
+};
+
+/*
+ * Return the file mode if @path exists or an error code if opening @path via
+ * filp_open() in read-only mode failed.
+ */
+int get_file_mode(const char *path)
+{
+	struct file *file;
+	int res;
+
+	file = filp_open(path, O_RDONLY, 0400);
+	if (IS_ERR(file)) {
+		res = PTR_ERR(file);
+		goto out;
+	}
+	res = file_inode(file)->i_mode;
+	filp_close(file, NULL);
+
+out:
+	return res;
+}
+
+static int mkdir(const char *name, umode_t mode)
+{
+	struct dentry *dentry;
+	struct path path;
+	int err;
+
+	err = get_file_mode(name);
+	if (err >= 0 && S_ISDIR(err))
+		return 0;
+
+	dentry = kern_path_create(AT_FDCWD, name, &path, LOOKUP_DIRECTORY);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
+
+	err = vfs_mkdir(&init_user_ns, d_inode(path.dentry), dentry, mode);
+	done_path_create(&path, dentry);
+
+	return err;
+}
+
+static int mount_configfs(void)
+{
+	int res;
+
+	res = get_file_mode("/sys/kernel/config/unit-test");
+	if (res >= 0)
+		return 0;
+	res = mkdir("/sys", 0755);
+	if (res < 0)
+		return res;
+	res = mkdir("/sys/kernel", 0755);
+	if (res < 0)
+		return res;
+	res = mkdir("/sys/kernel/config", 0755);
+	if (res < 0)
+		return res;
+	pr_info("mounting configfs ...\n");
+	res = do_mount("", "/sys/kernel/config", "configfs", 0, NULL);
+	if (res < 0)
+		pr_err("mounting configfs failed: %d\n", res);
+	else
+		pr_info("mounted configfs.\n");
+	return res;
+}
+
+static void unmount_configfs(void)
+{
+	/* How to unmount a filesystem from kernel code? */
+}
+
+#define KUNIT_EXPECT_MODE(test, left_arg, mask, right)			\
+({									\
+	const int left = (left_arg);					\
+									\
+	KUNIT_EXPECT_TRUE_MSG(test, left >= 0 && (left & mask) == right, \
+		"(" #left_arg "(%d) & " #mask ") != " #right, left);	\
+})
+
+static void configfs_mounted(struct kunit *test)
+{
+	KUNIT_EXPECT_MODE(test, get_file_mode("/"), 0500, 0500);
+	KUNIT_EXPECT_MODE(test, get_file_mode("/sys"), 0500, 0500);
+	KUNIT_EXPECT_MODE(test, get_file_mode("/sys/kernel"), 0500, 0500);
+	KUNIT_EXPECT_MODE(test, get_file_mode("/sys/kernel/config"), 0500, 0500);
+	KUNIT_EXPECT_MODE(test, get_file_mode("/sys/kernel/config/unit-test"),
+			  0500, 0500);
+	KUNIT_EXPECT_MODE(test, get_file_mode
+			  ("/sys/kernel/config/unit-test/text_attr"),
+			  0700, 0600);
+}
+
+static void configfs_text_attr(struct kunit *test)
+{
+	struct file *f = filp_open("/sys/kernel/config/unit-test/text_attr",
+				   O_RDWR, 0);
+	static const char text1[] =
+		"The quick brown fox jumps over the lazy dog";
+	const int off1 = 0;
+	const int len1 = strlen(text1);
+	static const char text2[] = "huge";
+	const int off2 = strlen(text1) - strlen(text2) - 4;
+	const int len2 = strlen(text2);
+	char text3[sizeof(text1)];
+	int res;
+	loff_t pos;
+
+	KUNIT_EXPECT_EQ(test, PTR_ERR_OR_ZERO(f), 0);
+	if (IS_ERR(f))
+		return;
+	/* Write at a non-zero offset. */
+	pos = off2;
+	res = kernel_write(f, text2, len2, &pos);
+	KUNIT_EXPECT_EQ(test, res, len2);
+	KUNIT_EXPECT_EQ(test, pos, off2 + len2);
+	/* Verify the effect of the above kernel_write() call. */
+	pos = 0;
+	res = kernel_read(f, text3, sizeof(text3), &pos);
+	KUNIT_EXPECT_EQ(test, res, len2);
+	KUNIT_EXPECT_EQ(test, pos, len2);
+	if (res >= 0) {
+		text3[res] = '\0';
+		KUNIT_EXPECT_STREQ(test, text3, text2);
+	}
+	/* Write at offset zero. */
+	pos = off1;
+	res = kernel_write(f, text1, len1, &pos);
+	KUNIT_EXPECT_EQ(test, res, len1);
+	KUNIT_EXPECT_EQ(test, pos, len1);
+	/* Verify the effect of the above kernel_write() call. */
+	pos = 0;
+	res = kernel_read(f, text3, sizeof(text3), &pos);
+	KUNIT_EXPECT_EQ(test, res, len1);
+	KUNIT_EXPECT_EQ(test, pos, len1);
+	if (res >= 0) {
+		text3[res] = '\0';
+		KUNIT_EXPECT_STREQ(test, text3, text1);
+	}
+	/* Write at a non-zero offset. */
+	pos = off2;
+	res = kernel_write(f, text2, len2, &pos);
+	KUNIT_EXPECT_EQ(test, res, len2);
+	KUNIT_EXPECT_EQ(test, pos, off2 + len2);
+	/* Verify that the above kernel_write() call truncated the attribute. */
+	pos = 0;
+	res = kernel_read(f, text3, sizeof(text3), &pos);
+	KUNIT_EXPECT_EQ(test, res, len2);
+	KUNIT_EXPECT_EQ(test, pos, len2);
+	if (res >= 0) {
+		text3[res] = '\0';
+		KUNIT_EXPECT_STREQ(test, text3, text2);
+	}
+	/* Read from offset 1. */
+	pos = 1;
+	res = kernel_read(f, text3, sizeof(text3), &pos);
+	KUNIT_EXPECT_EQ(test, res, len2 - 1);
+	KUNIT_EXPECT_EQ(test, pos, len2);
+	if (res >= 0) {
+		text3[res] = '\0';
+		KUNIT_EXPECT_STREQ(test, text3, text2 + 1);
+	}
+	/* Write at offset -1. */
+	pos = -1;
+	res = kernel_write(f, text1, len1, &pos);
+	KUNIT_EXPECT_EQ(test, res, -EINVAL);
+	/* Write at the largest possible positive offset. */
+	pos = LLONG_MAX - len1;
+	res = kernel_write(f, text1, len1, &pos);
+	KUNIT_EXPECT_EQ(test, res, len1);
+	/* Read from offset -1. */
+	pos = -1;
+	res = kernel_read(f, text3, sizeof(text3), &pos);
+	KUNIT_EXPECT_EQ(test, res, -EINVAL);
+	/* Read from the largest possible positive offset. */
+	pos = LLONG_MAX - sizeof(text3);
+	res = kernel_read(f, text3, sizeof(text3), &pos);
+	KUNIT_EXPECT_EQ(test, res, 0);
+	/* Verify the effect of the latest kernel_write() call. */
+	pos = 0;
+	res = kernel_read(f, text3, sizeof(text3), &pos);
+	KUNIT_EXPECT_EQ(test, res, len1);
+	KUNIT_EXPECT_EQ(test, pos, len1);
+	if (res >= 0) {
+		text3[res] = '\0';
+		KUNIT_EXPECT_STREQ(test, text3, text1);
+	}
+	filp_close(f, NULL);
+}
+
+#define KUNIT_EXPECT_MEMEQ(test, left, right, len)			\
+	KUNIT_EXPECT_TRUE_MSG(test, memcmp(left, right, len) == 0,	\
+			      #left " != " #right ": %.*s <> %.*s",	\
+			      (int)len, left, (int)len, right)
+
+static void configfs_bin_attr(struct kunit *test)
+{
+	struct file *f = filp_open("/sys/kernel/config/unit-test/bin_attr",
+				   O_RDWR, 0);
+	static const u8 data1[] =
+		"\xff\x00The quick brown fox jumps over the lazy dog";
+	const int off1 = 0;
+	const int len1 = sizeof(data1) - 1;
+	static const u8 data2[] = "huge";
+	const int off2 = len1 - strlen(data2) - 4;
+	const int len2 = strlen(data2);
+	u8 data3[sizeof(data1)];
+	int res;
+	loff_t pos;
+
+	bin_attr.nbytes = len1;
+
+	KUNIT_EXPECT_EQ(test, PTR_ERR_OR_ZERO(f), 0);
+	if (IS_ERR(f))
+		return;
+	/* Write at offset zero. */
+	pos = off1;
+	res = kernel_write(f, data1, len1, &pos);
+	KUNIT_EXPECT_EQ(test, res, len1);
+	KUNIT_EXPECT_EQ(test, pos, off1 + len1);
+	/* Write at a non-zero offset. */
+	pos = off2;
+	res = kernel_write(f, data2, len2, &pos);
+	KUNIT_EXPECT_EQ(test, res, len2);
+	KUNIT_EXPECT_EQ(test, pos, off2 + len2);
+	filp_close(f, NULL);
+
+	/*
+	 * buffer->bin_attr->write() is called from inside
+	 * configfs_release_bin_file() and the latter function is
+	 * called asynchronously. Hence the down() calls below to wait
+	 * until the write method has been called.
+	 */
+	down(&bin_attr_written);
+	down(&bin_attr_written);
+
+	f = filp_open("/sys/kernel/config/unit-test/bin_attr", O_RDONLY, 0);
+	KUNIT_EXPECT_EQ(test, PTR_ERR_OR_ZERO(f), 0);
+	if (IS_ERR(f))
+		return;
+	/* Verify the effect of the two kernel_write() calls. */
+	pos = 0;
+	res = kernel_read(f, data3, sizeof(data3), &pos);
+	KUNIT_EXPECT_EQ(test, res, len1);
+	KUNIT_EXPECT_EQ(test, pos, len1);
+	if (res >= 0) {
+		data3[res] = '\0';
+		KUNIT_EXPECT_MEMEQ(test, data3,
+			"\xff\x00The quick brown fox jumps over the huge dog",
+			len1);
+	}
+	/* Read from offset 1. */
+	pos = 1;
+	res = kernel_read(f, data3, sizeof(data3), &pos);
+	KUNIT_EXPECT_EQ(test, res, len1 - 1);
+	KUNIT_EXPECT_EQ(test, pos, len1);
+	if (res >= 0) {
+		data3[res] = '\0';
+		KUNIT_EXPECT_MEMEQ(test, data3,
+			"\x00The quick brown fox jumps over the huge dog",
+			len1 - 1);
+	}
+	filp_close(f, NULL);
+
+	f = filp_open("/sys/kernel/config/unit-test/bin_attr", O_RDWR, 0);
+	KUNIT_EXPECT_EQ(test, PTR_ERR_OR_ZERO(f), 0);
+	if (IS_ERR(f))
+		return;
+	/* Write at offset -1. */
+	pos = -1;
+	res = kernel_write(f, data1, len1, &pos);
+	KUNIT_EXPECT_EQ(test, res, -EINVAL);
+	/* Write at the largest possible positive offset. */
+	pos = LLONG_MAX - len1;
+	res = kernel_write(f, data1, len1, &pos);
+	KUNIT_EXPECT_EQ(test, res, -EFBIG);
+	filp_close(f, NULL);
+
+	/* Wait until the .write() function has been called. */
+	down(&bin_attr_written);
+
+	KUNIT_EXPECT_EQ(test, bin_attr.nbytes, 0);
+
+	f = filp_open("/sys/kernel/config/unit-test/bin_attr", O_RDONLY, 0);
+	KUNIT_EXPECT_EQ(test, PTR_ERR_OR_ZERO(f), 0);
+	if (IS_ERR(f))
+		return;
+	/* Read from offset -1. */
+	pos = -1;
+	res = kernel_read(f, data3, sizeof(data3), &pos);
+	KUNIT_EXPECT_EQ(test, res, -EINVAL);
+	/* Read from the largest possible positive offset. */
+	pos = LLONG_MAX - sizeof(data3);
+	res = kernel_read(f, data3, sizeof(data3), &pos);
+	KUNIT_EXPECT_EQ(test, res, 0);
+	KUNIT_EXPECT_EQ(test, pos, LLONG_MAX - sizeof(data3));
+	/* Read from offset zero. */
+	pos = 0;
+	res = kernel_read(f, data3, sizeof(data3), &pos);
+	KUNIT_EXPECT_EQ(test, res, 0);
+	KUNIT_EXPECT_EQ(test, pos, 0);
+	filp_close(f, NULL);
+}
+
+static struct kunit_case configfs_test_cases[] = {
+	KUNIT_CASE(configfs_mounted),
+	KUNIT_CASE(configfs_text_attr),
+	KUNIT_CASE(configfs_bin_attr),
+	{},
+};
+
+static struct configfs_subsystem test_subsys = {
+	.su_group = {
+		.cg_item = {
+			.ci_namebuf = "unit-test",
+			.ci_type    = &test_configfs_type,
+		}
+	},
+};
+
+static int configfs_suite_init(void)
+{
+	int res;
+
+	config_group_init(&test_subsys.su_group);
+	mutex_init(&test_subsys.su_mutex);
+	res = configfs_register_subsystem(&test_subsys);
+	if (res < 0) {
+		pr_err("Registration of configfs subsystem failed: %d\n", res);
+		return res;
+	}
+	return mount_configfs();
+}
+
+static void configfs_suite_exit(void)
+{
+	configfs_unregister_subsystem(&test_subsys);
+	unmount_configfs();
+}
+
+static struct kunit_suite configfs_test_module = {
+	.name		= "configfs unit tests",
+	.init_suite	= configfs_suite_init,
+	.exit_suite	= configfs_suite_exit,
+	.test_cases	= configfs_test_cases,
+};
+kunit_test_suites(&configfs_test_module);
+
+MODULE_LICENSE("GPL v2");

Re: [PATCH v4 3/3] configfs: Add unit tests

[Christoph Hellwig]

> text and binary attribute support. This is how I run these tests:
> 
> set -e
> if [ -e .config ]; then
>     make ARCH=um mrproper
> fi
> if [ ! -e .kunit/.kunitconfig ]; then
>     cat <<EOF >.kunit/.kunitconfig
> CONFIG_CONFIGFS_FS=y
> CONFIG_CONFIGFS_KUNIT_TEST=y
> CONFIG_KUNIT=y
> CONFIG_PROVE_LOCKING=y
> CONFIG_SYSFS=y
> CONFIG_UBSAN=y
> EOF
>     cp .kunit/.kunitconfig .kunit/.config
> fi
> ./tools/testing/kunit/kunit.py run

This is very useful documentation, but shouldn't it go into a README.kunit
or similar instead of a commit message?

> +config CONFIGFS_KUNIT_TEST
> +	bool "Configfs Kunit test" if !KUNIT_ALL_TESTS
> +	depends on CONFIGFS_FS && KUNIT=y
> +	default KUNIT_ALL_TESTS

Why does it depend on KUNIT=y?  What is the issue with a modular KUNIT
build?


> +static int mkdir(const char *name, umode_t mode)
> +{
> +	struct dentry *dentry;
> +	struct path path;
> +	int err;
> +
> +	err = get_file_mode(name);
> +	if (err >= 0 && S_ISDIR(err))
> +		return 0;
> +
> +	dentry = kern_path_create(AT_FDCWD, name, &path, LOOKUP_DIRECTORY);
> +	if (IS_ERR(dentry))
> +		return PTR_ERR(dentry);
> +
> +	err = vfs_mkdir(&init_user_ns, d_inode(path.dentry), dentry, mode);
> +	done_path_create(&path, dentry);

To me this sounds like userspace would be a better place for these
kinds of tests.

Re: [PATCH v4 3/3] configfs: Add unit tests

[Bart Van Assche]

On 8/9/21 7:59 AM, Christoph Hellwig wrote:
>> text and binary attribute support. This is how I run these tests:
>>
>> set -e
>> if [ -e .config ]; then
>>     make ARCH=um mrproper
>> fi
>> if [ ! -e .kunit/.kunitconfig ]; then
>>     cat <<EOF >.kunit/.kunitconfig
>> CONFIG_CONFIGFS_FS=y
>> CONFIG_CONFIGFS_KUNIT_TEST=y
>> CONFIG_KUNIT=y
>> CONFIG_PROVE_LOCKING=y
>> CONFIG_SYSFS=y
>> CONFIG_UBSAN=y
>> EOF
>>     cp .kunit/.kunitconfig .kunit/.config
>> fi
>> ./tools/testing/kunit/kunit.py run
> 
> This is very useful documentation, but shouldn't it go into a README.kunit
> or similar instead of a commit message?

I can store this documentation in a new README, but isn't this something
that has already been explained in
Documentation/dev-tools/kunit/kunit-tool.rst?

>> +config CONFIGFS_KUNIT_TEST
>> +	bool "Configfs Kunit test" if !KUNIT_ALL_TESTS
>> +	depends on CONFIGFS_FS && KUNIT=y
>> +	default KUNIT_ALL_TESTS
> 
> Why does it depend on KUNIT=y?  What is the issue with a modular KUNIT
> build?

The unit tests calls do_mount(). do_mount() has not been exported and
hence is not available to kernel modules. Hence the exclusion of KUNIT=m.

>> +static int mkdir(const char *name, umode_t mode)
>> +{
>> +	struct dentry *dentry;
>> +	struct path path;
>> +	int err;
>> +
>> +	err = get_file_mode(name);
>> +	if (err >= 0 && S_ISDIR(err))
>> +		return 0;
>> +
>> +	dentry = kern_path_create(AT_FDCWD, name, &path, LOOKUP_DIRECTORY);
>> +	if (IS_ERR(dentry))
>> +		return PTR_ERR(dentry);
>> +
>> +	err = vfs_mkdir(&init_user_ns, d_inode(path.dentry), dentry, mode);
>> +	done_path_create(&path, dentry);
> 
> To me this sounds like userspace would be a better place for these
> kinds of tests.

Splitting the code that can only be run from inside the kernel (creation
of configfs attributes) and the code that can be run from user space and
making sure that the two run in a coordinated fashion would involve a
significant amount of work. I prefer to keep the current approach.

Thanks,

Bart.

Re: [PATCH v4 3/3] configfs: Add unit tests

[Christoph Hellwig]

On Mon, Aug 09, 2021 at 11:31:23AM -0700, Bart Van Assche wrote:
> I can store this documentation in a new README, but isn't this something
> that has already been explained in
> Documentation/dev-tools/kunit/kunit-tool.rst?

So reference that.

> 
> >> +config CONFIGFS_KUNIT_TEST
> >> +	bool "Configfs Kunit test" if !KUNIT_ALL_TESTS
> >> +	depends on CONFIGFS_FS && KUNIT=y
> >> +	default KUNIT_ALL_TESTS
> > 
> > Why does it depend on KUNIT=y?  What is the issue with a modular KUNIT
> > build?
> 
> The unit tests calls do_mount(). do_mount() has not been exported and
> hence is not available to kernel modules. Hence the exclusion of KUNIT=m.

You should probably document that.  But then again this is another
big red flag that this code should live in userspace.

> > To me this sounds like userspace would be a better place for these
> > kinds of tests.
> 
> Splitting the code that can only be run from inside the kernel (creation
> of configfs attributes) and the code that can be run from user space and
> making sure that the two run in a coordinated fashion would involve a
> significant amount of work. I prefer to keep the current approach.

But userspace is the right place to do this kind of pathname
based file system I/O.

So for the current in-kernel approach:

Nacked-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v4 3/3] configfs: Add unit tests

[Bart Van Assche]

On 8/10/21 9:50 AM, Christoph Hellwig wrote:
> On Mon, Aug 09, 2021 at 11:31:23AM -0700, Bart Van Assche wrote:
>>>> +config CONFIGFS_KUNIT_TEST
>>>> +	bool "Configfs Kunit test" if !KUNIT_ALL_TESTS
>>>> +	depends on CONFIGFS_FS && KUNIT=y
>>>> +	default KUNIT_ALL_TESTS
>>>
>>> Why does it depend on KUNIT=y?  What is the issue with a modular KUNIT
>>> build?
>>
>> The unit tests calls do_mount(). do_mount() has not been exported and
>> hence is not available to kernel modules. Hence the exclusion of KUNIT=m.
> 
> You should probably document that.  But then again this is another
> big red flag that this code should live in userspace.
> 
>>> To me this sounds like userspace would be a better place for these
>>> kinds of tests.
>>
>> Splitting the code that can only be run from inside the kernel (creation
>> of configfs attributes) and the code that can be run from user space and
>> making sure that the two run in a coordinated fashion would involve a
>> significant amount of work. I prefer to keep the current approach.
> 
> But userspace is the right place to do this kind of pathname
> based file system I/O.

Shuah, as selftest maintainer, can you recommend an approach? How about 
splitting patch 3/3 from this series into a kernel module (the code that 
creates the configfs test attributes) and user space code (the code that 
reads and writes the configfs attributes) and adding the user space code 
in a subdirectory of tools/testing/selftests/?

Thanks,

Bart.

Re: [PATCH v4 3/3] configfs: Add unit tests

[Shuah Khan]

On 8/10/21 12:45 PM, Bart Van Assche wrote:
> On 8/10/21 9:50 AM, Christoph Hellwig wrote:
>> On Mon, Aug 09, 2021 at 11:31:23AM -0700, Bart Van Assche wrote:
>>>>> +config CONFIGFS_KUNIT_TEST
>>>>> +    bool "Configfs Kunit test" if !KUNIT_ALL_TESTS
>>>>> +    depends on CONFIGFS_FS && KUNIT=y
>>>>> +    default KUNIT_ALL_TESTS
>>>>
>>>> Why does it depend on KUNIT=y?  What is the issue with a modular KUNIT
>>>> build?
>>>
>>> The unit tests calls do_mount(). do_mount() has not been exported and
>>> hence is not available to kernel modules. Hence the exclusion of KUNIT=m.
>>
>> You should probably document that.  But then again this is another
>> big red flag that this code should live in userspace.
>>
>>>> To me this sounds like userspace would be a better place for these
>>>> kinds of tests.
>>>
>>> Splitting the code that can only be run from inside the kernel (creation
>>> of configfs attributes) and the code that can be run from user space and
>>> making sure that the two run in a coordinated fashion would involve a
>>> significant amount of work. I prefer to keep the current approach.
>>
>> But userspace is the right place to do this kind of pathname
>> based file system I/O.
> 
> Shuah, as selftest maintainer, can you recommend an approach? How about splitting patch 3/3 from this series into a kernel module (the code that creates the configfs test attributes) and user space code (the code that reads and writes the configfs attributes) and adding the user space code in a subdirectory of tools/testing/selftests/?
> 

I am missing a lot of context here. I don't see this series in my inbox
except patch 2/3 which says:

"A common feature of unit testing frameworks is support for sharing a test
configuration across multiple unit tests. Add this functionality to the
KUnit framework. This functionality will be used in the next patch in this
series."

That doesn't tell me much other than what happens that it is a common unit
testing framework without explaining why it should be done this way.

Taking a quick look at the original message on lore - I agree with Christoph
that this code belongs in userspace. I would like to see the division of
kernel userspace.

Why do the unit tests need to call do_mount() - can whatever the unit tests
are currently doing can be done from userspace.

If part of the test code must live in kernel space then kernel test module
approach can be used.

thanks,
-- Shuah

Re: [PATCH v4 3/3] configfs: Add unit tests

[Brendan Higgins]

On Tue, Aug 10, 2021 at 1:50 PM Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 8/10/21 12:45 PM, Bart Van Assche wrote:
> > On 8/10/21 9:50 AM, Christoph Hellwig wrote:
> >> On Mon, Aug 09, 2021 at 11:31:23AM -0700, Bart Van Assche wrote:
> >>>>> +config CONFIGFS_KUNIT_TEST
> >>>>> +    bool "Configfs Kunit test" if !KUNIT_ALL_TESTS
> >>>>> +    depends on CONFIGFS_FS && KUNIT=y
> >>>>> +    default KUNIT_ALL_TESTS
> >>>>
> >>>> Why does it depend on KUNIT=y?  What is the issue with a modular KUNIT
> >>>> build?
> >>>
> >>> The unit tests calls do_mount(). do_mount() has not been exported and
> >>> hence is not available to kernel modules. Hence the exclusion of KUNIT=m.
> >>
> >> You should probably document that.  But then again this is another
> >> big red flag that this code should live in userspace.
> >>
> >>>> To me this sounds like userspace would be a better place for these
> >>>> kinds of tests.
> >>>
> >>> Splitting the code that can only be run from inside the kernel (creation
> >>> of configfs attributes) and the code that can be run from user space and
> >>> making sure that the two run in a coordinated fashion would involve a
> >>> significant amount of work. I prefer to keep the current approach.
> >>
> >> But userspace is the right place to do this kind of pathname
> >> based file system I/O.
> >
> > Shuah, as selftest maintainer, can you recommend an approach? How about splitting patch 3/3 from this series into a kernel module (the code that creates the configfs test attributes) and user space code (the code that reads and writes the configfs attributes) and adding the user space code in a subdirectory of tools/testing/selftests/?
> >
>
> I am missing a lot of context here. I don't see this series in my inbox
> except patch 2/3 which says:
>
> "A common feature of unit testing frameworks is support for sharing a test
> configuration across multiple unit tests. Add this functionality to the
> KUnit framework. This functionality will be used in the next patch in this
> series."

Yeah, I mentioned this to one of the other KUnit people who said he
might want to post some comments. Bart, could you CC
kunit-dev@googlegroups.com and/or linux-kselftest@vger.kernel.org
if/when you send follow-up patches?

Actually, I suppose regardless of what you do with this patch, you
will probably want to merge via the kselftest tree (KUnit changes and
many tests go through the kselftest tree as well). So, you should
probably CC linux-kselftest@vger.kernel.org no matter what.

> That doesn't tell me much other than what happens that it is a common unit
> testing framework without explaining why it should be done this way.
>
> Taking a quick look at the original message on lore - I agree with Christoph
> that this code belongs in userspace. I would like to see the division of
> kernel userspace.
>
> Why do the unit tests need to call do_mount() - can whatever the unit tests
> are currently doing can be done from userspace.
>
> If part of the test code must live in kernel space then kernel test module
> approach can be used.
>
> thanks,
> -- Shuah
>
>
>
>

Re: [PATCH v4 3/3] configfs: Add unit tests

[Brendan Higgins]

On Wed, Aug 11, 2021 at 5:00 AM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Tue, Aug 10, 2021 at 1:50 PM Shuah Khan <skhan@linuxfoundation.org> wrote:
> >
> > On 8/10/21 12:45 PM, Bart Van Assche wrote:
> > > On 8/10/21 9:50 AM, Christoph Hellwig wrote:
> > >> On Mon, Aug 09, 2021 at 11:31:23AM -0700, Bart Van Assche wrote:
> > >>>>> +config CONFIGFS_KUNIT_TEST
> > >>>>> +    bool "Configfs Kunit test" if !KUNIT_ALL_TESTS
> > >>>>> +    depends on CONFIGFS_FS && KUNIT=y
> > >>>>> +    default KUNIT_ALL_TESTS
> > >>>>
> > >>>> Why does it depend on KUNIT=y?  What is the issue with a modular KUNIT
> > >>>> build?
> > >>>
> > >>> The unit tests calls do_mount(). do_mount() has not been exported and
> > >>> hence is not available to kernel modules. Hence the exclusion of KUNIT=m.
> > >>
> > >> You should probably document that.  But then again this is another
> > >> big red flag that this code should live in userspace.
> > >>
> > >>>> To me this sounds like userspace would be a better place for these
> > >>>> kinds of tests.
> > >>>
> > >>> Splitting the code that can only be run from inside the kernel (creation
> > >>> of configfs attributes) and the code that can be run from user space and
> > >>> making sure that the two run in a coordinated fashion would involve a
> > >>> significant amount of work. I prefer to keep the current approach.
> > >>
> > >> But userspace is the right place to do this kind of pathname
> > >> based file system I/O.
> > >
> > > Shuah, as selftest maintainer, can you recommend an approach? How about splitting patch 3/3 from this series into a kernel module (the code that creates the configfs test attributes) and user space code (the code that reads and writes the configfs attributes) and adding the user space code in a subdirectory of tools/testing/selftests/?
> > >
> >
> > I am missing a lot of context here. I don't see this series in my inbox
> > except patch 2/3 which says:
> >
> > "A common feature of unit testing frameworks is support for sharing a test
> > configuration across multiple unit tests. Add this functionality to the
> > KUnit framework. This functionality will be used in the next patch in this
> > series."
>
> Yeah, I mentioned this to one of the other KUnit people who said he
> might want to post some comments. Bart, could you CC
> kunit-dev@googlegroups.com and/or linux-kselftest@vger.kernel.org
> if/when you send follow-up patches?
>
> Actually, I suppose regardless of what you do with this patch, you
> will probably want to merge via the kselftest tree (KUnit changes and
> many tests go through the kselftest tree as well). So, you should
> probably CC linux-kselftest@vger.kernel.org no matter what.
>
> > That doesn't tell me much other than what happens that it is a common unit
> > testing framework without explaining why it should be done this way.
> >
> > Taking a quick look at the original message on lore - I agree with Christoph
> > that this code belongs in userspace. I would like to see the division of
> > kernel userspace.
> >
> > Why do the unit tests need to call do_mount() - can whatever the unit tests
> > are currently doing can be done from userspace.
> >
> > If part of the test code must live in kernel space then kernel test module
> > approach can be used.

I am not sure if you are still working on this test or not, but Kees
recently posted a patch to create a KUnit UAPI:
https://lore.kernel.org/lkml/20220227184517.504931-8-keescook@chromium.org/

Not sure if you have any interest, or even if this would be the right
tool for the job, but I figured it's worth a look because it might
save you the effort of having to completely rewrite your test.

Cheers

Re: [PATCH v4 3/3] configfs: Add unit tests

[Brendan Higgins]

On Mon, Aug 9, 2021 at 7:59 AM Christoph Hellwig <hch@lst.de> wrote:
>
> > text and binary attribute support. This is how I run these tests:
> >
> > set -e
> > if [ -e .config ]; then
> >     make ARCH=um mrproper
> > fi
> > if [ ! -e .kunit/.kunitconfig ]; then
> >     cat <<EOF >.kunit/.kunitconfig
> > CONFIG_CONFIGFS_FS=y
> > CONFIG_CONFIGFS_KUNIT_TEST=y
> > CONFIG_KUNIT=y
> > CONFIG_PROVE_LOCKING=y
> > CONFIG_SYSFS=y
> > CONFIG_UBSAN=y
> > EOF
> >     cp .kunit/.kunitconfig .kunit/.config
> > fi
> > ./tools/testing/kunit/kunit.py run
>
> This is very useful documentation, but shouldn't it go into a README.kunit
> or similar instead of a commit message?

You could also put this in a .kunitconfig specific to your subsystem
like we did for ext4:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/.kunitconfig

You can then build using this .kunitconfig with something like:

./tools/testing/kunit/kunit.py run --kunitconfig=fs/ext4/.kunitconfig

> > +config CONFIGFS_KUNIT_TEST
> > +     bool "Configfs Kunit test" if !KUNIT_ALL_TESTS
> > +     depends on CONFIGFS_FS && KUNIT=y
> > +     default KUNIT_ALL_TESTS

Cheers!

Re: [PATCH v4 3/3] configfs: Add unit tests

[Bart Van Assche]

On 8/10/21 3:00 PM, Brendan Higgins wrote:
> On Mon, Aug 9, 2021 at 7:59 AM Christoph Hellwig <hch@lst.de> wrote:
>>
>>> text and binary attribute support. This is how I run these tests:
>>>
>>> set -e
>>> if [ -e .config ]; then
>>>     make ARCH=um mrproper
>>> fi
>>> if [ ! -e .kunit/.kunitconfig ]; then
>>>     cat <<EOF >.kunit/.kunitconfig
>>> CONFIG_CONFIGFS_FS=y
>>> CONFIG_CONFIGFS_KUNIT_TEST=y
>>> CONFIG_KUNIT=y
>>> CONFIG_PROVE_LOCKING=y
>>> CONFIG_SYSFS=y
>>> CONFIG_UBSAN=y
>>> EOF
>>>     cp .kunit/.kunitconfig .kunit/.config
>>> fi
>>> ./tools/testing/kunit/kunit.py run
>>
>> This is very useful documentation, but shouldn't it go into a README.kunit
>> or similar instead of a commit message?
> 
> You could also put this in a .kunitconfig specific to your subsystem
> like we did for ext4:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/.kunitconfig
> 
> You can then build using this .kunitconfig with something like:
> 
> ./tools/testing/kunit/kunit.py run --kunitconfig=fs/ext4/.kunitconfig

Will look into this. Thanks for the feedback!

Bart.

Re: [PATCH v4 0/3] Restore the kernel v5.13 text attribute write behavior

[Christoph Hellwig]

Thanks,

I've applied patch 1 and will send it to Linus this week.

Patch 2 seems like something that should go through the kunit tree,
and if we do so patch 3 should go along with that.