From: Zhongping Tan <[email protected]>
It is likely to enter a wrong case and return an error when registerring
a misc device. As a result, misc->list will be intialized to a dead loop
which is possible to go into wrong situation if anyone refers to it else
where.
Move the initializion line out of all error branches to avoid any side
effect.
Signed-off-by: Zhongping Tan <[email protected]>
Signed-off-by: Orson Zhai <[email protected]>
---
drivers/char/misc.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index c9cd1ea6844a..876e7d57cc6c 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -188,8 +188,6 @@ int misc_register(struct miscdevice *misc)
int err = 0;
bool is_dynamic = (misc->minor == MISC_DYNAMIC_MINOR);
- INIT_LIST_HEAD(&misc->list);
-
mutex_lock(&misc_mtx);
if (is_dynamic) {
@@ -233,6 +231,7 @@ int misc_register(struct miscdevice *misc)
* Add it to the front, so that later devices can "override"
* earlier defaults
*/
+ INIT_LIST_HEAD(&misc->list);
list_add(&misc->list, &misc_list);
out:
mutex_unlock(&misc_mtx);
--
2.12.2
On Mon, Jun 26, 2017 at 11:31 AM, Orson Zhai <[email protected]> wrote:
> From: Zhongping Tan <[email protected]>
>
> It is likely to enter a wrong case and return an error when registerring
> a misc device. As a result, misc->list will be intialized to a dead loop
> which is possible to go into wrong situation if anyone refers to it else
> where.
>
> Move the initializion line out of all error branches to avoid any side
> effect.
>
> Signed-off-by: Zhongping Tan <[email protected]>
> Signed-off-by: Orson Zhai <[email protected]>
I fail to see the problem. Did you run into a bug that gets fixed by
this patch, or did you arrive here after code inspection?
As far as I can tell, the INIT_LIST_HEAD() on the entry has
no effect at all, the fields simply get initialized in the list_add(),
and the list traversal is protected using misc_mtx.
Arnd
Hi Arnd：
If we can get list_add(&misc->list, &misc_list), then there is no problem at all, but if the misc_register return "-EBUSY"(Maybe the same miscdevice register twice ), then the deadloop will happen at list_for_each_entry(c, &misc_list, list);
So at my opinion just remove the initialization code or do initialization when we need do list_add.
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Arnd Bergmann
Sent: Monday, June 26, 2017 6:03 PM
To: Orson Zhai (翟京)
Cc: Greg Kroah-Hartman; Linux Kernel Mailing List; Zhongping Tan (谭中平)
Subject: Re: [RFC PATCH] char: misc: Init misc->list in a safe way
On Mon, Jun 26, 2017 at 11:31 AM, Orson Zhai <[email protected]> wrote:
> From: Zhongping Tan <[email protected]>
>
> It is likely to enter a wrong case and return an error when
> registerring a misc device. As a result, misc->list will be intialized
> to a dead loop which is possible to go into wrong situation if anyone
> refers to it else where.
>
> Move the initializion line out of all error branches to avoid any side
> effect.
>
> Signed-off-by: Zhongping Tan <[email protected]>
> Signed-off-by: Orson Zhai <[email protected]>
I fail to see the problem. Did you run into a bug that gets fixed by this patch, or did you arrive here after code inspection?
As far as I can tell, the INIT_LIST_HEAD() on the entry has no effect at all, the fields simply get initialized in the list_add(), and the list traversal is protected using misc_mtx.
Arnd
On Mon, Jun 26, 2017 at 1:48 PM, Zhongping Tan (谭中平)
<[email protected]> wrote:
> Hi Arnd：
> If we can get list_add(&misc->list, &misc_list), then there is no problem at all, but if the misc_register return "-EBUSY"(Maybe the same miscdevice register twice ), then the deadloop will happen at list_for_each_entry(c, &misc_list, list);
> So at my opinion just remove the initialization code or do initialization when we need do list_add.
I think you are misinterpreting a bug you see: the pointer we pass
into misc_register() must not already be registered, which means that
nothing references misc->list at all.
If misc_register() returns success, and you then call it another time,
you will see the exact behavior that you describe, entering an endless
loop in "list_for_each_entry(c, &misc_list, list)". The correct fix for that
is in the calling code, to ensure that the same device can not get
registered multiple times.
Arnd