[RFC PATCH] char: misc: Init misc->list in a safe way

[RFC PATCH] char: misc: Init misc->list in a safe way

[Orson Zhai]

From: Zhongping Tan <zhongping.tan@spreadtrum.com>

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 <zhongping.tan@spreadtrum.com>
Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
---
 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

Re: [RFC PATCH] char: misc: Init misc->list in a safe way

[Arnd Bergmann]

On Mon, Jun 26, 2017 at 11:31 AM, Orson Zhai <orson.zhai@spreadtrum.com> wrote:
> From: Zhongping Tan <zhongping.tan@spreadtrum.com>
>
> 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 <zhongping.tan@spreadtrum.com>
> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>

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

RE: [RFC PATCH] char: misc: Init misc->list in a safe way

[Zhongping Tan (谭中平)]

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: arndbergmann@gmail.com [mailto:arndbergmann@gmail.com] 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 <orson.zhai@spreadtrum.com> wrote:
> From: Zhongping Tan <zhongping.tan@spreadtrum.com>
>
> 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 <zhongping.tan@spreadtrum.com>
> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>

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

Re: [RFC PATCH] char: misc: Init misc->list in a safe way

[Arnd Bergmann]

On Mon, Jun 26, 2017 at 1:48 PM, Zhongping Tan (谭中平)
<Zhongping.Tan@spreadtrum.com> 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

Re: 答复: [RFC PATCH] char: misc: Init misc->list in a safe way

[Arnd Bergmann]

On Mon, Jun 26, 2017 at 4:00 PM, Zhongping Tan (谭中平)
<Zhongping.Tan@spreadtrum.com> wrote:
> hi Arnd:
> Another way to describe this question,  misc_register shouldn't modify the member of the miscdevice especially when return error. Let the caller to ensure the list have been initialized,
> or when return error, please don't  initialize the list.

Why not? The caller should only initialize a couple of fields in the
structure (name, minor, fops, ...)
not never even look at the list entry, which is really internal to the
misc_register()/misc_unregister().

       Arnd

RE: 答复: [RFC PATCH] char: misc: Init misc->list in a safe way

[Zhongping Tan (谭中平)]

Ok, firstly we need to discuss the list usage, for list head we need do initialization, but for list node we don't need initialization at all. And for misc_list head, we use LIST_HEAD to define and initialize it. So I don't know why we put INIT_LIST_HEAD(&misc->list) in function misc_register, any bugs when without it?

-----Original Message-----
From: arndbergmann@gmail.com [mailto:arndbergmann@gmail.com] On Behalf Of Arnd Bergmann
Sent: Monday, June 26, 2017 10:10 PM
To: Zhongping Tan (谭中平)
Cc: Orson Zhai (翟京); Greg Kroah-Hartman; Linux Kernel Mailing List
Subject: Re: 答复: [RFC PATCH] char: misc: Init misc->list in a safe way

On Mon, Jun 26, 2017 at 4:00 PM, Zhongping Tan (谭中平)
<Zhongping.Tan@spreadtrum.com> wrote:
> hi Arnd:
> Another way to describe this question,  misc_register shouldn't modify 
> the member of the miscdevice especially when return error. Let the caller to ensure the list have been initialized, or when return error, please don't  initialize the list.

Why not? The caller should only initialize a couple of fields in the structure (name, minor, fops, ...) not never even look at the list entry, which is really internal to the misc_register()/misc_unregister().

       Arnd

Re: 答复: [RFC PATCH] char: misc: Init misc->list in a safe way

[Greg Kroah-Hartman]

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Tue, Jun 27, 2017 at 02:02:13AM +0000, Zhongping Tan (谭中平) wrote:
> Ok, firstly we need to discuss the list usage, for list head we need
> do initialization, but for list node we don't need initialization at
> all.

I don't understand, why is your misc driver touching that field at all?
Do I need to go and make it "private" somehow?

> And for misc_list head, we use LIST_HEAD to define and initialize
> it. So I don't know why we put INIT_LIST_HEAD(&misc->list) in function
> misc_register, any bugs when without it?

Again, what is wrong with the code today?  What driver is this causing
problems for?

thanks,

greg k-h

Re: [RFC PATCH] char: misc: Init misc->list in a safe way

[Orson Zhai]

Hi Greg & Arnd,

