drivers/char/misc.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-)
misc_minor_alloc was allocating id for minor only in case of
MISC_DYNAMIC_MINOR but misc_minor_free was always freeing ids
using ida_free causing a mismatch and following
warn:
> > WARNING: CPU: 0 PID: 159 at lib/idr.c:525 ida_free+0x3e0/0x41f
> > ida_free called for id=127 which is not allocated.
> > <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
...
> > [<60941eb4>] ida_free+0x3e0/0x41f
> > [<605ac993>] misc_minor_free+0x3e/0xbc
> > [<605acb82>] misc_deregister+0x171/0x1b3
misc_minor_alloc is changed to allocate id from ida for all dynamic/
misc dynamic minors
Signed-off-by: Vimal Agrawal <vimal.agrawal@sophos.com>
---
drivers/char/misc.c | 33 ++++++++++++++++++++++++++-------
1 file changed, 26 insertions(+), 7 deletions(-)
diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 541edc26ec89..fbe51e776c15 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -63,16 +63,28 @@ static DEFINE_MUTEX(misc_mtx);
#define DYNAMIC_MINORS 128 /* like dynamic majors */
static DEFINE_IDA(misc_minors_ida);
-static int misc_minor_alloc(void)
+static int misc_minor_alloc(int minor)
{
int ret;
- ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
- if (ret >= 0) {
- ret = DYNAMIC_MINORS - ret - 1;
- } else {
- ret = ida_alloc_range(&misc_minors_ida, MISC_DYNAMIC_MINOR + 1,
+ if (minor == MISC_DYNAMIC_MINOR) {
+ /* allocate free id */
+ ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
+ if (ret >= 0) {
+ ret = DYNAMIC_MINORS - ret - 1;
+ } else {
+ ret = ida_alloc_range(&misc_minors_ida, MISC_DYNAMIC_MINOR + 1,
MINORMASK, GFP_KERNEL);
+ }
+ } else {
+ /* specific minor, check if it is in dynamic or misc dynamic range */
+ if (minor < DYNAMIC_MINORS) {
+ minor = DYNAMIC_MINORS - minor - 1;
+ ret = ida_alloc_range(&misc_minors_ida, minor, minor, GFP_KERNEL);
+ }
+
+ if (minor > MISC_DYNAMIC_MINOR)
+ ret = ida_alloc_range(&misc_minors_ida, minor, minor, GFP_KERNEL);
}
return ret;
}
@@ -219,7 +231,7 @@ int misc_register(struct miscdevice *misc)
mutex_lock(&misc_mtx);
if (is_dynamic) {
- int i = misc_minor_alloc();
+ int i = misc_minor_alloc(misc->minor);
if (i < 0) {
err = -EBUSY;
@@ -228,6 +240,7 @@ int misc_register(struct miscdevice *misc)
misc->minor = i;
} else {
struct miscdevice *c;
+ int i;
list_for_each_entry(c, &misc_list, list) {
if (c->minor == misc->minor) {
@@ -235,6 +248,12 @@ int misc_register(struct miscdevice *misc)
goto out;
}
}
+
+ i = misc_minor_alloc(misc->minor);
+ if (i < 0) {
+ err = -EBUSY;
+ goto out;
+ }
}
dev = MKDEV(MISC_MAJOR, misc->minor);
--
2.17.1Hi Vimal,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Vimal-Agrawal/misc-misc_minor_alloc-to-allocate-ids-for-all-dynamic-misc-dynamic-minors/20241014-211915
base: char-misc/char-misc-testing
patch link: https://lore.kernel.org/r/20241014131416.27324-1-vimal.agrawal%40sophos.com
patch subject: [PATCH] misc: misc_minor_alloc to allocate ids for all dynamic/misc dynamic minors
config: sparc64-randconfig-r072-20241015 (https://download.01.org/0day-ci/archive/20241016/202410161811.aIPEJHOt-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 14.1.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202410161811.aIPEJHOt-lkp@intel.com/
smatch warnings:
drivers/char/misc.c:89 misc_minor_alloc() error: uninitialized symbol 'ret'.
vim +/ret +89 drivers/char/misc.c
d52c84545e305b Vimal Agrawal 2024-10-14 66 static int misc_minor_alloc(int minor)
ab760791c0cfbb D Scott Phillips 2022-11-14 67 {
ab760791c0cfbb D Scott Phillips 2022-11-14 68 int ret;
ab760791c0cfbb D Scott Phillips 2022-11-14 69
d52c84545e305b Vimal Agrawal 2024-10-14 70 if (minor == MISC_DYNAMIC_MINOR) {
d52c84545e305b Vimal Agrawal 2024-10-14 71 /* allocate free id */
ab760791c0cfbb D Scott Phillips 2022-11-14 72 ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
ab760791c0cfbb D Scott Phillips 2022-11-14 73 if (ret >= 0) {
ab760791c0cfbb D Scott Phillips 2022-11-14 74 ret = DYNAMIC_MINORS - ret - 1;
ab760791c0cfbb D Scott Phillips 2022-11-14 75 } else {
ab760791c0cfbb D Scott Phillips 2022-11-14 76 ret = ida_alloc_range(&misc_minors_ida, MISC_DYNAMIC_MINOR + 1,
ab760791c0cfbb D Scott Phillips 2022-11-14 77 MINORMASK, GFP_KERNEL);
ab760791c0cfbb D Scott Phillips 2022-11-14 78 }
d52c84545e305b Vimal Agrawal 2024-10-14 79 } else {
d52c84545e305b Vimal Agrawal 2024-10-14 80 /* specific minor, check if it is in dynamic or misc dynamic range */
d52c84545e305b Vimal Agrawal 2024-10-14 81 if (minor < DYNAMIC_MINORS) {
d52c84545e305b Vimal Agrawal 2024-10-14 82 minor = DYNAMIC_MINORS - minor - 1;
d52c84545e305b Vimal Agrawal 2024-10-14 83 ret = ida_alloc_range(&misc_minors_ida, minor, minor, GFP_KERNEL);
d52c84545e305b Vimal Agrawal 2024-10-14 84 }
d52c84545e305b Vimal Agrawal 2024-10-14 85
d52c84545e305b Vimal Agrawal 2024-10-14 86 if (minor > MISC_DYNAMIC_MINOR)
d52c84545e305b Vimal Agrawal 2024-10-14 87 ret = ida_alloc_range(&misc_minors_ida, minor, minor, GFP_KERNEL);
What about if minor is >= DYNAMIC_MINORS (128) but <= MISC_DYNAMIC_MINOR (255)?
d52c84545e305b Vimal Agrawal 2024-10-14 88 }
ab760791c0cfbb D Scott Phillips 2022-11-14 @89 return ret;
ab760791c0cfbb D Scott Phillips 2022-11-14 90 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Dan,
Both warning and missed case for return from this function for static
minor are already fixed in the v2 version of the patch. I will be
sending out the v3 version shortly ( after splitting it from kunit
changes).
Thanks for pointing this out.
Vimal
On Wed, Oct 16, 2024 at 7:52 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> Hi Vimal,
>
> kernel test robot noticed the following build warnings:
>
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Vimal-Agrawal/misc-misc_minor_alloc-to-allocate-ids-for-all-dynamic-misc-dynamic-minors/20241014-211915
> base: char-misc/char-misc-testing
> patch link: https://lore.kernel.org/r/20241014131416.27324-1-vimal.agrawal%40sophos.com
> patch subject: [PATCH] misc: misc_minor_alloc to allocate ids for all dynamic/misc dynamic minors
> config: sparc64-randconfig-r072-20241015 (https://download.01.org/0day-ci/archive/20241016/202410161811.aIPEJHOt-lkp@intel.com/config)
> compiler: sparc64-linux-gcc (GCC) 14.1.0
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202410161811.aIPEJHOt-lkp@intel.com/
>
> smatch warnings:
> drivers/char/misc.c:89 misc_minor_alloc() error: uninitialized symbol 'ret'.
>
> vim +/ret +89 drivers/char/misc.c
>
> d52c84545e305b Vimal Agrawal 2024-10-14 66 static int misc_minor_alloc(int minor)
> ab760791c0cfbb D Scott Phillips 2022-11-14 67 {
> ab760791c0cfbb D Scott Phillips 2022-11-14 68 int ret;
> ab760791c0cfbb D Scott Phillips 2022-11-14 69
> d52c84545e305b Vimal Agrawal 2024-10-14 70 if (minor == MISC_DYNAMIC_MINOR) {
> d52c84545e305b Vimal Agrawal 2024-10-14 71 /* allocate free id */
> ab760791c0cfbb D Scott Phillips 2022-11-14 72 ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
> ab760791c0cfbb D Scott Phillips 2022-11-14 73 if (ret >= 0) {
> ab760791c0cfbb D Scott Phillips 2022-11-14 74 ret = DYNAMIC_MINORS - ret - 1;
> ab760791c0cfbb D Scott Phillips 2022-11-14 75 } else {
> ab760791c0cfbb D Scott Phillips 2022-11-14 76 ret = ida_alloc_range(&misc_minors_ida, MISC_DYNAMIC_MINOR + 1,
> ab760791c0cfbb D Scott Phillips 2022-11-14 77 MINORMASK, GFP_KERNEL);
> ab760791c0cfbb D Scott Phillips 2022-11-14 78 }
> d52c84545e305b Vimal Agrawal 2024-10-14 79 } else {
> d52c84545e305b Vimal Agrawal 2024-10-14 80 /* specific minor, check if it is in dynamic or misc dynamic range */
> d52c84545e305b Vimal Agrawal 2024-10-14 81 if (minor < DYNAMIC_MINORS) {
> d52c84545e305b Vimal Agrawal 2024-10-14 82 minor = DYNAMIC_MINORS - minor - 1;
> d52c84545e305b Vimal Agrawal 2024-10-14 83 ret = ida_alloc_range(&misc_minors_ida, minor, minor, GFP_KERNEL);
> d52c84545e305b Vimal Agrawal 2024-10-14 84 }
> d52c84545e305b Vimal Agrawal 2024-10-14 85
> d52c84545e305b Vimal Agrawal 2024-10-14 86 if (minor > MISC_DYNAMIC_MINOR)
> d52c84545e305b Vimal Agrawal 2024-10-14 87 ret = ida_alloc_range(&misc_minors_ida, minor, minor, GFP_KERNEL);
>
> What about if minor is >= DYNAMIC_MINORS (128) but <= MISC_DYNAMIC_MINOR (255)?
>
> d52c84545e305b Vimal Agrawal 2024-10-14 88 }
> ab760791c0cfbb D Scott Phillips 2022-11-14 @89 return ret;
> ab760791c0cfbb D Scott Phillips 2022-11-14 90 }
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>
On Mon, Oct 14, 2024 at 01:14:16PM +0000, Vimal Agrawal wrote: > misc_minor_alloc was allocating id for minor only in case of > MISC_DYNAMIC_MINOR but misc_minor_free was always freeing ids > using ida_free causing a mismatch and following > warn: > > > WARNING: CPU: 0 PID: 159 at lib/idr.c:525 ida_free+0x3e0/0x41f > > > ida_free called for id=127 which is not allocated. > > > <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< > ... > > > [<60941eb4>] ida_free+0x3e0/0x41f > > > [<605ac993>] misc_minor_free+0x3e/0xbc > > > [<605acb82>] misc_deregister+0x171/0x1b3 > > misc_minor_alloc is changed to allocate id from ida for all dynamic/ > misc dynamic minors > > Signed-off-by: Vimal Agrawal <vimal.agrawal@sophos.com> What commit id does this fix? And I think we need a test for this somewhere, care to write a kunit test? thanks, greg k-h
misc_minor_alloc was allocating id using ida for minor only in case of
MISC_DYNAMIC_MINOR but misc_minor_free was always freeing ids
using ida_free causing a mismatch and following warn:
> > WARNING: CPU: 0 PID: 159 at lib/idr.c:525 ida_free+0x3e0/0x41f
> > ida_free called for id=127 which is not allocated.
> > <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
...
> > [<60941eb4>] ida_free+0x3e0/0x41f
> > [<605ac993>] misc_minor_free+0x3e/0xbc
> > [<605acb82>] misc_deregister+0x171/0x1b3
misc_minor_alloc is changed to allocate id from ida for all minors
falling in the range of dynamic/ misc dynamic minors
Fixes: 0ad35fed618c ("char: misc: Increase the maximum number of dynamic misc devices to 1048448")
Signed-off-by: Vimal Agrawal <vimal.agrawal@sophos.com>
---
drivers/char/misc.c | 35 +++++++++++++++++-----
lib/Kconfig.debug | 11 +++++++
lib/Makefile | 1 +
lib/test_misc_minor.c | 67 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 107 insertions(+), 7 deletions(-)
create mode 100644 lib/test_misc_minor.c
diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 541edc26ec89..9d0cd3459b4f 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -63,16 +63,30 @@ static DEFINE_MUTEX(misc_mtx);
#define DYNAMIC_MINORS 128 /* like dynamic majors */
static DEFINE_IDA(misc_minors_ida);
-static int misc_minor_alloc(void)
+static int misc_minor_alloc(int minor)
{
int ret;
- ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
- if (ret >= 0) {
- ret = DYNAMIC_MINORS - ret - 1;
- } else {
- ret = ida_alloc_range(&misc_minors_ida, MISC_DYNAMIC_MINOR + 1,
+ if (minor == MISC_DYNAMIC_MINOR) {
+ /* allocate free id */
+ ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
+ if (ret >= 0) {
+ ret = DYNAMIC_MINORS - ret - 1;
+ } else {
+ ret = ida_alloc_range(&misc_minors_ida, MISC_DYNAMIC_MINOR + 1,
MINORMASK, GFP_KERNEL);
+ }
+ } else {
+ /* specific minor, check if it is in dynamic or misc dynamic range */
+ if (minor < DYNAMIC_MINORS) {
+ minor = DYNAMIC_MINORS - minor - 1;
+ ret = ida_alloc_range(&misc_minors_ida, minor, minor, GFP_KERNEL);
+ } else if (minor > MISC_DYNAMIC_MINOR) {
+ ret = ida_alloc_range(&misc_minors_ida, minor, minor, GFP_KERNEL);
+ } else {
+ /* case of non-dynamic minors, no need to allocate id */
+ ret = 0;
+ }
}
return ret;
}
@@ -219,7 +233,7 @@ int misc_register(struct miscdevice *misc)
mutex_lock(&misc_mtx);
if (is_dynamic) {
- int i = misc_minor_alloc();
+ int i = misc_minor_alloc(misc->minor);
if (i < 0) {
err = -EBUSY;
@@ -228,6 +242,7 @@ int misc_register(struct miscdevice *misc)
misc->minor = i;
} else {
struct miscdevice *c;
+ int i;
list_for_each_entry(c, &misc_list, list) {
if (c->minor == misc->minor) {
@@ -235,6 +250,12 @@ int misc_register(struct miscdevice *misc)
goto out;
}
}
+
+ i = misc_minor_alloc(misc->minor);
+ if (i < 0) {
+ err = -EBUSY;
+ goto out;
+ }
}
dev = MKDEV(MISC_MAJOR, misc->minor);
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 7315f643817a..5a5d27284e0a 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2488,6 +2488,17 @@ config TEST_RHASHTABLE
config TEST_IDA
tristate "Perform selftest on IDA functions"
+config TEST_MISC_MINOR
+ tristate "Basic misc minor 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
+
+ If unsure, say N.
+
config TEST_PARMAN
tristate "Perform selftest on priority array manager"
depends on PARMAN
diff --git a/lib/Makefile b/lib/Makefile
index 773adf88af41..631d73f96f76 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o
obj-$(CONFIG_TEST_IOV_ITER) += kunit_iov_iter.o
obj-$(CONFIG_HASH_KUNIT_TEST) += test_hash.o
obj-$(CONFIG_TEST_IDA) += test_ida.o
+obj-$(CONFIG_TEST_MISC_MINOR) += test_misc_minor.o
obj-$(CONFIG_TEST_UBSAN) += test_ubsan.o
CFLAGS_test_ubsan.o += $(call cc-disable-warning, vla)
CFLAGS_test_ubsan.o += $(call cc-disable-warning, unused-but-set-variable)
diff --git a/lib/test_misc_minor.c b/lib/test_misc_minor.c
new file mode 100644
index 000000000000..bcec3fb1c46a
--- /dev/null
+++ b/lib/test_misc_minor.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <kunit/test.h>
+#include <kunit/test-bug.h>
+#include <linux/module.h>
+#include <linux/miscdevice.h>
+
+/* dynamic minor (2) */
+static struct miscdevice dev_dynamic_minor = {
+ .minor = 2,
+ .name = "dev_dynamic_minor",
+};
+
+/* static minor (LCD_MINOR) */
+static struct miscdevice dev_static_minor = {
+ .minor = LCD_MINOR,
+ .name = "dev_static_minor",
+};
+
+/* misc dynamic minor */
+static struct miscdevice dev_misc_dynamic_minor = {
+ .minor = MISC_DYNAMIC_MINOR,
+ .name = "dev_misc_dynamic_minor",
+};
+
+static void kunit_dynamic_minor(struct kunit *test)
+{
+ int ret;
+
+ ret=misc_register(&dev_dynamic_minor);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, 2, dev_dynamic_minor.minor);
+ misc_deregister(&dev_dynamic_minor);
+}
+
+static void kunit_static_minor(struct kunit *test)
+{
+ int ret;
+
+ ret=misc_register(&dev_static_minor);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, LCD_MINOR, dev_static_minor.minor);
+ misc_deregister(&dev_static_minor);
+}
+
+static void kunit_misc_dynamic_minor(struct kunit *test)
+{
+ int ret;
+
+ ret=misc_register(&dev_misc_dynamic_minor);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ misc_deregister(&dev_misc_dynamic_minor);
+}
+
+static struct kunit_case test_cases[] = {
+ KUNIT_CASE(kunit_dynamic_minor),
+ KUNIT_CASE(kunit_static_minor),
+ KUNIT_CASE(kunit_misc_dynamic_minor),
+ {}
+};
+
+static struct kunit_suite test_suite = {
+ .name = "misc_minor_test",
+ .test_cases = test_cases,
+};
+kunit_test_suite(test_suite);
+
+MODULE_LICENSE("GPL");
--
2.17.1On 10/15/24 00:02, Vimal Agrawal wrote:
...
> +static struct kunit_suite test_suite = {
> + .name = "misc_minor_test",
> + .test_cases = test_cases,
> +};
> +kunit_test_suite(test_suite);
> +
> +MODULE_LICENSE("GPL");
Since commit 1fffe7a34c89 ("script: modpost: emit a warning when the
description is missing"), a module without a MODULE_DESCRIPTION() will
result in a warning when built with make W=1. Recently, multiple
developers have been eradicating these warnings treewide, and very few
(if any) are left, so please don't introduce a new one :)
Please add the missing MODULE_DESCRIPTION()
/jeff
Hi Jeff,
Thanks. I will be adding MODULE_DESCRIPTION in the next version of the
patch. Will be splitting kunit changes from this patch in two patch
series.
Vimal
On Wed, Oct 16, 2024 at 3:48 AM Jeff Johnson <quic_jjohnson@quicinc.com> wrote:
>
> On 10/15/24 00:02, Vimal Agrawal wrote:
> ...
> > +static struct kunit_suite test_suite = {
> > + .name = "misc_minor_test",
> > + .test_cases = test_cases,
> > +};
> > +kunit_test_suite(test_suite);
> > +
> > +MODULE_LICENSE("GPL");
>
> Since commit 1fffe7a34c89 ("script: modpost: emit a warning when the
> description is missing"), a module without a MODULE_DESCRIPTION() will
> result in a warning when built with make W=1. Recently, multiple
> developers have been eradicating these warnings treewide, and very few
> (if any) are left, so please don't introduce a new one :)
>
> Please add the missing MODULE_DESCRIPTION()
>
> /jeff
basic kunit tests for misc minor
Signed-off-by: Vimal Agrawal <vimal.agrawal@sophos.com>
---
lib/Kconfig.debug | 11 +++++++
lib/Makefile | 1 +
lib/test_misc_minor.c | 69 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 81 insertions(+)
create mode 100644 lib/test_misc_minor.c
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 7315f643817a..5a5d27284e0a 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2488,6 +2488,17 @@ config TEST_RHASHTABLE
config TEST_IDA
tristate "Perform selftest on IDA functions"
+config TEST_MISC_MINOR
+ tristate "Basic misc minor 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
+
+ If unsure, say N.
+
config TEST_PARMAN
tristate "Perform selftest on priority array manager"
depends on PARMAN
diff --git a/lib/Makefile b/lib/Makefile
index 773adf88af41..631d73f96f76 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o
obj-$(CONFIG_TEST_IOV_ITER) += kunit_iov_iter.o
obj-$(CONFIG_HASH_KUNIT_TEST) += test_hash.o
obj-$(CONFIG_TEST_IDA) += test_ida.o
+obj-$(CONFIG_TEST_MISC_MINOR) += test_misc_minor.o
obj-$(CONFIG_TEST_UBSAN) += test_ubsan.o
CFLAGS_test_ubsan.o += $(call cc-disable-warning, vla)
CFLAGS_test_ubsan.o += $(call cc-disable-warning, unused-but-set-variable)
diff --git a/lib/test_misc_minor.c b/lib/test_misc_minor.c
new file mode 100644
index 000000000000..293e0fb7e43e
--- /dev/null
+++ b/lib/test_misc_minor.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <kunit/test.h>
+#include <kunit/test-bug.h>
+#include <linux/module.h>
+#include <linux/miscdevice.h>
+
+/* dynamic minor (2) */
+static struct miscdevice dev_dynamic_minor = {
+ .minor = 2,
+ .name = "dev_dynamic_minor",
+};
+
+/* static minor (LCD_MINOR) */
+static struct miscdevice dev_static_minor = {
+ .minor = LCD_MINOR,
+ .name = "dev_static_minor",
+};
+
+/* misc dynamic minor */
+static struct miscdevice dev_misc_dynamic_minor = {
+ .minor = MISC_DYNAMIC_MINOR,
+ .name = "dev_misc_dynamic_minor",
+};
+
+static void kunit_dynamic_minor(struct kunit *test)
+{
+ int ret;
+
+ ret = misc_register(&dev_dynamic_minor);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, 2, dev_dynamic_minor.minor);
+ misc_deregister(&dev_dynamic_minor);
+}
+
+static void kunit_static_minor(struct kunit *test)
+{
+ int ret;
+
+ ret = misc_register(&dev_static_minor);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, LCD_MINOR, dev_static_minor.minor);
+ misc_deregister(&dev_static_minor);
+}
+
+static void kunit_misc_dynamic_minor(struct kunit *test)
+{
+ int ret;
+
+ ret = misc_register(&dev_misc_dynamic_minor);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ misc_deregister(&dev_misc_dynamic_minor);
+}
+
+static struct kunit_case test_cases[] = {
+ KUNIT_CASE(kunit_dynamic_minor),
+ KUNIT_CASE(kunit_static_minor),
+ KUNIT_CASE(kunit_misc_dynamic_minor),
+ {}
+};
+
+static struct kunit_suite test_suite = {
+ .name = "misc_minor_test",
+ .test_cases = test_cases,
+};
+kunit_test_suite(test_suite);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Vimal Agrawal");
+MODULE_DESCRIPTION("misc minor testing");
--
2.17.1On Thu, Oct 17, 2024 at 11:43:28AM +0000, Vimal Agrawal wrote: > basic kunit tests for misc minor > > Signed-off-by: Vimal Agrawal <vimal.agrawal@sophos.com> Your "From:" line says: From: Vimal Agrawal <avimalin@gmail.com> Which does not match your signed-off-by line :(
On Thu, Oct 17, 2024 at 11:43:28AM +0000, Vimal Agrawal wrote: > basic kunit tests for misc minor > > Signed-off-by: Vimal Agrawal <vimal.agrawal@sophos.com> Why is this showing up before patch 1/2? And the version is for the whole series, not for the individual patches. And this _is_ a v2, it is split from the previous one, so please document it as such. Please do a v3 with all of this fixed up. thanks, greg k-h
misc_minor_alloc was allocating id using ida for minor only in case of
MISC_DYNAMIC_MINOR but misc_minor_free was always freeing ids
using ida_free causing a mismatch and following warn:
> > WARNING: CPU: 0 PID: 159 at lib/idr.c:525 ida_free+0x3e0/0x41f
> > ida_free called for id=127 which is not allocated.
> > <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
...
> > [<60941eb4>] ida_free+0x3e0/0x41f
> > [<605ac993>] misc_minor_free+0x3e/0xbc
> > [<605acb82>] misc_deregister+0x171/0x1b3
misc_minor_alloc is changed to allocate id from ida for all minors
falling in the range of dynamic/ misc dynamic minors
Fixes: ab760791c0cf ("char: misc: Increase the maximum number of dynamic misc devices to 1048448")
Signed-off-by: Vimal Agrawal <avimalin@gmail.com>
Cc: stable@vger.kernel.org
---
v2: Added Fixes:
added missed case for static minor in misc_minor_alloc
v3: Removed kunit changes as that will be added as second patch in this two patch series
v4: Updated Signed-off-by: to match from:
drivers/char/misc.c | 39 ++++++++++++++++++++++++++++++---------
1 file changed, 30 insertions(+), 9 deletions(-)
diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 541edc26ec89..2cf595d2e10b 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -63,16 +63,30 @@ static DEFINE_MUTEX(misc_mtx);
#define DYNAMIC_MINORS 128 /* like dynamic majors */
static DEFINE_IDA(misc_minors_ida);
-static int misc_minor_alloc(void)
+static int misc_minor_alloc(int minor)
{
- int ret;
-
- ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
- if (ret >= 0) {
- ret = DYNAMIC_MINORS - ret - 1;
+ int ret = 0;
+
+ if (minor == MISC_DYNAMIC_MINOR) {
+ /* allocate free id */
+ ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
+ if (ret >= 0) {
+ ret = DYNAMIC_MINORS - ret - 1;
+ } else {
+ ret = ida_alloc_range(&misc_minors_ida, MISC_DYNAMIC_MINOR + 1,
+ MINORMASK, GFP_KERNEL);
+ }
} else {
- ret = ida_alloc_range(&misc_minors_ida, MISC_DYNAMIC_MINOR + 1,
- MINORMASK, GFP_KERNEL);
+ /* specific minor, check if it is in dynamic or misc dynamic range */
+ if (minor < DYNAMIC_MINORS) {
+ minor = DYNAMIC_MINORS - minor - 1;
+ ret = ida_alloc_range(&misc_minors_ida, minor, minor, GFP_KERNEL);
+ } else if (minor > MISC_DYNAMIC_MINOR) {
+ ret = ida_alloc_range(&misc_minors_ida, minor, minor, GFP_KERNEL);
+ } else {
+ /* case of non-dynamic minors, no need to allocate id */
+ ret = 0;
+ }
}
return ret;
}
@@ -219,7 +233,7 @@ int misc_register(struct miscdevice *misc)
mutex_lock(&misc_mtx);
if (is_dynamic) {
- int i = misc_minor_alloc();
+ int i = misc_minor_alloc(misc->minor);
if (i < 0) {
err = -EBUSY;
@@ -228,6 +242,7 @@ int misc_register(struct miscdevice *misc)
misc->minor = i;
} else {
struct miscdevice *c;
+ int i;
list_for_each_entry(c, &misc_list, list) {
if (c->minor == misc->minor) {
@@ -235,6 +250,12 @@ int misc_register(struct miscdevice *misc)
goto out;
}
}
+
+ i = misc_minor_alloc(misc->minor);
+ if (i < 0) {
+ err = -EBUSY;
+ goto out;
+ }
}
dev = MKDEV(MISC_MAJOR, misc->minor);
--
2.17.1On Thu, Oct 17, 2024 at 01:35:32PM +0000, Vimal Agrawal wrote:
> misc_minor_alloc was allocating id using ida for minor only in case of
> MISC_DYNAMIC_MINOR but misc_minor_free was always freeing ids
> using ida_free causing a mismatch and following warn:
> > > WARNING: CPU: 0 PID: 159 at lib/idr.c:525 ida_free+0x3e0/0x41f
> > > ida_free called for id=127 which is not allocated.
> > > <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> ...
> > > [<60941eb4>] ida_free+0x3e0/0x41f
> > > [<605ac993>] misc_minor_free+0x3e/0xbc
> > > [<605acb82>] misc_deregister+0x171/0x1b3
>
> misc_minor_alloc is changed to allocate id from ida for all minors
> falling in the range of dynamic/ misc dynamic minors
>
> Fixes: ab760791c0cf ("char: misc: Increase the maximum number of dynamic misc devices to 1048448")
> Signed-off-by: Vimal Agrawal <avimalin@gmail.com>
Sorry, but no, do not hide behind a gmail.com address. Either fix your
corporate email system to be able to send patches out, or use the other
method of sending from a different address as documented in the kernel
documentation.
As it is, I can't take this, sorry.
greg k-h
On Thu, Oct 17, 2024 at 03:39:06PM +0200, Greg KH wrote:
> On Thu, Oct 17, 2024 at 01:35:32PM +0000, Vimal Agrawal wrote:
> > misc_minor_alloc was allocating id using ida for minor only in case of
> > MISC_DYNAMIC_MINOR but misc_minor_free was always freeing ids
> > using ida_free causing a mismatch and following warn:
> > > > WARNING: CPU: 0 PID: 159 at lib/idr.c:525 ida_free+0x3e0/0x41f
> > > > ida_free called for id=127 which is not allocated.
> > > > <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> > ...
> > > > [<60941eb4>] ida_free+0x3e0/0x41f
> > > > [<605ac993>] misc_minor_free+0x3e/0xbc
> > > > [<605acb82>] misc_deregister+0x171/0x1b3
> >
> > misc_minor_alloc is changed to allocate id from ida for all minors
> > falling in the range of dynamic/ misc dynamic minors
> >
> > Fixes: ab760791c0cf ("char: misc: Increase the maximum number of dynamic misc devices to 1048448")
> > Signed-off-by: Vimal Agrawal <avimalin@gmail.com>
>
> Sorry, but no, do not hide behind a gmail.com address. Either fix your
> corporate email system to be able to send patches out, or use the other
> method of sending from a different address as documented in the kernel
> documentation.
>
> As it is, I can't take this, sorry.
Also, only patch 1/2 showed up, what happened to patch 2/2?
thanks,
greg k-h
basic kunit tests for misc minor
Signed-off-by: Vimal Agrawal <avimalin@gmail.com>
---
v2: Split from previous patch
v3:
v4: Match patch version for whole patch series
lib/Kconfig.debug | 11 +++++++
lib/Makefile | 1 +
lib/test_misc_minor.c | 69 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 81 insertions(+)
create mode 100644 lib/test_misc_minor.c
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 7315f643817a..5a5d27284e0a 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2488,6 +2488,17 @@ config TEST_RHASHTABLE
config TEST_IDA
tristate "Perform selftest on IDA functions"
+config TEST_MISC_MINOR
+ tristate "Basic misc minor 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
+
+ If unsure, say N.
+
config TEST_PARMAN
tristate "Perform selftest on priority array manager"
depends on PARMAN
diff --git a/lib/Makefile b/lib/Makefile
index 773adf88af41..631d73f96f76 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o
obj-$(CONFIG_TEST_IOV_ITER) += kunit_iov_iter.o
obj-$(CONFIG_HASH_KUNIT_TEST) += test_hash.o
obj-$(CONFIG_TEST_IDA) += test_ida.o
+obj-$(CONFIG_TEST_MISC_MINOR) += test_misc_minor.o
obj-$(CONFIG_TEST_UBSAN) += test_ubsan.o
CFLAGS_test_ubsan.o += $(call cc-disable-warning, vla)
CFLAGS_test_ubsan.o += $(call cc-disable-warning, unused-but-set-variable)
diff --git a/lib/test_misc_minor.c b/lib/test_misc_minor.c
new file mode 100644
index 000000000000..293e0fb7e43e
--- /dev/null
+++ b/lib/test_misc_minor.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <kunit/test.h>
+#include <kunit/test-bug.h>
+#include <linux/module.h>
+#include <linux/miscdevice.h>
+
+/* dynamic minor (2) */
+static struct miscdevice dev_dynamic_minor = {
+ .minor = 2,
+ .name = "dev_dynamic_minor",
+};
+
+/* static minor (LCD_MINOR) */
+static struct miscdevice dev_static_minor = {
+ .minor = LCD_MINOR,
+ .name = "dev_static_minor",
+};
+
+/* misc dynamic minor */
+static struct miscdevice dev_misc_dynamic_minor = {
+ .minor = MISC_DYNAMIC_MINOR,
+ .name = "dev_misc_dynamic_minor",
+};
+
+static void kunit_dynamic_minor(struct kunit *test)
+{
+ int ret;
+
+ ret = misc_register(&dev_dynamic_minor);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, 2, dev_dynamic_minor.minor);
+ misc_deregister(&dev_dynamic_minor);
+}
+
+static void kunit_static_minor(struct kunit *test)
+{
+ int ret;
+
+ ret = misc_register(&dev_static_minor);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, LCD_MINOR, dev_static_minor.minor);
+ misc_deregister(&dev_static_minor);
+}
+
+static void kunit_misc_dynamic_minor(struct kunit *test)
+{
+ int ret;
+
+ ret = misc_register(&dev_misc_dynamic_minor);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ misc_deregister(&dev_misc_dynamic_minor);
+}
+
+static struct kunit_case test_cases[] = {
+ KUNIT_CASE(kunit_dynamic_minor),
+ KUNIT_CASE(kunit_static_minor),
+ KUNIT_CASE(kunit_misc_dynamic_minor),
+ {}
+};
+
+static struct kunit_suite test_suite = {
+ .name = "misc_minor_test",
+ .test_cases = test_cases,
+};
+kunit_test_suite(test_suite);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Vimal Agrawal");
+MODULE_DESCRIPTION("misc minor testing");
--
2.17.1From: Vimal Agrawal <vimal.agrawal@sophos.com>
misc_minor_alloc was allocating id using ida for minor only in case of
MISC_DYNAMIC_MINOR but misc_minor_free was always freeing ids
using ida_free causing a mismatch and following warn:
> > WARNING: CPU: 0 PID: 159 at lib/idr.c:525 ida_free+0x3e0/0x41f
> > ida_free called for id=127 which is not allocated.
> > <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
...
> > [<60941eb4>] ida_free+0x3e0/0x41f
> > [<605ac993>] misc_minor_free+0x3e/0xbc
> > [<605acb82>] misc_deregister+0x171/0x1b3
misc_minor_alloc is changed to allocate id from ida for all minors
falling in the range of dynamic/ misc dynamic minors
Fixes: ab760791c0cf ("char: misc: Increase the maximum number of dynamic misc devices to 1048448")
Signed-off-by: Vimal Agrawal <vimal.agrawal@sophos.com>
Reviewed-by: Dirk VanDerMerwe <dirk.vandermerwe@sophos.com>
Cc: stable@vger.kernel.org
---
v2: Added Fixes:
Added missed case for static minor in misc_minor_alloc
v3: Removed kunit changes as that will be added as second patch in this two patch series
v4: Updated Signed-off-by: to match from:
v5: Used corporate id in from: and Signed-off-by:
drivers/char/misc.c | 39 ++++++++++++++++++++++++++++++---------
1 file changed, 30 insertions(+), 9 deletions(-)
diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 541edc26ec89..2cf595d2e10b 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -63,16 +63,30 @@ static DEFINE_MUTEX(misc_mtx);
#define DYNAMIC_MINORS 128 /* like dynamic majors */
static DEFINE_IDA(misc_minors_ida);
-static int misc_minor_alloc(void)
+static int misc_minor_alloc(int minor)
{
- int ret;
-
- ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
- if (ret >= 0) {
- ret = DYNAMIC_MINORS - ret - 1;
+ int ret = 0;
+
+ if (minor == MISC_DYNAMIC_MINOR) {
+ /* allocate free id */
+ ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
+ if (ret >= 0) {
+ ret = DYNAMIC_MINORS - ret - 1;
+ } else {
+ ret = ida_alloc_range(&misc_minors_ida, MISC_DYNAMIC_MINOR + 1,
+ MINORMASK, GFP_KERNEL);
+ }
} else {
- ret = ida_alloc_range(&misc_minors_ida, MISC_DYNAMIC_MINOR + 1,
- MINORMASK, GFP_KERNEL);
+ /* specific minor, check if it is in dynamic or misc dynamic range */
+ if (minor < DYNAMIC_MINORS) {
+ minor = DYNAMIC_MINORS - minor - 1;
+ ret = ida_alloc_range(&misc_minors_ida, minor, minor, GFP_KERNEL);
+ } else if (minor > MISC_DYNAMIC_MINOR) {
+ ret = ida_alloc_range(&misc_minors_ida, minor, minor, GFP_KERNEL);
+ } else {
+ /* case of non-dynamic minors, no need to allocate id */
+ ret = 0;
+ }
}
return ret;
}
@@ -219,7 +233,7 @@ int misc_register(struct miscdevice *misc)
mutex_lock(&misc_mtx);
if (is_dynamic) {
- int i = misc_minor_alloc();
+ int i = misc_minor_alloc(misc->minor);
if (i < 0) {
err = -EBUSY;
@@ -228,6 +242,7 @@ int misc_register(struct miscdevice *misc)
misc->minor = i;
} else {
struct miscdevice *c;
+ int i;
list_for_each_entry(c, &misc_list, list) {
if (c->minor == misc->minor) {
@@ -235,6 +250,12 @@ int misc_register(struct miscdevice *misc)
goto out;
}
}
+
+ i = misc_minor_alloc(misc->minor);
+ if (i < 0) {
+ err = -EBUSY;
+ goto out;
+ }
}
dev = MKDEV(MISC_MAJOR, misc->minor);
--
2.17.1From: Vimal Agrawal <vimal.agrawal@sophos.com>
basic kunit tests for misc minor
Signed-off-by: Vimal Agrawal <vimal.agrawal@sophos.com>
Reviewed-by: Dirk VanDerMerwe <dirk.vandermerwe@sophos.com>
---
v2: Split from previous patch
v3:
v4: Match patch version for whole patch series
v5: Moved kunit test code to drivers/misc/. Used corporate id in from: and Signed-off-by:
drivers/misc/Makefile | 1 +
drivers/misc/misc_minor_kunit.c | 69 +++++++++++++++++++++++++++++++++
lib/Kconfig.debug | 11 ++++++
3 files changed, 81 insertions(+)
create mode 100644 drivers/misc/misc_minor_kunit.c
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index a9f94525e181..22112861084c 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_SENSORS_BH1770) += bh1770glc.o
obj-$(CONFIG_SENSORS_APDS990X) += apds990x.o
obj-$(CONFIG_ENCLOSURE_SERVICES) += enclosure.o
obj-$(CONFIG_KGDB_TESTS) += kgdbts.o
+obj-$(CONFIG_TEST_MISC_MINOR) += misc_minor_kunit.o
obj-$(CONFIG_SGI_XP) += sgi-xp/
obj-$(CONFIG_SGI_GRU) += sgi-gru/
obj-$(CONFIG_SMPRO_ERRMON) += smpro-errmon.o
diff --git a/drivers/misc/misc_minor_kunit.c b/drivers/misc/misc_minor_kunit.c
new file mode 100644
index 000000000000..293e0fb7e43e
--- /dev/null
+++ b/drivers/misc/misc_minor_kunit.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <kunit/test.h>
+#include <kunit/test-bug.h>
+#include <linux/module.h>
+#include <linux/miscdevice.h>
+
+/* dynamic minor (2) */
+static struct miscdevice dev_dynamic_minor = {
+ .minor = 2,
+ .name = "dev_dynamic_minor",
+};
+
+/* static minor (LCD_MINOR) */
+static struct miscdevice dev_static_minor = {
+ .minor = LCD_MINOR,
+ .name = "dev_static_minor",
+};
+
+/* misc dynamic minor */
+static struct miscdevice dev_misc_dynamic_minor = {
+ .minor = MISC_DYNAMIC_MINOR,
+ .name = "dev_misc_dynamic_minor",
+};
+
+static void kunit_dynamic_minor(struct kunit *test)
+{
+ int ret;
+
+ ret = misc_register(&dev_dynamic_minor);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, 2, dev_dynamic_minor.minor);
+ misc_deregister(&dev_dynamic_minor);
+}
+
+static void kunit_static_minor(struct kunit *test)
+{
+ int ret;
+
+ ret = misc_register(&dev_static_minor);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ KUNIT_EXPECT_EQ(test, LCD_MINOR, dev_static_minor.minor);
+ misc_deregister(&dev_static_minor);
+}
+
+static void kunit_misc_dynamic_minor(struct kunit *test)
+{
+ int ret;
+
+ ret = misc_register(&dev_misc_dynamic_minor);
+ KUNIT_EXPECT_EQ(test, 0, ret);
+ misc_deregister(&dev_misc_dynamic_minor);
+}
+
+static struct kunit_case test_cases[] = {
+ KUNIT_CASE(kunit_dynamic_minor),
+ KUNIT_CASE(kunit_static_minor),
+ KUNIT_CASE(kunit_misc_dynamic_minor),
+ {}
+};
+
+static struct kunit_suite test_suite = {
+ .name = "misc_minor_test",
+ .test_cases = test_cases,
+};
+kunit_test_suite(test_suite);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Vimal Agrawal");
+MODULE_DESCRIPTION("misc minor testing");
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 7315f643817a..5a5d27284e0a 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2488,6 +2488,17 @@ config TEST_RHASHTABLE
config TEST_IDA
tristate "Perform selftest on IDA functions"
+config TEST_MISC_MINOR
+ tristate "Basic misc minor 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
+
+ If unsure, say N.
+
config TEST_PARMAN
tristate "Perform selftest on priority array manager"
depends on PARMAN
--
2.17.1misc_minor_alloc was allocating id using ida for minor only in case of
MISC_DYNAMIC_MINOR but misc_minor_free was always freeing ids
using ida_free causing a mismatch and following warn:
> > WARNING: CPU: 0 PID: 159 at lib/idr.c:525 ida_free+0x3e0/0x41f
> > ida_free called for id=127 which is not allocated.
> > <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
...
> > [<60941eb4>] ida_free+0x3e0/0x41f
> > [<605ac993>] misc_minor_free+0x3e/0xbc
> > [<605acb82>] misc_deregister+0x171/0x1b3
misc_minor_alloc is changed to allocate id from ida for all minors
falling in the range of dynamic/ misc dynamic minors
Fixes: ab760791c0cf ("char: misc: Increase the maximum number of dynamic misc devices to 1048448")
Signed-off-by: Vimal Agrawal <vimal.agrawal@sophos.com>
Cc: stable@vger.kernel.org
---
v2: Added Fixes:
added missed case for static minor in misc_minor_alloc
v3: removed kunit changes as that will be added as second patch in this two patch series
drivers/char/misc.c | 35 ++++++++++++++++++++++++++++-------
1 file changed, 28 insertions(+), 7 deletions(-)
diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 541edc26ec89..9d0cd3459b4f 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -63,16 +63,30 @@ static DEFINE_MUTEX(misc_mtx);
#define DYNAMIC_MINORS 128 /* like dynamic majors */
static DEFINE_IDA(misc_minors_ida);
-static int misc_minor_alloc(void)
+static int misc_minor_alloc(int minor)
{
int ret = 0;
- ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
- if (ret >= 0) {
- ret = DYNAMIC_MINORS - ret - 1;
- } else {
- ret = ida_alloc_range(&misc_minors_ida, MISC_DYNAMIC_MINOR + 1,
+ if (minor == MISC_DYNAMIC_MINOR) {
+ /* allocate free id */
+ ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
+ if (ret >= 0) {
+ ret = DYNAMIC_MINORS - ret - 1;
+ } else {
+ ret = ida_alloc_range(&misc_minors_ida, MISC_DYNAMIC_MINOR + 1,
MINORMASK, GFP_KERNEL);
+ }
+ } else {
+ /* specific minor, check if it is in dynamic or misc dynamic range */
+ if (minor < DYNAMIC_MINORS) {
+ minor = DYNAMIC_MINORS - minor - 1;
+ ret = ida_alloc_range(&misc_minors_ida, minor, minor, GFP_KERNEL);
+ } else if (minor > MISC_DYNAMIC_MINOR) {
+ ret = ida_alloc_range(&misc_minors_ida, minor, minor, GFP_KERNEL);
+ } else {
+ /* case of non-dynamic minors, no need to allocate id */
+ ret = 0;
+ }
}
return ret;
}
@@ -219,7 +233,7 @@ int misc_register(struct miscdevice *misc)
mutex_lock(&misc_mtx);
if (is_dynamic) {
- int i = misc_minor_alloc();
+ int i = misc_minor_alloc(misc->minor);
if (i < 0) {
err = -EBUSY;
@@ -228,6 +242,7 @@ int misc_register(struct miscdevice *misc)
misc->minor = i;
} else {
struct miscdevice *c;
+ int i;
list_for_each_entry(c, &misc_list, list) {
if (c->minor == misc->minor) {
@@ -235,6 +250,12 @@ int misc_register(struct miscdevice *misc)
goto out;
}
}
+
+ i = misc_minor_alloc(misc->minor);
+ if (i < 0) {
+ err = -EBUSY;
+ goto out;
+ }
}
dev = MKDEV(MISC_MAJOR, misc->minor);
--
2.17.1Hi Greg,
Added commit that this fixed under "Fixes:" in commit log
As this was not causing any failure in any functions, I tested it by
looking at WARN in logs.
I wrote a very basic kunit for testing misc_register as I could not
find any tests for it.
I was hoping to fail misc_register if someone passes minor value in
the range of dynamic and misc dynamic minor but I do see at least one
case where PSMOUSE_MINOR(1)
is passed. Unless we decide to change the range of dynamic minors from
0 -127 to 1-127 but I am not sure if there are other such cases so to
be safer this patch is allowing
callers to pass minor in the range of dynamic /misc dynamic minors as
was the case earlier.
Thanks,
Vimal
On Tue, Oct 15, 2024 at 12:32 PM Vimal Agrawal <avimalin@gmail.com> wrote:
>
> misc_minor_alloc was allocating id using ida for minor only in case of
> MISC_DYNAMIC_MINOR but misc_minor_free was always freeing ids
> using ida_free causing a mismatch and following warn:
> > > WARNING: CPU: 0 PID: 159 at lib/idr.c:525 ida_free+0x3e0/0x41f
> > > ida_free called for id=127 which is not allocated.
> > > <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> ...
> > > [<60941eb4>] ida_free+0x3e0/0x41f
> > > [<605ac993>] misc_minor_free+0x3e/0xbc
> > > [<605acb82>] misc_deregister+0x171/0x1b3
>
> misc_minor_alloc is changed to allocate id from ida for all minors
> falling in the range of dynamic/ misc dynamic minors
>
> Fixes: 0ad35fed618c ("char: misc: Increase the maximum number of dynamic misc devices to 1048448")
> Signed-off-by: Vimal Agrawal <vimal.agrawal@sophos.com>
> ---
> drivers/char/misc.c | 35 +++++++++++++++++-----
> lib/Kconfig.debug | 11 +++++++
> lib/Makefile | 1 +
> lib/test_misc_minor.c | 67 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 107 insertions(+), 7 deletions(-)
> create mode 100644 lib/test_misc_minor.c
>
> diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> index 541edc26ec89..9d0cd3459b4f 100644
> --- a/drivers/char/misc.c
> +++ b/drivers/char/misc.c
> @@ -63,16 +63,30 @@ static DEFINE_MUTEX(misc_mtx);
> #define DYNAMIC_MINORS 128 /* like dynamic majors */
> static DEFINE_IDA(misc_minors_ida);
>
> -static int misc_minor_alloc(void)
> +static int misc_minor_alloc(int minor)
> {
> int ret;
>
> - ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
> - if (ret >= 0) {
> - ret = DYNAMIC_MINORS - ret - 1;
> - } else {
> - ret = ida_alloc_range(&misc_minors_ida, MISC_DYNAMIC_MINOR + 1,
> + if (minor == MISC_DYNAMIC_MINOR) {
> + /* allocate free id */
> + ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
> + if (ret >= 0) {
> + ret = DYNAMIC_MINORS - ret - 1;
> + } else {
> + ret = ida_alloc_range(&misc_minors_ida, MISC_DYNAMIC_MINOR + 1,
> MINORMASK, GFP_KERNEL);
> + }
> + } else {
> + /* specific minor, check if it is in dynamic or misc dynamic range */
> + if (minor < DYNAMIC_MINORS) {
> + minor = DYNAMIC_MINORS - minor - 1;
> + ret = ida_alloc_range(&misc_minors_ida, minor, minor, GFP_KERNEL);
> + } else if (minor > MISC_DYNAMIC_MINOR) {
> + ret = ida_alloc_range(&misc_minors_ida, minor, minor, GFP_KERNEL);
> + } else {
> + /* case of non-dynamic minors, no need to allocate id */
> + ret = 0;
> + }
> }
> return ret;
> }
> @@ -219,7 +233,7 @@ int misc_register(struct miscdevice *misc)
> mutex_lock(&misc_mtx);
>
> if (is_dynamic) {
> - int i = misc_minor_alloc();
> + int i = misc_minor_alloc(misc->minor);
>
> if (i < 0) {
> err = -EBUSY;
> @@ -228,6 +242,7 @@ int misc_register(struct miscdevice *misc)
> misc->minor = i;
> } else {
> struct miscdevice *c;
> + int i;
>
> list_for_each_entry(c, &misc_list, list) {
> if (c->minor == misc->minor) {
> @@ -235,6 +250,12 @@ int misc_register(struct miscdevice *misc)
> goto out;
> }
> }
> +
> + i = misc_minor_alloc(misc->minor);
> + if (i < 0) {
> + err = -EBUSY;
> + goto out;
> + }
> }
>
> dev = MKDEV(MISC_MAJOR, misc->minor);
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 7315f643817a..5a5d27284e0a 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2488,6 +2488,17 @@ config TEST_RHASHTABLE
> config TEST_IDA
> tristate "Perform selftest on IDA functions"
>
> +config TEST_MISC_MINOR
> + tristate "Basic misc minor 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
> +
> + If unsure, say N.
> +
> config TEST_PARMAN
> tristate "Perform selftest on priority array manager"
> depends on PARMAN
> diff --git a/lib/Makefile b/lib/Makefile
> index 773adf88af41..631d73f96f76 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -68,6 +68,7 @@ obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o
> obj-$(CONFIG_TEST_IOV_ITER) += kunit_iov_iter.o
> obj-$(CONFIG_HASH_KUNIT_TEST) += test_hash.o
> obj-$(CONFIG_TEST_IDA) += test_ida.o
> +obj-$(CONFIG_TEST_MISC_MINOR) += test_misc_minor.o
> obj-$(CONFIG_TEST_UBSAN) += test_ubsan.o
> CFLAGS_test_ubsan.o += $(call cc-disable-warning, vla)
> CFLAGS_test_ubsan.o += $(call cc-disable-warning, unused-but-set-variable)
> diff --git a/lib/test_misc_minor.c b/lib/test_misc_minor.c
> new file mode 100644
> index 000000000000..bcec3fb1c46a
> --- /dev/null
> +++ b/lib/test_misc_minor.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <kunit/test.h>
> +#include <kunit/test-bug.h>
> +#include <linux/module.h>
> +#include <linux/miscdevice.h>
> +
> +/* dynamic minor (2) */
> +static struct miscdevice dev_dynamic_minor = {
> + .minor = 2,
> + .name = "dev_dynamic_minor",
> +};
> +
> +/* static minor (LCD_MINOR) */
> +static struct miscdevice dev_static_minor = {
> + .minor = LCD_MINOR,
> + .name = "dev_static_minor",
> +};
> +
> +/* misc dynamic minor */
> +static struct miscdevice dev_misc_dynamic_minor = {
> + .minor = MISC_DYNAMIC_MINOR,
> + .name = "dev_misc_dynamic_minor",
> +};
> +
> +static void kunit_dynamic_minor(struct kunit *test)
> +{
> + int ret;
> +
> + ret=misc_register(&dev_dynamic_minor);
> + KUNIT_EXPECT_EQ(test, 0, ret);
> + KUNIT_EXPECT_EQ(test, 2, dev_dynamic_minor.minor);
> + misc_deregister(&dev_dynamic_minor);
> +}
> +
> +static void kunit_static_minor(struct kunit *test)
> +{
> + int ret;
> +
> + ret=misc_register(&dev_static_minor);
> + KUNIT_EXPECT_EQ(test, 0, ret);
> + KUNIT_EXPECT_EQ(test, LCD_MINOR, dev_static_minor.minor);
> + misc_deregister(&dev_static_minor);
> +}
> +
> +static void kunit_misc_dynamic_minor(struct kunit *test)
> +{
> + int ret;
> +
> + ret=misc_register(&dev_misc_dynamic_minor);
> + KUNIT_EXPECT_EQ(test, 0, ret);
> + misc_deregister(&dev_misc_dynamic_minor);
> +}
> +
> +static struct kunit_case test_cases[] = {
> + KUNIT_CASE(kunit_dynamic_minor),
> + KUNIT_CASE(kunit_static_minor),
> + KUNIT_CASE(kunit_misc_dynamic_minor),
> + {}
> +};
> +
> +static struct kunit_suite test_suite = {
> + .name = "misc_minor_test",
> + .test_cases = test_cases,
> +};
> +kunit_test_suite(test_suite);
> +
> +MODULE_LICENSE("GPL");
> --
> 2.17.1
>
On Tue, Oct 15, 2024 at 07:02:26AM +0000, Vimal Agrawal wrote:
> misc_minor_alloc was allocating id using ida for minor only in case of
> MISC_DYNAMIC_MINOR but misc_minor_free was always freeing ids
> using ida_free causing a mismatch and following warn:
> > > WARNING: CPU: 0 PID: 159 at lib/idr.c:525 ida_free+0x3e0/0x41f
> > > ida_free called for id=127 which is not allocated.
> > > <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> ...
> > > [<60941eb4>] ida_free+0x3e0/0x41f
> > > [<605ac993>] misc_minor_free+0x3e/0xbc
> > > [<605acb82>] misc_deregister+0x171/0x1b3
>
> misc_minor_alloc is changed to allocate id from ida for all minors
> falling in the range of dynamic/ misc dynamic minors
>
> Fixes: 0ad35fed618c ("char: misc: Increase the maximum number of dynamic misc devices to 1048448")
> Signed-off-by: Vimal Agrawal <vimal.agrawal@sophos.com>
> ---
> drivers/char/misc.c | 35 +++++++++++++++++-----
> lib/Kconfig.debug | 11 +++++++
> lib/Makefile | 1 +
> lib/test_misc_minor.c | 67 +++++++++++++++++++++++++++++++++++++++++++
Cool, thanks for the test, but shouldn't that be a separate patch? That
doesn't need to be backported anywhere :)
Can you split this up into a 2 patch series?
Also, you need to fix your "From:" line, it does not match up with your
signed-off-by: line :(
thanks,
greg k-h
On Tue, Oct 15, 2024 at 07:02:26AM +0000, Vimal Agrawal wrote:
> misc_minor_alloc was allocating id using ida for minor only in case of
> MISC_DYNAMIC_MINOR but misc_minor_free was always freeing ids
> using ida_free causing a mismatch and following warn:
> > > WARNING: CPU: 0 PID: 159 at lib/idr.c:525 ida_free+0x3e0/0x41f
> > > ida_free called for id=127 which is not allocated.
> > > <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> ...
> > > [<60941eb4>] ida_free+0x3e0/0x41f
> > > [<605ac993>] misc_minor_free+0x3e/0xbc
> > > [<605acb82>] misc_deregister+0x171/0x1b3
>
> misc_minor_alloc is changed to allocate id from ida for all minors
> falling in the range of dynamic/ misc dynamic minors
>
> Fixes: 0ad35fed618c ("char: misc: Increase the maximum number of dynamic misc devices to 1048448")
> Signed-off-by: Vimal Agrawal <vimal.agrawal@sophos.com>
> ---
> drivers/char/misc.c | 35 +++++++++++++++++-----
> lib/Kconfig.debug | 11 +++++++
> lib/Makefile | 1 +
> lib/test_misc_minor.c | 67 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 107 insertions(+), 7 deletions(-)
> create mode 100644 lib/test_misc_minor.c
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- This looks like a new version of a previously submitted patch, but you
did not list below the --- line any changes from the previous version.
Please read the section entitled "The canonical patch format" in the
kernel file, Documentation/process/submitting-patches.rst for what
needs to be done here to properly describe this.
- You have marked a patch with a "Fixes:" tag for a commit that is in an
older released kernel, yet you do not have a cc: stable line in the
signed-off-by area at all, which means that the patch will not be
applied to any older kernel releases. To properly fix this, please
follow the documented rules in the
Documentation/process/stable-kernel-rules.rst file for how to resolve
this.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
© 2016 - 2024 Red Hat, Inc.