[PATCH 0/4] char: misc: improve test and dynamic allocation

[PATCH 0/4] char: misc: improve test and dynamic allocation

[Thadeu Lima de Souza Cascardo]

These apply on top of commit 6d04d2b554b1 ("misc: misc_minor_alloc to use
ida for all dynamic/misc dynamic minors") and commit 37df9043329b
("misc:minor basic kunit tests"), both at char-misc-next.

The tests are written in order to detect regressions around the bug fixed
by commit 6d04d2b554b1 ("misc: misc_minor_alloc to use ida for all
dynamic/misc dynamic minors") and the followup fixing commits "char: misc:
restrict the dynamic range to exclude reserved minors" and "char: misc:
deallocate static minor in error path".

Thadeu Lima de Souza Cascardo (4):
  char: misc: improve testing Kconfig description
  char: misc: add test cases
  char: misc: restrict the dynamic range to exclude reserved minors
  char: misc: deallocate static minor in error path

 drivers/char/misc.c             |   6 +-
 drivers/misc/misc_minor_kunit.c | 515 +++++++++++++++++++++++++++++++-
 include/linux/miscdevice.h      |   1 +
 lib/Kconfig.debug               |  15 +-
 4 files changed, 529 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH 1/4] char: misc: improve testing Kconfig description

[Thadeu Lima de Souza Cascardo]

Describe that it tests the miscdevice API and include the usual disclaimer
about KUnit not being fit for production kernels.

While at it, also fix KUnit capitalization.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 lib/Kconfig.debug | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 775966cf6114..8612ba8771aa 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2493,13 +2493,20 @@ config TEST_IDA
 	tristate "Perform selftest on IDA functions"
 
 config TEST_MISC_MINOR
-	tristate "Basic misc minor Kunit test" if !KUNIT_ALL_TESTS
+	tristate "miscdevice KUnit test" if !KUNIT_ALL_TESTS
 	depends on KUNIT
 	default KUNIT_ALL_TESTS
 	help
-	  Kunit test for the misc minor.
-	  It tests misc minor functions for dynamic and misc dynamic minor.
-	  This include misc_xxx functions
+	  Kunit test for miscdevice API, specially its behavior in respect to
+	  static and dynamic minor numbers.
+
+	  KUnit tests run during boot and output the results to the debug log
+	  in TAP format (https://testanything.org/). Only useful for kernel devs
+	  running the KUnit test harness, and not intended for inclusion into a
+	  production build.
+
+	  For more information on KUnit and unit tests in general please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
 
 	  If unsure, say N.
 
-- 
2.34.1

[PATCH 2/4] char: misc: add test cases

[Thadeu Lima de Souza Cascardo]

Add test cases for static and dynamic minor number allocation and
deallocation.

While at it, improve description and test suite name.

Some of the cases include:

- that static and dynamic allocation reserved the expected minors.

- that registering duplicate minors or duplicate names will fail.

- that failing to create a sysfs file (due to duplicate names) will
  deallocate the dynamic minor correctly.

- that dynamic allocation does not allocate a minor number in the static
  range.

- that there are no collisions when mixing dynamic and static allocations.

- that registering a static number in the dynamic range won't conflict with
  a dynamic allocation.

This last test verifies the bug fixed by commit 6d04d2b554b1 ("misc:
misc_minor_alloc to use ida for all dynamic/misc dynamic minors") has not
regressed.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 drivers/misc/misc_minor_kunit.c | 515 +++++++++++++++++++++++++++++++-
 1 file changed, 513 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/misc_minor_kunit.c b/drivers/misc/misc_minor_kunit.c
index 293e0fb7e43e..84e13bc5c61c 100644
--- a/drivers/misc/misc_minor_kunit.c
+++ b/drivers/misc/misc_minor_kunit.c
@@ -51,19 +51,530 @@ static void kunit_misc_dynamic_minor(struct kunit *test)
 	misc_deregister(&dev_misc_dynamic_minor);
 }
 
+struct miscdev_test_case {
+	const char *str;
+	int minor;
+};
+
+static struct miscdev_test_case miscdev_test_ranges[] = {
+	{
+		.str = "lower static range, top",
+		.minor = 15,
+	},
+	{
+		.str = "upper static range, bottom",
+		.minor = 130,
+	},
+	{
+		.str = "lower static range, bottom",
+		.minor = 0,
+	},
+	{
+		.str = "upper static range, top",
+		.minor = MISC_DYNAMIC_MINOR - 1,
+	},
+};
+
+KUNIT_ARRAY_PARAM_DESC(miscdev, miscdev_test_ranges, str);
+
+static int miscdev_find_minors(struct kunit_suite *suite)
+{
+	int ret;
+	struct miscdevice miscstat = {
+		.name = "miscstat",
+	};
+	int i;
+
+	for (i = 15; i >= 0; i--) {
+		miscstat.minor = i;
+		ret = misc_register(&miscstat);
+		if (ret == 0)
+			break;
+	}
+
+	if (ret == 0) {
+		kunit_info(suite, "found misc device minor %d available\n",
+				miscstat.minor);
+		miscdev_test_ranges[0].minor = miscstat.minor;
+		misc_deregister(&miscstat);
+	} else {
+		return ret;
+	}
+
+	for (i = 128; i < MISC_DYNAMIC_MINOR; i++) {
+		miscstat.minor = i;
+		ret = misc_register(&miscstat);
+		if (ret == 0)
+			break;
+	}
+
+	if (ret == 0) {
+		kunit_info(suite, "found misc device minor %d available\n",
+				miscstat.minor);
+		miscdev_test_ranges[1].minor = miscstat.minor;
+		misc_deregister(&miscstat);
+	} else {
+		return ret;
+	}
+
+	for (i = 0; i < miscdev_test_ranges[0].minor; i++) {
+		miscstat.minor = i;
+		ret = misc_register(&miscstat);
+		if (ret == 0)
+			break;
+	}
+
+	if (ret == 0) {
+		kunit_info(suite, "found misc device minor %d available\n",
+			miscstat.minor);
+		miscdev_test_ranges[2].minor = miscstat.minor;
+		misc_deregister(&miscstat);
+	} else {
+		return ret;
+	}
+
+	for (i = MISC_DYNAMIC_MINOR - 1; i > miscdev_test_ranges[1].minor; i--) {
+		miscstat.minor = i;
+		ret = misc_register(&miscstat);
+		if (ret == 0)
+			break;
+	}
+
+	if (ret == 0) {
+		kunit_info(suite, "found misc device minor %d available\n",
+			miscstat.minor);
+		miscdev_test_ranges[3].minor = miscstat.minor;
+		misc_deregister(&miscstat);
+	}
+
+	return ret;
+}
+
+static bool is_valid_dynamic_minor(int minor)
+{
+	if (minor < 0)
+		return false;
+	if (minor == MISC_DYNAMIC_MINOR)
+		return false;
+	if (minor >= 0 && minor <= 15)
+		return false;
+	if (minor >= 128 && minor < MISC_DYNAMIC_MINOR)
+		return false;
+	return true;
+}
+
+static void miscdev_test_static_basic(struct kunit *test)
+{
+	struct miscdevice misc_test = {
+		.name = "misc_test",
+	};
+	int ret;
+	const struct miscdev_test_case *params = test->param_value;
+
+	misc_test.minor = params->minor;
+
+	ret = misc_register(&misc_test);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_EQ(test, misc_test.minor, params->minor);
+	if (ret == 0)
+		misc_deregister(&misc_test);
+}
+
+static void miscdev_test_dynamic_basic(struct kunit *test)
+{
+	struct miscdevice misc_test = {
+		.minor = MISC_DYNAMIC_MINOR,
+		.name = "misc_test",
+	};
+	int ret;
+
+	ret = misc_register(&misc_test);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_TRUE(test, is_valid_dynamic_minor(misc_test.minor));
+	if (ret == 0)
+		misc_deregister(&misc_test);
+}
+
+static void miscdev_test_twice(struct kunit *test)
+{
+	struct miscdevice misc_test = {
+		.name = "misc_test",
+	};
+	int ret;
+	const struct miscdev_test_case *params = test->param_value;
+
+	misc_test.minor = params->minor;
+
+	ret = misc_register(&misc_test);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_EQ(test, misc_test.minor, params->minor);
+	if (ret == 0)
+		misc_deregister(&misc_test);
+
+	ret = misc_register(&misc_test);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_EQ(test, misc_test.minor, params->minor);
+	if (ret == 0)
+		misc_deregister(&misc_test);
+}
+
+static void miscdev_test_duplicate_minor(struct kunit *test)
+{
+	struct miscdevice misc1 = {
+		.name = "misc1",
+	};
+	struct miscdevice misc2 = {
+		.name = "misc2",
+	};
+	int ret;
+	const struct miscdev_test_case *params = test->param_value;
+
+	misc1.minor = params->minor;
+	misc2.minor = params->minor;
+
+	ret = misc_register(&misc1);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_EQ(test, misc1.minor, params->minor);
+
+	ret = misc_register(&misc2);
+	KUNIT_EXPECT_EQ(test, ret, -EBUSY);
+	if (ret == 0)
+		misc_deregister(&misc2);
+
+	misc_deregister(&misc1);
+}
+
+static void miscdev_test_duplicate_name(struct kunit *test)
+{
+	struct miscdevice misc1 = {
+		.minor = MISC_DYNAMIC_MINOR,
+		.name = "misc1",
+	};
+	struct miscdevice misc2 = {
+		.minor = MISC_DYNAMIC_MINOR,
+		.name = "misc1",
+	};
+	int ret;
+
+	ret = misc_register(&misc1);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_TRUE(test, is_valid_dynamic_minor(misc1.minor));
+
+	ret = misc_register(&misc2);
+	KUNIT_EXPECT_EQ(test, ret, -EEXIST);
+	if (ret == 0)
+		misc_deregister(&misc2);
+
+	misc_deregister(&misc1);
+}
+
+/*
+ * Test that after a duplicate name failure, the reserved minor number is
+ * freed to be allocated next.
+ */
+static void miscdev_test_duplicate_name_leak(struct kunit *test)
+{
+	struct miscdevice misc1 = {
+		.minor = MISC_DYNAMIC_MINOR,
+		.name = "misc1",
+	};
+	struct miscdevice misc2 = {
+		.minor = MISC_DYNAMIC_MINOR,
+		.name = "misc1",
+	};
+	struct miscdevice misc3 = {
+		.minor = MISC_DYNAMIC_MINOR,
+		.name = "misc3",
+	};
+	int ret;
+	int dyn_minor;
+
+	ret = misc_register(&misc1);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_TRUE(test, is_valid_dynamic_minor(misc1.minor));
+
+	/*
+	 * Find out what is the next minor number available.
+	 */
+	ret = misc_register(&misc3);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_TRUE(test, is_valid_dynamic_minor(misc3.minor));
+	dyn_minor = misc3.minor;
+	misc_deregister(&misc3);
+	misc3.minor = MISC_DYNAMIC_MINOR;
+
+	ret = misc_register(&misc2);
+	KUNIT_EXPECT_EQ(test, ret, -EEXIST);
+	if (ret == 0)
+		misc_deregister(&misc2);
+
+	/*
+	 * Now check that we can still get the same minor we found before.
+	 */
+	ret = misc_register(&misc3);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_TRUE(test, is_valid_dynamic_minor(misc3.minor));
+	KUNIT_EXPECT_EQ(test, misc3.minor, dyn_minor);
+	misc_deregister(&misc3);
+
+	misc_deregister(&misc1);
+}
+
+/*
+ * Try to register a static minor with a duplicate name. That might not
+ * deallocate the minor, preventing it from being used again.
+ */
+static void miscdev_test_duplicate_error(struct kunit *test)
+{
+	struct miscdevice miscdyn = {
+		.minor = MISC_DYNAMIC_MINOR,
+		.name = "name1",
+	};
+	struct miscdevice miscstat = {
+		.name = "name1",
+	};
+	struct miscdevice miscnew = {
+		.name = "name2",
+	};
+	int ret;
+	const struct miscdev_test_case *params = test->param_value;
+
+	miscstat.minor = params->minor;
+	miscnew.minor = params->minor;
+
+	ret = misc_register(&miscdyn);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_TRUE(test, is_valid_dynamic_minor(miscdyn.minor));
+
+	ret = misc_register(&miscstat);
+	KUNIT_EXPECT_EQ(test, ret, -EEXIST);
+	if (ret == 0)
+		misc_deregister(&miscstat);
+
+	ret = misc_register(&miscnew);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_EQ(test, miscnew.minor, params->minor);
+	if (ret == 0)
+		misc_deregister(&miscnew);
+
+	misc_deregister(&miscdyn);
+}
+
+static void miscdev_test_dynamic_only_range(struct kunit *test)
+{
+	int ret;
+	struct miscdevice *miscdev;
+	const int dynamic_minors = 256;
+	int i;
+	bool found_16 = false;
+
+	miscdev = kunit_kmalloc_array(test, dynamic_minors,
+					sizeof(struct miscdevice),
+					GFP_KERNEL | __GFP_ZERO);
+
+	for (i = 0; i < dynamic_minors; i++) {
+		miscdev[i].minor = MISC_DYNAMIC_MINOR;
+		miscdev[i].name = kasprintf(GFP_KERNEL, "misc_test%d", i);
+		ret = misc_register(&miscdev[i]);
+		if (ret != 0)
+			break;
+		/*
+		 * This is the bug we are looking for!
+		 * We asked for a dynamic minor and got a minor in the static range space.
+		 */
+		if (miscdev[i].minor >= 0 && miscdev[i].minor <= 15) {
+			KUNIT_FAIL(test, "misc_register allocated minor %d\n", miscdev[i].minor);
+			i++;
+			break;
+		}
+		KUNIT_EXPECT_TRUE(test, is_valid_dynamic_minor(miscdev[i].minor));
+	}
+
+	for (i--; i >= 0; i--) {
+		/* Verify we are still able to allocate minor number 16. */
+		if (miscdev[i].minor == 16)
+			found_16 = true;
+		misc_deregister(&miscdev[i]);
+		kfree_const(miscdev[i].name);
+	}
+
+	KUNIT_EXPECT_TRUE(test, found_16);
+
+	KUNIT_EXPECT_EQ(test, ret, 0);
+}
+
+static void miscdev_test_collision(struct kunit *test)
+{
+	int ret;
+	struct miscdevice *miscdev;
+	struct miscdevice miscstat = {
+		.name = "miscstat",
+	};
+	const int dynamic_minors = 256;
+	int i;
+
+	miscdev = kunit_kmalloc_array(test, dynamic_minors,
+					sizeof(struct miscdevice),
+					GFP_KERNEL | __GFP_ZERO);
+
+	miscstat.minor = miscdev_test_ranges[0].minor;
+	ret = misc_register(&miscstat);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+	KUNIT_EXPECT_EQ(test, miscstat.minor, miscdev_test_ranges[0].minor);
+
+	for (i = 0; i < dynamic_minors; i++) {
+		miscdev[i].minor = MISC_DYNAMIC_MINOR;
+		miscdev[i].name = kasprintf(GFP_KERNEL, "misc_test%d", i);
+		ret = misc_register(&miscdev[i]);
+		if (ret != 0)
+			break;
+		KUNIT_EXPECT_TRUE(test, is_valid_dynamic_minor(miscdev[i].minor));
+	}
+
+	for (i--; i >= 0; i--) {
+		misc_deregister(&miscdev[i]);
+		kfree_const(miscdev[i].name);
+	}
+
+	misc_deregister(&miscstat);
+
+	KUNIT_EXPECT_EQ(test, ret, 0);
+}
+
+static void miscdev_test_collision_reverse(struct kunit *test)
+{
+	int ret;
+	struct miscdevice *miscdev;
+	struct miscdevice miscstat = {
+		.name = "miscstat",
+	};
+	const int dynamic_minors = 256;
+	int i;
+
+	miscdev = kunit_kmalloc_array(test, dynamic_minors,
+					sizeof(struct miscdevice),
+					GFP_KERNEL | __GFP_ZERO);
+
+	for (i = 0; i < dynamic_minors; i++) {
+		miscdev[i].minor = MISC_DYNAMIC_MINOR;
+		miscdev[i].name = kasprintf(GFP_KERNEL, "misc_test%d", i);
+		ret = misc_register(&miscdev[i]);
+		if (ret != 0)
+			break;
+		KUNIT_EXPECT_TRUE(test, is_valid_dynamic_minor(miscdev[i].minor));
+	}
+
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	miscstat.minor = miscdev_test_ranges[0].minor;
+	ret = misc_register(&miscstat);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_EQ(test, miscstat.minor, miscdev_test_ranges[0].minor);
+	if (ret == 0)
+		misc_deregister(&miscstat);
+
+	for (i--; i >= 0; i--) {
+		misc_deregister(&miscdev[i]);
+		kfree_const(miscdev[i].name);
+	}
+}
+
+static void miscdev_test_conflict(struct kunit *test)
+{
+	int ret;
+	struct miscdevice miscdyn = {
+		.name = "miscdyn",
+		.minor = MISC_DYNAMIC_MINOR,
+	};
+	struct miscdevice miscstat = {
+		.name = "miscstat",
+	};
+
+	ret = misc_register(&miscdyn);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+	KUNIT_EXPECT_TRUE(test, is_valid_dynamic_minor(miscdyn.minor));
+
+	/*
+	 * Try to register a static minor with the same minor as the
+	 * dynamic one.
+	 */
+	miscstat.minor = miscdyn.minor;
+	ret = misc_register(&miscstat);
+	KUNIT_EXPECT_EQ(test, ret, -EBUSY);
+	if (ret == 0)
+		misc_deregister(&miscstat);
+
+	misc_deregister(&miscdyn);
+}
+
+static void miscdev_test_conflict_reverse(struct kunit *test)
+{
+	int ret;
+	struct miscdevice miscdyn = {
+		.name = "miscdyn",
+		.minor = MISC_DYNAMIC_MINOR,
+	};
+	struct miscdevice miscstat = {
+		.name = "miscstat",
+	};
+
+	/*
+	 * Find the first available dynamic minor to use it as a static
+	 * minor later on.
+	 */
+	ret = misc_register(&miscdyn);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+	KUNIT_EXPECT_TRUE(test, is_valid_dynamic_minor(miscdyn.minor));
+	miscstat.minor = miscdyn.minor;
+	misc_deregister(&miscdyn);
+
+	ret = misc_register(&miscstat);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_EQ(test, miscstat.minor, miscdyn.minor);
+
+	/*
+	 * Try to register a dynamic minor after registering a static minor
+	 * within the dynamic range. It should work but get a different
+	 * minor.
+	 */
+	miscdyn.minor = MISC_DYNAMIC_MINOR;
+	ret = misc_register(&miscdyn);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	KUNIT_EXPECT_NE(test, miscdyn.minor, miscstat.minor);
+	KUNIT_EXPECT_TRUE(test, is_valid_dynamic_minor(miscdyn.minor));
+	if (ret == 0)
+		misc_deregister(&miscdyn);
+
+	misc_deregister(&miscstat);
+}
+
 static struct kunit_case test_cases[] = {
 	KUNIT_CASE(kunit_dynamic_minor),
 	KUNIT_CASE(kunit_static_minor),
 	KUNIT_CASE(kunit_misc_dynamic_minor),
+	KUNIT_CASE_PARAM(miscdev_test_static_basic, miscdev_gen_params),
+	KUNIT_CASE(miscdev_test_dynamic_basic),
+	KUNIT_CASE_PARAM(miscdev_test_twice, miscdev_gen_params),
+	KUNIT_CASE_PARAM(miscdev_test_duplicate_minor, miscdev_gen_params),
+	KUNIT_CASE(miscdev_test_duplicate_name),
+	KUNIT_CASE(miscdev_test_duplicate_name_leak),
+	KUNIT_CASE_PARAM(miscdev_test_duplicate_error, miscdev_gen_params),
+	KUNIT_CASE(miscdev_test_dynamic_only_range),
+	KUNIT_CASE(miscdev_test_collision),
+	KUNIT_CASE(miscdev_test_collision_reverse),
+	KUNIT_CASE(miscdev_test_conflict),
+	KUNIT_CASE(miscdev_test_conflict_reverse),
 	{}
 };
 
 static struct kunit_suite test_suite = {
-	.name = "misc_minor_test",
+	.name = "miscdev",
+	.suite_init = miscdev_find_minors,
 	.test_cases = test_cases,
 };
 kunit_test_suite(test_suite);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Vimal Agrawal");
-MODULE_DESCRIPTION("misc minor testing");
+MODULE_AUTHOR("Thadeu Lima de Souza Cascardo <cascardo@igalia.com>");
+MODULE_DESCRIPTION("Test module for misc character devices");
-- 
2.34.1

[PATCH 3/4] char: misc: restrict the dynamic range to exclude reserved minors

[Thadeu Lima de Souza Cascardo]

When this was first reported [1], the possibility of having sufficient
number of dynamic misc devices was theoretical.

What we know from commit ab760791c0cf ("char: misc: Increase the maximum
number of dynamic misc devices to 1048448"), is that the miscdevice
interface has been used for allocating more than the single-shot devices it
was designed for.

On such systems, it is certain that the dynamic allocation will allocate
certain reserved minor numbers, leading to failures when a later driver
tries to claim its reserved number.

Fixing this is a simple matter of defining the IDA range to allocate from
to exclude minors up to and including 15.

[1] https://lore.kernel.org/all/1257813017-28598-3-git-send-email-cascardo@holoscopio.com/

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 drivers/char/misc.c        | 4 +++-
 include/linux/miscdevice.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 2cf595d2e10b..7a768775e558 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -68,8 +68,10 @@ static int misc_minor_alloc(int minor)
 	int ret = 0;
 
 	if (minor == MISC_DYNAMIC_MINOR) {
+		int max = DYNAMIC_MINORS - 1 - MISC_STATIC_MAX_MINOR - 1;
 		/* allocate free id */
-		ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
+		/* Minors from 0 to 15 are reserved. */
+		ret = ida_alloc_max(&misc_minors_ida, max, GFP_KERNEL);
 		if (ret >= 0) {
 			ret = DYNAMIC_MINORS - ret - 1;
 		} else {
diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
index 69e110c2b86a..911a294d17b5 100644
--- a/include/linux/miscdevice.h
+++ b/include/linux/miscdevice.h
@@ -21,6 +21,7 @@
 #define APOLLO_MOUSE_MINOR	7	/* unused */
 #define PC110PAD_MINOR		9	/* unused */
 /*#define ADB_MOUSE_MINOR	10	FIXME OBSOLETE */
+#define MISC_STATIC_MAX_MINOR	15	/* Top of first reserved range */
 #define WATCHDOG_MINOR		130	/* Watchdog timer     */
 #define TEMP_MINOR		131	/* Temperature Sensor */
 #define APM_MINOR_DEV		134
-- 
2.34.1

[PATCH 4/4] char: misc: deallocate static minor in error path

[Thadeu Lima de Souza Cascardo]

When creating sysfs files fail, the allocated minor must be freed such that
it can be later reused. That is specially harmful for static minor numbers,
since those would always fail to register later on.

Fixes: 6d04d2b554b1 ("misc: misc_minor_alloc to use ida for all dynamic/misc dynamic minors")
Cc: stable@vger.kernel.org
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
---
 drivers/char/misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 7a768775e558..7843a1a34d64 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -266,8 +266,8 @@ int misc_register(struct miscdevice *misc)
 		device_create_with_groups(&misc_class, misc->parent, dev,
 					  misc, misc->groups, "%s", misc->name);
 	if (IS_ERR(misc->this_device)) {
+		misc_minor_free(misc->minor);
 		if (is_dynamic) {
-			misc_minor_free(misc->minor);
 			misc->minor = MISC_DYNAMIC_MINOR;
 		}
 		err = PTR_ERR(misc->this_device);
-- 
2.34.1

Re: [PATCH 4/4] char: misc: deallocate static minor in error path

[Greg Kroah-Hartman]

On Thu, Jan 23, 2025 at 09:32:49AM -0300, Thadeu Lima de Souza Cascardo wrote:
> When creating sysfs files fail, the allocated minor must be freed such that
> it can be later reused. That is specially harmful for static minor numbers,
> since those would always fail to register later on.
> 
> Fixes: 6d04d2b554b1 ("misc: misc_minor_alloc to use ida for all dynamic/misc dynamic minors")
> Cc: stable@vger.kernel.org
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> ---
>  drivers/char/misc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> index 7a768775e558..7843a1a34d64 100644
> --- a/drivers/char/misc.c
> +++ b/drivers/char/misc.c
> @@ -266,8 +266,8 @@ int misc_register(struct miscdevice *misc)
>  		device_create_with_groups(&misc_class, misc->parent, dev,
>  					  misc, misc->groups, "%s", misc->name);
>  	if (IS_ERR(misc->this_device)) {
> +		misc_minor_free(misc->minor);
>  		if (is_dynamic) {
> -			misc_minor_free(misc->minor);
>  			misc->minor = MISC_DYNAMIC_MINOR;
>  		}
>  		err = PTR_ERR(misc->this_device);
> -- 
> 2.34.1
> 
> 

Having a "fix" as patch 4/4 of a series is a pain, it should go to Linus
now, not for the next merge window, right?  I'll split this apart by
hand, but ugh, please be more careful next time.

thanks,

greg k-h

Re: [PATCH 3/4] char: misc: restrict the dynamic range to exclude reserved minors

[Greg Kroah-Hartman]

On Thu, Jan 23, 2025 at 09:32:48AM -0300, Thadeu Lima de Souza Cascardo wrote:
> When this was first reported [1], the possibility of having sufficient
> number of dynamic misc devices was theoretical.
> 
> What we know from commit ab760791c0cf ("char: misc: Increase the maximum
> number of dynamic misc devices to 1048448"), is that the miscdevice
> interface has been used for allocating more than the single-shot devices it
> was designed for.

Do we have any in-kernel drivers that abuse it this way?  If so, let's
fix them up.

> On such systems, it is certain that the dynamic allocation will allocate
> certain reserved minor numbers, leading to failures when a later driver
> tries to claim its reserved number.
> 
> Fixing this is a simple matter of defining the IDA range to allocate from
> to exclude minors up to and including 15.
> 
> [1] https://lore.kernel.org/all/1257813017-28598-3-git-send-email-cascardo@holoscopio.com/
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> ---
>  drivers/char/misc.c        | 4 +++-
>  include/linux/miscdevice.h | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> index 2cf595d2e10b..7a768775e558 100644
> --- a/drivers/char/misc.c
> +++ b/drivers/char/misc.c
> @@ -68,8 +68,10 @@ static int misc_minor_alloc(int minor)
>  	int ret = 0;
>  
>  	if (minor == MISC_DYNAMIC_MINOR) {
> +		int max = DYNAMIC_MINORS - 1 - MISC_STATIC_MAX_MINOR - 1;
>  		/* allocate free id */
> -		ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
> +		/* Minors from 0 to 15 are reserved. */
> +		ret = ida_alloc_max(&misc_minors_ida, max, GFP_KERNEL);
>  		if (ret >= 0) {
>  			ret = DYNAMIC_MINORS - ret - 1;
>  		} else {
> diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
> index 69e110c2b86a..911a294d17b5 100644
> --- a/include/linux/miscdevice.h
> +++ b/include/linux/miscdevice.h
> @@ -21,6 +21,7 @@
>  #define APOLLO_MOUSE_MINOR	7	/* unused */
>  #define PC110PAD_MINOR		9	/* unused */
>  /*#define ADB_MOUSE_MINOR	10	FIXME OBSOLETE */
> +#define MISC_STATIC_MAX_MINOR	15	/* Top of first reserved range */

I don't understand, why is 15 the magic number here?  All of those
"unused" values can just be removed, all systems should be using dynamic
/dev/ now for many many years, and even if they aren't, these minors
aren't being used by anyone else as the in-kernel users are long gone.

So why are we reserving this range if no one needs it?

confused,

greg k-h

Re: [PATCH 2/4] char: misc: add test cases

[Greg Kroah-Hartman]

On Thu, Jan 23, 2025 at 09:32:47AM -0300, Thadeu Lima de Souza Cascardo wrote:
> Add test cases for static and dynamic minor number allocation and
> deallocation.
> 
> While at it, improve description and test suite name.
> 
> Some of the cases include:
> 
> - that static and dynamic allocation reserved the expected minors.
> 
> - that registering duplicate minors or duplicate names will fail.
> 
> - that failing to create a sysfs file (due to duplicate names) will
>   deallocate the dynamic minor correctly.
> 
> - that dynamic allocation does not allocate a minor number in the static
>   range.
> 
> - that there are no collisions when mixing dynamic and static allocations.
> 
> - that registering a static number in the dynamic range won't conflict with
>   a dynamic allocation.
> 
> This last test verifies the bug fixed by commit 6d04d2b554b1 ("misc:
> misc_minor_alloc to use ida for all dynamic/misc dynamic minors") has not
> regressed.
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> ---
>  drivers/misc/misc_minor_kunit.c | 515 +++++++++++++++++++++++++++++++-
>  1 file changed, 513 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/misc_minor_kunit.c b/drivers/misc/misc_minor_kunit.c
> index 293e0fb7e43e..84e13bc5c61c 100644
> --- a/drivers/misc/misc_minor_kunit.c
> +++ b/drivers/misc/misc_minor_kunit.c
> @@ -51,19 +51,530 @@ static void kunit_misc_dynamic_minor(struct kunit *test)
>  	misc_deregister(&dev_misc_dynamic_minor);
>  }
>  
> +struct miscdev_test_case {
> +	const char *str;
> +	int minor;
> +};
> +
> +static struct miscdev_test_case miscdev_test_ranges[] = {
> +	{
> +		.str = "lower static range, top",
> +		.minor = 15,

As I don't agree with the previous patch, I'll not apply this one.

How about at least just redoing this one and using the values that the
code currently allocates?

thanks,

greg k-h

Re: [PATCH 3/4] char: misc: restrict the dynamic range to exclude reserved minors

[Thadeu Lima de Souza Cascardo]

On Thu, Feb 20, 2025 at 03:31:11PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Jan 23, 2025 at 09:32:48AM -0300, Thadeu Lima de Souza Cascardo wrote:
> > When this was first reported [1], the possibility of having sufficient
> > number of dynamic misc devices was theoretical.
> > 
> > What we know from commit ab760791c0cf ("char: misc: Increase the maximum
> > number of dynamic misc devices to 1048448"), is that the miscdevice
> > interface has been used for allocating more than the single-shot devices it
> > was designed for.
> 
> Do we have any in-kernel drivers that abuse it this way?  If so, let's
> fix them up.
> 

From the discussion 15 years ago, though found only by code inspection, dlm
was theoretically capable of creating such multiple devices. But, in
practice, its user space never did create more than one. 

But from commit ab760791c0cf ("char: misc: Increase the maximum number of
dynamic misc devices to 1048448") description, we know at least
coresight_tmc is abusing it like that.

I can work on fixing coresight_tmc and any other abusers, but they will
require their own class (though I thought about making it possible to
create compatibility symlinks under /sys/class/misc/). So, this should take
a bit longer. In the meantime, I think we shold apply something like this
patch for v6.15 and even consider it for stable.

> > On such systems, it is certain that the dynamic allocation will allocate
> > certain reserved minor numbers, leading to failures when a later driver
> > tries to claim its reserved number.
> > 
> > Fixing this is a simple matter of defining the IDA range to allocate from
> > to exclude minors up to and including 15.
> > 
> > [1] https://lore.kernel.org/all/1257813017-28598-3-git-send-email-cascardo@holoscopio.com/
> > 
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> > ---
> >  drivers/char/misc.c        | 4 +++-
> >  include/linux/miscdevice.h | 1 +
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> > index 2cf595d2e10b..7a768775e558 100644
> > --- a/drivers/char/misc.c
> > +++ b/drivers/char/misc.c
> > @@ -68,8 +68,10 @@ static int misc_minor_alloc(int minor)
> >  	int ret = 0;
> >  
> >  	if (minor == MISC_DYNAMIC_MINOR) {
> > +		int max = DYNAMIC_MINORS - 1 - MISC_STATIC_MAX_MINOR - 1;
> >  		/* allocate free id */
> > -		ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
> > +		/* Minors from 0 to 15 are reserved. */
> > +		ret = ida_alloc_max(&misc_minors_ida, max, GFP_KERNEL);
> >  		if (ret >= 0) {
> >  			ret = DYNAMIC_MINORS - ret - 1;
> >  		} else {
> > diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
> > index 69e110c2b86a..911a294d17b5 100644
> > --- a/include/linux/miscdevice.h
> > +++ b/include/linux/miscdevice.h
> > @@ -21,6 +21,7 @@
> >  #define APOLLO_MOUSE_MINOR	7	/* unused */
> >  #define PC110PAD_MINOR		9	/* unused */
> >  /*#define ADB_MOUSE_MINOR	10	FIXME OBSOLETE */
> > +#define MISC_STATIC_MAX_MINOR	15	/* Top of first reserved range */
> 
> I don't understand, why is 15 the magic number here?  All of those
> "unused" values can just be removed, all systems should be using dynamic
> /dev/ now for many many years, and even if they aren't, these minors
> aren't being used by anyone else as the in-kernel users are long gone.
> 
> So why are we reserving this range if no one needs it?

Because those were reserved historically. They are still documented under
Documentation/admin-guide/devices.txt. What do we gain by not reserving
those? Since we moved past 255 for dynamic allocation, we are not going to
miss those 15 minors.

One alternative is that we just disregard the 0-255 range entirely for
dynamic allocation, which should also simplify the code a lot. If that
would be preferable, I can work on that and submit it soon.

Thanks.
Cascardo.

> 
> confused,
> 
> greg k-h
> 

Re: [PATCH 3/4] char: misc: restrict the dynamic range to exclude reserved minors

[Greg Kroah-Hartman]

On Thu, Feb 20, 2025 at 11:52:31AM -0300, Thadeu Lima de Souza Cascardo wrote:
> On Thu, Feb 20, 2025 at 03:31:11PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Jan 23, 2025 at 09:32:48AM -0300, Thadeu Lima de Souza Cascardo wrote:
> > > When this was first reported [1], the possibility of having sufficient
> > > number of dynamic misc devices was theoretical.
> > > 
> > > What we know from commit ab760791c0cf ("char: misc: Increase the maximum
> > > number of dynamic misc devices to 1048448"), is that the miscdevice
> > > interface has been used for allocating more than the single-shot devices it
> > > was designed for.
> > 
> > Do we have any in-kernel drivers that abuse it this way?  If so, let's
> > fix them up.
> > 
> 
> >From the discussion 15 years ago, though found only by code inspection, dlm
> was theoretically capable of creating such multiple devices. But, in
> practice, its user space never did create more than one. 
> 
> But from commit ab760791c0cf ("char: misc: Increase the maximum number of
> dynamic misc devices to 1048448") description, we know at least
> coresight_tmc is abusing it like that.

Ick.

> I can work on fixing coresight_tmc and any other abusers, but they will
> require their own class (though I thought about making it possible to
> create compatibility symlinks under /sys/class/misc/). So, this should take
> a bit longer. In the meantime, I think we shold apply something like this
> patch for v6.15 and even consider it for stable.

No such need for compatibility symlinks, devices should be able to be
found in the correct location no matter where they are in the system,
right?

But yes, a class is needed, thanks!

> > > On such systems, it is certain that the dynamic allocation will allocate
> > > certain reserved minor numbers, leading to failures when a later driver
> > > tries to claim its reserved number.
> > > 
> > > Fixing this is a simple matter of defining the IDA range to allocate from
> > > to exclude minors up to and including 15.
> > > 
> > > [1] https://lore.kernel.org/all/1257813017-28598-3-git-send-email-cascardo@holoscopio.com/
> > > 
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> > > ---
> > >  drivers/char/misc.c        | 4 +++-
> > >  include/linux/miscdevice.h | 1 +
> > >  2 files changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> > > index 2cf595d2e10b..7a768775e558 100644
> > > --- a/drivers/char/misc.c
> > > +++ b/drivers/char/misc.c
> > > @@ -68,8 +68,10 @@ static int misc_minor_alloc(int minor)
> > >  	int ret = 0;
> > >  
> > >  	if (minor == MISC_DYNAMIC_MINOR) {
> > > +		int max = DYNAMIC_MINORS - 1 - MISC_STATIC_MAX_MINOR - 1;
> > >  		/* allocate free id */
> > > -		ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
> > > +		/* Minors from 0 to 15 are reserved. */
> > > +		ret = ida_alloc_max(&misc_minors_ida, max, GFP_KERNEL);
> > >  		if (ret >= 0) {
> > >  			ret = DYNAMIC_MINORS - ret - 1;
> > >  		} else {
> > > diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
> > > index 69e110c2b86a..911a294d17b5 100644
> > > --- a/include/linux/miscdevice.h
> > > +++ b/include/linux/miscdevice.h
> > > @@ -21,6 +21,7 @@
> > >  #define APOLLO_MOUSE_MINOR	7	/* unused */
> > >  #define PC110PAD_MINOR		9	/* unused */
> > >  /*#define ADB_MOUSE_MINOR	10	FIXME OBSOLETE */
> > > +#define MISC_STATIC_MAX_MINOR	15	/* Top of first reserved range */
> > 
> > I don't understand, why is 15 the magic number here?  All of those
> > "unused" values can just be removed, all systems should be using dynamic
> > /dev/ now for many many years, and even if they aren't, these minors
> > aren't being used by anyone else as the in-kernel users are long gone.
> > 
> > So why are we reserving this range if no one needs it?
> 
> Because those were reserved historically. They are still documented under
> Documentation/admin-guide/devices.txt. What do we gain by not reserving
> those? Since we moved past 255 for dynamic allocation, we are not going to
> miss those 15 minors.

True, but it turns out we don't need to reserve them anymore, so let's
not.

> One alternative is that we just disregard the 0-255 range entirely for
> dynamic allocation, which should also simplify the code a lot. If that
> would be preferable, I can work on that and submit it soon.

I'm all for that, that might just make things much simpler, as a dynamic
number shouldn't care what it's set at, right?  Try it and see?

thanks,

greg k-h