On Tue, Jun 27, 2017 at 08:29:17AM +0200, Greg Kroah-Hartman wrote:
> 
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
> 
> A: No.
> Q: Should I include quotations after my reply?
> 
> http://daringfireball.net/2007/07/on_top

We are so sorry for any troubles to you. I will take the role of Zhongping to
continue discussion here.

> 
> On Tue, Jun 27, 2017 at 02:02:13AM +0000, Zhongping Tan (谭中平) wrote:
> > Ok, firstly we need to discuss the list usage, for list head we need
> > do initialization, but for list node we don't need initialization at
> > all.
> 
> I don't understand, why is your misc driver touching that field at all?
> Do I need to go and make it "private" somehow?

There maybe some mis-understanding caused by not very well english expression.
I'll clarify what had happended to us.

> 
> > And for misc_list head, we use LIST_HEAD to define and initialize
> > it. So I don't know why we put INIT_LIST_HEAD(&misc->list) in function
> > misc_register, any bugs when without it?
> 
> Again, what is wrong with the code today?  What driver is this causing
> problems for?

It maybe a little bit long story.

In recent, our test team report a crash of non-stop reboot test on a mobile phone after 46 hours.

After some investigation, we got the information as following,

&misc_list = 0xFFFFFFFF822AC780 -> (
    next_=_0xFFFFFFFFA0087158 -> (
      next = 0xFFFFFFFFA0087158 -> (
        next = 0xFFFFFFFFA0087158,
        prev = 0xFFFFFFFFA0087158 -> (
          next = 0xFFFFFFFFA0087158,
          prev = 0xFFFFFFFFA0087158)),
      prev = 0xFFFFFFFFA0087158 -> (
        next = 0xFFFFFFFFA0087158,
        prev = 0xFFFFFFFFA0087158)),
    prev = 0xFFFF88007BF55618)

it seems that misc_list fall into a loop which cause surfaceflinger (a service from Android) dead.

And we got futher more information after that, 

  (struct miscdevice*)0xFFFFFFFFA0087140 = 0xFFFFFFFFA0087140 -> (
    minor = 20,
    name = 0xFFFFFFFFA008606D -> "fm",
    fops = 0xFFFFFFFFA0086700 -> (
      owner = 0xFFFFFFFFA0087480,
      llseek = 0x0,
      read = 0xFFFFFFFFA0081860,
      write = 0x0,
      read_iter = 0x0,
      write_iter = 0x0,
      iterate = 0x0,
      poll = 0x0,
      unlocked_ioctl = 0xFFFFFFFFA00832C0,
      compat_ioctl = 0xFFFFFFFFA0083870,
      mmap = 0x0,
      open = 0xFFFFFFFFA0081B80,
      flush = 0x0,
      release = 0xFFFFFFFFA00838C0,
      fsync = 0x0,
      aio_fsync = 0x0,
      fasync = 0x0,
      lock = 0x0,
      sendpage = 0x0,
      get_unmapped_area = 0x0,
      check_flags = 0x0,
      flock = 0x0,
      splice_write = 0x0,
      splice_read = 0x0,
      setlease = 0x0,
      fallocate = 0x0,
      show_fdinfo = 0x0),
    list = (
      next = 0xFFFFFFFFA0087158,
      prev = 0xFFFFFFFFA0087158),
    parent = 0x0,
    this_device = 0xFFFF88007BD32000,
    groups = 0x0,
    nodename = 0x0,
    mode = 0) 

We found the device is "fm". We highly suspect that fm driver call misc_register twice and reinitialize list to make ->pre & ->next pointing to himself.

Meanwhile, we checked fm driver and found nothing obviously wrong in the code.
Consider that this is a crash after 46 hours continuous power-on/off, it maybe caused by some special cases we are hard to know for now.
We think it might make some sence to add protection code into misc_register() at first.

Hope this could help to understand our situation.

We'll try to provide any detail inforamtion about this if necessary.

Thanks,
Orson

> 
> thanks,
> 
> greg k-h

Re: [RFC PATCH] char: misc: Init misc->list in a safe way

[Greg Kroah-Hartman]

