Re: [PATCH v2 03/10] blktrace: fix debugfs use after free

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 4/19/20 12:45 PM, Luis Chamberlain wrote:
+int __must_check blk_queue_debugfs_register(struct request_queue *q)
+{
+	struct dentry *dir = NULL;
+
+	/* This can happen if we have a bug in the lower layers */

What does "this" refer to? Which layers does "lower layers" refer to? Most software developers consider a module that calls directly into another module as a higher layer (callbacks through function pointers do not count; see also https://en.wikipedia.org/wiki/Modular_programming). According to that definition block drivers are a software layer immediately above the block layer core.

How about changing that comment into the following to make it unambiguous (if this is what you meant)?

	/*
	 * Check whether the debugfs directory already exists. This can
	 * only happen as the result of a bug in a block driver.
	 */

+	dir = debugfs_lookup(kobject_name(q->kobj.parent), blk_debugfs_root);
+	if (dir) {
+		pr_warn("%s: registering request_queue debugfs directory twice is not allowed\n",
+			kobject_name(q->kobj.parent));
+		dput(dir);
+		return -EALREADY;
+	}
+
+	q->debugfs_dir = debugfs_create_dir(kobject_name(q->kobj.parent),
+					    blk_debugfs_root);
+	if (!q->debugfs_dir)
+		return -ENOMEM;
+
+	return 0;
+}

kobject_name(q->kobj.parent) is used three times in the above function. How about introducing a local variable that holds the result of that expression?

+static bool blk_trace_target_disk(const char *target, const char *diskname)
+{
+	if (strlen(target) != strlen(diskname))
+		return false;
+
+	if (!strncmp(target, diskname,
+		     min_t(size_t, strlen(target), strlen(diskname))))
+		return true;
+
+	return false;
+}

The above code looks weird to me. When the second if-statement is reached, it is guaranteed that 'target' and 'diskname' have the same length. So why to calculate the minimum length in the second if-statement of two strings that have the same length?

Independent of what the purpose of the above code is, can that code be rewritten such that it does not depend on the details of how names are assigned to disks and partitions? Would disk_get_part() be useful here?

Thanks,

Bart.




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux