Re: [PATCH v25 12/12] LRNG - add interface for gathering of raw entropy

Am Donnerstag, 21. November 2019, 13:18:10 CET schrieb Nicolai Stange:

Hi Nicolai,

Hi Stephan,

two general remarks on debugfs usage below

Stephan Müller [off-list ref] writes:

quoted

diff --git a/drivers/char/lrng/lrng_testing.c

b/drivers/char/lrng/lrng_testing.c new file mode 100644
index 000000000000..5c33d3bd2172

--- /dev/null
+++ b/drivers/char/lrng/lrng_testing.c

<snip>

quoted

+/*
+ * This data structure holds the dentry's of the debugfs files
establishing + * the interface to user space.
+ */
+struct lrng_raw_debugfs {
+	struct dentry *lrng_raw_debugfs_root; /* root dentry */
+	struct dentry *lrng_raw_debugfs_lrng_raw; /* .../lrng_raw */
+};
+
+static struct lrng_raw_debugfs lrng_raw_debugfs;
+
+/* DebugFS operations and definition of the debugfs files */
+static ssize_t lrng_raw_read(struct file *file, char __user *to,
+			     size_t count, loff_t *ppos)
+{
+	loff_t pos = *ppos;
+	int ret;
+
+	if (!count)
+		return 0;
+	lrng_raw_entropy_init();
+	ret = lrng_raw_extract_user(to, count);
+	lrng_raw_entropy_fini();
+	if (ret < 0)
+		return ret;
+	count -= ret;
+	*ppos = pos + count;
+	return ret;
+}
+
+/* Module init: allocate memory, register the debugfs files */
+static int lrng_raw_debugfs_init(void)
+{
+	lrng_raw_debugfs.lrng_raw_debugfs_root =
+		debugfs_create_dir(KBUILD_MODNAME, NULL);
+	if (IS_ERR(lrng_raw_debugfs.lrng_raw_debugfs_root)) {
+		lrng_raw_debugfs.lrng_raw_debugfs_root = NULL;
+		return PTR_ERR(lrng_raw_debugfs.lrng_raw_debugfs_root);
+	}

I think pointers returned by the debugfs API are not supposed to get
checked for NULL/IS_ERR(), c.f commit ff9fb72bc077 ("debugfs: return
error values, not NULL") or the the output from

  git log --pretty=oneline | grep 'no need to check return value of
debugfs_create'

(Also the above code is dubious: you're effectively returning
 PTR_ERR(NULL)).

Removed the check compliant to the mentioned patches.

quoted

+	return 0;
+}
+
+static struct file_operations lrng_raw_name_fops = {
+	.owner = THIS_MODULE,
+	.read = lrng_raw_read,
+};
+
+static int lrng_raw_debugfs_init_name(void)
+{
+	lrng_raw_debugfs.lrng_raw_debugfs_lrng_raw =
+		debugfs_create_file("lrng_raw", 0400,
+				    lrng_raw_debugfs.lrng_raw_debugfs_root,
+				    NULL, &lrng_raw_name_fops);q

CONFIG_LRNG_TESTING is a bool and thus, this debugfs file can't ever get
removed. Even if it could, this inode hasn't got any data associated
with it and so file removal would not be a problem for lrng_raw_read().

Correct.

Please consider using debugfs_create_file_unsafe() instead to save
debugfs from kmalloc()ing a proxy file_operations protecting your fops
against concurrent file removal.

Yes, you are correct. Changed.

quoted

+	if (IS_ERR(lrng_raw_debugfs.lrng_raw_debugfs_lrng_raw)) {
+		lrng_raw_debugfs.lrng_raw_debugfs_lrng_raw = NULL;
+		return PTR_ERR(lrng_raw_debugfs.lrng_raw_debugfs_lrng_raw);
+	}

Same comment regarding return value checking applies here.

Same here: I removed the check.

With that, I also removed the static variable to maintain the two dentries 
following the examples seen in other kernel code. Also, the __exit function is 
removed as we do not need it as you pointed out.

Thanks a lot.

Thanks,

Nicolai

quoted

+	return 0;
+}
+
+static int __init lrng_raw_init(void)
+{
+	int ret = lrng_raw_debugfs_init();
+
+	if (ret < 0)
+		return ret;
+
+	ret = lrng_raw_debugfs_init_name();
+	if (ret < 0)
+		debugfs_remove_recursive(
+					lrng_raw_debugfs.lrng_raw_debugfs_root);
+
+	return ret;
+}
+
+static void __exit lrng_raw_exit(void)
+{
+	debugfs_remove_recursive(lrng_raw_debugfs.lrng_raw_debugfs_root);
+}
+
+module_init(lrng_raw_init);
+module_exit(lrng_raw_exit);
+
+MODULE_LICENSE("Dual BSD/GPL");
+MODULE_AUTHOR("Stephan Mueller <smueller@chronox.de>");
+MODULE_DESCRIPTION("Kernel module for gathering raw entropy");

Ciao
Stephan