On Wed, Jun 28, 2017 at 09:54:32AM +0800, Orson Zhai wrote:
> We found the device is "fm". We highly suspect that fm driver call
> misc_register twice and reinitialize list to make ->pre & ->next
> pointing to himself.
> 
> Meanwhile, we checked fm driver and found nothing obviously wrong in the code.

Do you have a pointer to this driver?  Is it in the kernel tree?

> Consider that this is a crash after 46 hours continuous power-on/off,
> it maybe caused by some special cases we are hard to know for now.

What would cause this driver to want to register/unregister itself?  Is
it "recycling" the misc structure, or creating it new each time?

And what kernel version are you testing here?

> We think it might make some sence to add protection code into
> misc_register() at first.

To protect from "foolish" callers?  Usually we fix the calling code to
not do foolish things. :)

thanks,

greg k-h

Re: [RFC PATCH] char: misc: Init misc->list in a safe way

[Arnd Bergmann]

On Wed, Jun 28, 2017 at 7:18 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Jun 28, 2017 at 09:54:32AM +0800, Orson Zhai wrote:
>> We found the device is "fm". We highly suspect that fm driver call
>> misc_register twice and reinitialize list to make ->pre & ->next
>> pointing to himself.
>>
>> Meanwhile, we checked fm driver and found nothing obviously wrong in the code.
>
> Do you have a pointer to this driver?  Is it in the kernel tree?

I found a version of some spreadtrum FM driver in the sources for the
Samsung Galaxy
J3, this is the driver https://pastebin.com/p7Y7xQNE

The driver has a single static 'misc_device' structure that will get
registered each
time the probe() function is called. The driver also supports both a static
platform_device definition (which a proper driver should not have, the device
should always be created either from platform code or from DT), and probing
from device tree.

If the DT has multiple "sprd,sprd_fm" device nodes, or a driver is lacking
the "#ifndef CONFIG_OF" guard around the static platform device, it should
always crash with the described symptom, but I don't see why it would only
happen after many hours of boot testing.

        Arnd

Re: [RFC PATCH] char: misc: Init misc->list in a safe way

[Greg Kroah-Hartman]

On Wed, Jun 28, 2017 at 12:34:28PM +0200, Arnd Bergmann wrote:
> On Wed, Jun 28, 2017 at 7:18 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Wed, Jun 28, 2017 at 09:54:32AM +0800, Orson Zhai wrote:
> >> We found the device is "fm". We highly suspect that fm driver call
> >> misc_register twice and reinitialize list to make ->pre & ->next
> >> pointing to himself.
> >>
> >> Meanwhile, we checked fm driver and found nothing obviously wrong in the code.
> >
> > Do you have a pointer to this driver?  Is it in the kernel tree?
> 
> I found a version of some spreadtrum FM driver in the sources for the
> Samsung Galaxy
> J3, this is the driver https://pastebin.com/p7Y7xQNE

Ah nice, Orson, is that the driver?

Any objection for me adding it to the kernel tree so we can fix up the
issues that Arnd points out in it (not to mention the coding style
issues...)

thanks,

greg k-h

Re: [RFC PATCH] char: misc: Init misc->list in a safe way

[Chunyan Zhang]

Hi Greg,

On 28 June 2017 at 19:21, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> On Wed, Jun 28, 2017 at 12:34:28PM +0200, Arnd Bergmann wrote:
>> On Wed, Jun 28, 2017 at 7:18 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Wed, Jun 28, 2017 at 09:54:32AM +0800, Orson Zhai wrote:
>> >> We found the device is "fm". We highly suspect that fm driver call
>> >> misc_register twice and reinitialize list to make ->pre & ->next
>> >> pointing to himself.
>> >>
>> >> Meanwhile, we checked fm driver and found nothing obviously wrong in the code.
>> >
>> > Do you have a pointer to this driver?  Is it in the kernel tree?
>>
>> I found a version of some spreadtrum FM driver in the sources for the
>> Samsung Galaxy
>> J3, this is the driver https://pastebin.com/p7Y7xQNE
>
> Ah nice, Orson, is that the driver?

For some unknown reason, Orson was missing in this list :)
I'm adding him back.

This is not the latest version of our FM driver.  You can find the
version we're using within Spreadtrum here [1], and the kernel version
we're using is v4.4.49.

>
> Any objection for me adding it to the kernel tree so we can fix up the

We have no objection to adding this driver to the kernel tree.

> issues that Arnd points out in it (not to mention the coding style
> issues...)

Actually, our FM driver owner Songhe (cc'ed here) had checked by
adding logs, the probe() function was called only once during the
whole initialization.
They have made a few times of review on the driver and haven't found
any suspicion in it, so Zhongping thought out the solution you can see
in this patch.

Many thanks for your help,
Chunyan

>
> thanks,
>
> greg k-h

Re: [RFC PATCH] char: misc: Init misc->list in a safe way

[Greg Kroah-Hartman]

On Thu, Jun 29, 2017 at 02:54:23PM +0800, Chunyan Zhang wrote:
> Hi Greg,
> 
> On 28 June 2017 at 19:21, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > On Wed, Jun 28, 2017 at 12:34:28PM +0200, Arnd Bergmann wrote:
> >> On Wed, Jun 28, 2017 at 7:18 AM, Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org> wrote:
> >> > On Wed, Jun 28, 2017 at 09:54:32AM +0800, Orson Zhai wrote:
> >> >> We found the device is "fm". We highly suspect that fm driver call
> >> >> misc_register twice and reinitialize list to make ->pre & ->next
> >> >> pointing to himself.
> >> >>
> >> >> Meanwhile, we checked fm driver and found nothing obviously wrong in the code.
> >> >
> >> > Do you have a pointer to this driver?  Is it in the kernel tree?
> >>
> >> I found a version of some spreadtrum FM driver in the sources for the
> >> Samsung Galaxy
> >> J3, this is the driver https://pastebin.com/p7Y7xQNE
> >
> > Ah nice, Orson, is that the driver?
> 
> For some unknown reason, Orson was missing in this list :)
> I'm adding him back.
> 
> This is not the latest version of our FM driver.  You can find the
> version we're using within Spreadtrum here [1], and the kernel version
> we're using is v4.4.49.

You forgot the link in your footnote, so we can read the code :)

> > Any objection for me adding it to the kernel tree so we can fix up the
> 
> We have no objection to adding this driver to the kernel tree.

Great, want me to make up the patch, or do you want to?

thanks,

greg k-h

Re: [RFC PATCH] char: misc: Init misc->list in a safe way

[Chunyan Zhang]

 ti

On 29 June 2017 at 15:03, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> On Thu, Jun 29, 2017 at 02:54:23PM +0800, Chunyan Zhang wrote:
>> Hi Greg,
>>
>> On 28 June 2017 at 19:21, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>> > On Wed, Jun 28, 2017 at 12:34:28PM +0200, Arnd Bergmann wrote:
>> >> On Wed, Jun 28, 2017 at 7:18 AM, Greg Kroah-Hartman
>> >> <gregkh@linuxfoundation.org> wrote:
>> >> > On Wed, Jun 28, 2017 at 09:54:32AM +0800, Orson Zhai wrote:
>> >> >> We found the device is "fm". We highly suspect that fm driver call
>> >> >> misc_register twice and reinitialize list to make ->pre & ->next
>> >> >> pointing to himself.
>> >> >>
>> >> >> Meanwhile, we checked fm driver and found nothing obviously wrong in the code.
>> >> >
>> >> > Do you have a pointer to this driver?  Is it in the kernel tree?
>> >>
>> >> I found a version of some spreadtrum FM driver in the sources for the
>> >> Samsung Galaxy
>> >> J3, this is the driver https://pastebin.com/p7Y7xQNE
>> >
>> > Ah nice, Orson, is that the driver?
>>
>> For some unknown reason, Orson was missing in this list :)
>> I'm adding him back.
>>
>> This is not the latest version of our FM driver.  You can find the
>> version we're using within Spreadtrum here [1], and the kernel version
>> we're using is v4.4.49.
>
> You forgot the link in your footnote, so we can read the code :)

Ah, my mistake :)

[1] https://github.com/sprdlinux/kernel/blob/for-greg/drivers/misc/sprdwcn_bsp/sc2342/radio/fmdrv_ops.c

>
>> > Any objection for me adding it to the kernel tree so we can fix up the
>>
>> We have no objection to adding this driver to the kernel tree.
>
> Great, want me to make up the patch, or do you want to?

How do we have the heart to take your precious time :)
Anyway, we can do that on our own.

Thank you,
Chunyan

>
> thanks,
>
> greg k-h