LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Can printk() sleep at runtime?
@ 2018-05-31  9:08 Jia-Ju Bai
  2018-05-31  9:45 ` Sergey Senozhatsky
  2018-05-31 10:04 ` Petr Mladek
  0 siblings, 2 replies; 9+ messages in thread
From: Jia-Ju Bai @ 2018-05-31  9:08 UTC (permalink / raw)
  To: bhelgaas, pmladek, sergey.senozhatsky, rostedt, Al Viro, Greg KH,
	corbet, Linus Torvalds
  Cc: Linux Kernel Mailing List

Hello,

I write a static analysis tool (DSAC), and it finds that printk can sleep.
According to this finding, there is an example bug in drivers/pci/pci.c 
in Linux-4.16.7.

Here is the call path for this bug.
Please look at it *from the bottom up*.

========== BUG ==========
[FUNC] __might_sleep
kernel/locking/mutex.c: 747: __might_sleep in __mutex_lock_common
kernel/locking/mutex.c: 893: __mutex_lock_common in __mutex_lock
kernel/locking/mutex.c: 908: __mutex_lock in mutex_lock_nested
drivers/clk/clk.c, 123: mutex_lock_nested in clk_prepare_lock
drivers/clk/clk.c, 1369: clk_prepare_lock in clk_core_get_rate
drivers/clk/clk.c, 1393: clk_core_get_rate in clk_get_rate
lib/vsprintf.c, 1450: clk_get_rate in clock
lib/vsprintf.c, 1944: clock in pointer
lib/vsprintf.c, 2286: pointer in vsnprintf
lib/vsprintf.c, 2385: vsnprintf in vscnprintf
kernel/printk/printk.c, 1853: vscnprintf in vprintk_emit
kernel/printk/printk.c, 1947: vprintk_emit in vprintk_default
kernel/printk/printk_safe.c, 379: vprintk_default in vprintk_func
kernel/printk/printk.c, 1980: vprintk_func in printk
drivers/pci/pci.c, 5364: printk in pci_specified_resource_alignment
drivers/pci/pci.c, 5313: spin_lock in pci_specified_resource_alignment

In fact, I suspect that my report is false, because I always have an 
impression that printk() cannot sleep.
But according to the call path, I cannot find where I make the mistake...

So could someone please help me to point the mistake?


Best wishes,
Jia-Ju Bai

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Can printk() sleep at runtime?
  2018-05-31  9:08 Can printk() sleep at runtime? Jia-Ju Bai
@ 2018-05-31  9:45 ` Sergey Senozhatsky
  2018-05-31 10:04 ` Petr Mladek
  1 sibling, 0 replies; 9+ messages in thread
From: Sergey Senozhatsky @ 2018-05-31  9:45 UTC (permalink / raw)
  To: Jia-Ju Bai
  Cc: bhelgaas, pmladek, sergey.senozhatsky, rostedt, Al Viro, Greg KH,
	corbet, Linus Torvalds, Linux Kernel Mailing List,
	Michael Turquette, Stephen Boyd, linux-clk

Cc-ing CLK people.

On (05/31/18 17:08), Jia-Ju Bai wrote:
> [FUNC] __might_sleep
> kernel/locking/mutex.c: 747: __might_sleep in __mutex_lock_common
> kernel/locking/mutex.c: 893: __mutex_lock_common in __mutex_lock
> kernel/locking/mutex.c: 908: __mutex_lock in mutex_lock_nested
> drivers/clk/clk.c, 123: mutex_lock_nested in clk_prepare_lock
> drivers/clk/clk.c, 1369: clk_prepare_lock in clk_core_get_rate
> drivers/clk/clk.c, 1393: clk_core_get_rate in clk_get_rate
> lib/vsprintf.c, 1450: clk_get_rate in clock
> lib/vsprintf.c, 1944: clock in pointer
> lib/vsprintf.c, 2286: pointer in vsnprintf
> lib/vsprintf.c, 2385: vsnprintf in vscnprintf
> kernel/printk/printk.c, 1853: vscnprintf in vprintk_emit
> kernel/printk/printk.c, 1947: vprintk_emit in vprintk_default
> kernel/printk/printk_safe.c, 379: vprintk_default in vprintk_func
> kernel/printk/printk.c, 1980: vprintk_func in printk
> drivers/pci/pci.c, 5364: printk in pci_specified_resource_alignment
> drivers/pci/pci.c, 5313: spin_lock in pci_specified_resource_alignment
> 
> In fact, I suspect that my report is false, because I always have an
> impression that printk() cannot sleep.
> But according to the call path, I cannot find where I make the mistake...
> 
> So could someone please help me to point the mistake?

I think you are right.

number(buf, end, clk_get_rate(clk), spec) indeed locks the `prepare_lock'
mutex from atomic context.

	-ss

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Can printk() sleep at runtime?
  2018-05-31  9:08 Can printk() sleep at runtime? Jia-Ju Bai
  2018-05-31  9:45 ` Sergey Senozhatsky
@ 2018-05-31 10:04 ` Petr Mladek
  2018-05-31 14:32   ` Linus Torvalds
  1 sibling, 1 reply; 9+ messages in thread
From: Petr Mladek @ 2018-05-31 10:04 UTC (permalink / raw)
  To: Jia-Ju Bai
  Cc: bhelgaas, sergey.senozhatsky, rostedt, Al Viro, Greg KH, corbet,
	Linus Torvalds, Linux Kernel Mailing List, Thomas Gleixner,
	Geert Uytterhoeven, Stephen Boyd, Michael Turquette

Adding Geert and timer people into Cc.

On Thu 2018-05-31 17:08:49, Jia-Ju Bai wrote:
> Hello,
> 
> I write a static analysis tool (DSAC), and it finds that printk can sleep.
> According to this finding, there is an example bug in drivers/pci/pci.c in
> Linux-4.16.7.
> 
> Here is the call path for this bug.
> Please look at it *from the bottom up*.
> 
> ========== BUG ==========
> [FUNC] __might_sleep
> kernel/locking/mutex.c: 747: __might_sleep in __mutex_lock_common
> kernel/locking/mutex.c: 893: __mutex_lock_common in __mutex_lock
> kernel/locking/mutex.c: 908: __mutex_lock in mutex_lock_nested
> drivers/clk/clk.c, 123: mutex_lock_nested in clk_prepare_lock
> drivers/clk/clk.c, 1369: clk_prepare_lock in clk_core_get_rate
> drivers/clk/clk.c, 1393: clk_core_get_rate in clk_get_rate
> lib/vsprintf.c, 1450: clk_get_rate in clock

The problem is in %pCr print format implementation. It calls
clk_get_rate() that might sleep. I wonder if we could use some
lock-less alternative instead.

Anyway, we need to fix or remove this format. vsprintf-like functions
are called in any context and nobody expect that they might sleep.

In each case, printk() must call it under logbuf_lock spinlock.
Otherwise, it would need to allocate a buffer for the formatted
string and it is not acceptable.

Best Regards,
Petr

> lib/vsprintf.c, 1944: clock in pointer
> lib/vsprintf.c, 2286: pointer in vsnprintf
> lib/vsprintf.c, 2385: vsnprintf in vscnprintf
> kernel/printk/printk.c, 1853: vscnprintf in vprintk_emit
> kernel/printk/printk.c, 1947: vprintk_emit in vprintk_default
> kernel/printk/printk_safe.c, 379: vprintk_default in vprintk_func
> kernel/printk/printk.c, 1980: vprintk_func in printk
> drivers/pci/pci.c, 5364: printk in pci_specified_resource_alignment
> drivers/pci/pci.c, 5313: spin_lock in pci_specified_resource_alignment
> 
> In fact, I suspect that my report is false, because I always have an
> impression that printk() cannot sleep.
> But according to the call path, I cannot find where I make the mistake...
> 
> So could someone please help me to point the mistake?
> 
> 
> Best wishes,
> Jia-Ju Bai

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Can printk() sleep at runtime?
  2018-05-31 10:04 ` Petr Mladek
@ 2018-05-31 14:32   ` Linus Torvalds
  2018-05-31 15:19     ` Stephen Boyd
  0 siblings, 1 reply; 9+ messages in thread
From: Linus Torvalds @ 2018-05-31 14:32 UTC (permalink / raw)
  To: Petr Mladek
  Cc: baijiaju1990, Bjorn Helgaas, SergeySenozhatsky, Steven Rostedt,
	Al Viro, Greg Kroah-Hartman, Jonathan Corbet,
	Linux Kernel Mailing List, Thomas Gleixner, Geert Uytterhoeven,
	sboyd, Michael Turquette

On Thu, May 31, 2018 at 5:05 AM Petr Mladek <pmladek@suse.com> wrote:
>
> Anyway, we need to fix or remove this format. vsprintf-like functions
> are called in any context and nobody expect that they might sleep.

Ack. I guess the argument is that "%pCr" is rare, and none of *those*
users may care, but I do think that doing things wrong as-is.

It's too subtle to have to know you're in a particular locking context
when you use a particular %p modifier.

                   Linus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Can printk() sleep at runtime?
  2018-05-31 14:32   ` Linus Torvalds
@ 2018-05-31 15:19     ` Stephen Boyd
  2018-05-31 16:42       ` Geert Uytterhoeven
  0 siblings, 1 reply; 9+ messages in thread
From: Stephen Boyd @ 2018-05-31 15:19 UTC (permalink / raw)
  To: Linus Torvalds, Petr Mladek
  Cc: baijiaju1990, Bjorn Helgaas, SergeySenozhatsky, Steven Rostedt,
	Al Viro, Greg Kroah-Hartman, Jonathan Corbet,
	Linux Kernel Mailing List, Thomas Gleixner, Geert Uytterhoeven,
	Michael Turquette

Quoting Linus Torvalds (2018-05-31 07:32:10)
> On Thu, May 31, 2018 at 5:05 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > Anyway, we need to fix or remove this format. vsprintf-like functions
> > are called in any context and nobody expect that they might sleep.
> 
> Ack. I guess the argument is that "%pCr" is rare, and none of *those*
> users may care, but I do think that doing things wrong as-is.
> 
> It's too subtle to have to know you're in a particular locking context
> when you use a particular %p modifier.
> 

Agreed. Removing the format seems to be the best approach. It looks like
only Geert has used it in the last few years and it hasn't been used
much otherwise.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Can printk() sleep at runtime?
  2018-05-31 15:19     ` Stephen Boyd
@ 2018-05-31 16:42       ` Geert Uytterhoeven
  2018-05-31 21:13         ` Steven Rostedt
  0 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2018-05-31 16:42 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Linus Torvalds, Petr Mladek, baijiaju1990, Bjorn Helgaas,
	SergeySenozhatsky, Steven Rostedt, Al Viro, Greg Kroah-Hartman,
	Jonathan Corbet, Linux Kernel Mailing List, Thomas Gleixner,
	Geert Uytterhoeven, Michael Turquette

Hi Stephen,

On Thu, May 31, 2018 at 5:19 PM, Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting Linus Torvalds (2018-05-31 07:32:10)
>> On Thu, May 31, 2018 at 5:05 AM Petr Mladek <pmladek@suse.com> wrote:
>> > Anyway, we need to fix or remove this format. vsprintf-like functions
>> > are called in any context and nobody expect that they might sleep.
>>
>> Ack. I guess the argument is that "%pCr" is rare, and none of *those*
>> users may care, but I do think that doing things wrong as-is.
>>
>> It's too subtle to have to know you're in a particular locking context
>> when you use a particular %p modifier.
>
> Agreed. Removing the format seems to be the best approach. It looks like
> only Geert has used it in the last few years and it hasn't been used
> much otherwise.

Indeed, just 3 users (the broadcom one isn't mine):
    drivers/clk/renesas/renesas-cpg-mssr.c
    drivers/thermal/broadcom/bcm2835_thermal.c
    drivers/tty/serial/sh-sci.c

Alternatively, can we have a special version __clk_get_rate() that just
returns clk->core->rate?
Or would that be too inaccurate in the presence of CLK_GET_RATE_NOCACHE?
The function could still return 0 in case the flag is set.

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Can printk() sleep at runtime?
  2018-05-31 16:42       ` Geert Uytterhoeven
@ 2018-05-31 21:13         ` Steven Rostedt
  2018-06-01  0:59           ` Jia-Ju Bai
  0 siblings, 1 reply; 9+ messages in thread
From: Steven Rostedt @ 2018-05-31 21:13 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Stephen Boyd, Linus Torvalds, Petr Mladek, baijiaju1990,
	Bjorn Helgaas, SergeySenozhatsky, Al Viro, Greg Kroah-Hartman,
	Jonathan Corbet, Linux Kernel Mailing List, Thomas Gleixner,
	Geert Uytterhoeven, Michael Turquette

On Thu, 31 May 2018 18:42:48 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Stephen,
> 
> On Thu, May 31, 2018 at 5:19 PM, Stephen Boyd <sboyd@kernel.org> wrote:
> > Quoting Linus Torvalds (2018-05-31 07:32:10)  
> >> On Thu, May 31, 2018 at 5:05 AM Petr Mladek <pmladek@suse.com> wrote:  
> >> > Anyway, we need to fix or remove this format. vsprintf-like functions
> >> > are called in any context and nobody expect that they might sleep.  
> >>
> >> Ack. I guess the argument is that "%pCr" is rare, and none of *those*
> >> users may care, but I do think that doing things wrong as-is.
> >>
> >> It's too subtle to have to know you're in a particular locking context
> >> when you use a particular %p modifier.  
> >
> > Agreed. Removing the format seems to be the best approach. It looks like
> > only Geert has used it in the last few years and it hasn't been used
> > much otherwise.  
> 
> Indeed, just 3 users (the broadcom one isn't mine):
>     drivers/clk/renesas/renesas-cpg-mssr.c
>     drivers/thermal/broadcom/bcm2835_thermal.c
>     drivers/tty/serial/sh-sci.c
> 
> Alternatively, can we have a special version __clk_get_rate() that just
> returns clk->core->rate?
> Or would that be too inaccurate in the presence of CLK_GET_RATE_NOCACHE?
> The function could still return 0 in case the flag is set.

If it's only used in three locations, I think it would be better to
simply remove it from vsprintf() and have the three callers call
clk_get_rate() directly.

-- Steve

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Can printk() sleep at runtime?
  2018-05-31 21:13         ` Steven Rostedt
@ 2018-06-01  0:59           ` Jia-Ju Bai
  2018-06-01  9:30             ` Geert Uytterhoeven
  0 siblings, 1 reply; 9+ messages in thread
From: Jia-Ju Bai @ 2018-06-01  0:59 UTC (permalink / raw)
  To: Steven Rostedt, Geert Uytterhoeven
  Cc: Stephen Boyd, Linus Torvalds, Petr Mladek, Bjorn Helgaas,
	SergeySenozhatsky, Al Viro, Greg Kroah-Hartman, Jonathan Corbet,
	Linux Kernel Mailing List, Thomas Gleixner, Geert Uytterhoeven,
	Michael Turquette



On 2018/6/1 5:13, Steven Rostedt wrote:
> On Thu, 31 May 2018 18:42:48 +0200
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
>> Hi Stephen,
>>
>> On Thu, May 31, 2018 at 5:19 PM, Stephen Boyd <sboyd@kernel.org> wrote:
>>> Quoting Linus Torvalds (2018-05-31 07:32:10)
>>>> On Thu, May 31, 2018 at 5:05 AM Petr Mladek <pmladek@suse.com> wrote:
>>>>> Anyway, we need to fix or remove this format. vsprintf-like functions
>>>>> are called in any context and nobody expect that they might sleep.
>>>> Ack. I guess the argument is that "%pCr" is rare, and none of *those*
>>>> users may care, but I do think that doing things wrong as-is.
>>>>
>>>> It's too subtle to have to know you're in a particular locking context
>>>> when you use a particular %p modifier.
>>> Agreed. Removing the format seems to be the best approach. It looks like
>>> only Geert has used it in the last few years and it hasn't been used
>>> much otherwise.
>> Indeed, just 3 users (the broadcom one isn't mine):
>>      drivers/clk/renesas/renesas-cpg-mssr.c
>>      drivers/thermal/broadcom/bcm2835_thermal.c
>>      drivers/tty/serial/sh-sci.c
>>
>> Alternatively, can we have a special version __clk_get_rate() that just
>> returns clk->core->rate?
>> Or would that be too inaccurate in the presence of CLK_GET_RATE_NOCACHE?
>> The function could still return 0 in case the flag is set.
> If it's only used in three locations, I think it would be better to
> simply remove it from vsprintf() and have the three callers call
> clk_get_rate() directly.

Agreed.


Best wishes,
Jia-Ju Bai

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Can printk() sleep at runtime?
  2018-06-01  0:59           ` Jia-Ju Bai
@ 2018-06-01  9:30             ` Geert Uytterhoeven
  0 siblings, 0 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2018-06-01  9:30 UTC (permalink / raw)
  To: Jia-Ju Bai
  Cc: Steven Rostedt, Stephen Boyd, Linus Torvalds, Petr Mladek,
	Bjorn Helgaas, SergeySenozhatsky, Al Viro, Greg Kroah-Hartman,
	Jonathan Corbet, Linux Kernel Mailing List, Thomas Gleixner,
	Geert Uytterhoeven, Michael Turquette

Hi Jia-Ju,

On Fri, Jun 1, 2018 at 2:59 AM, Jia-Ju Bai <baijiaju1990@gmail.com> wrote:
> On 2018/6/1 5:13, Steven Rostedt wrote:
>> On Thu, 31 May 2018 18:42:48 +0200
>> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Thu, May 31, 2018 at 5:19 PM, Stephen Boyd <sboyd@kernel.org> wrote:
>>>> Quoting Linus Torvalds (2018-05-31 07:32:10)
>>>>> On Thu, May 31, 2018 at 5:05 AM Petr Mladek <pmladek@suse.com> wrote:
>>>>>>
>>>>>> Anyway, we need to fix or remove this format. vsprintf-like functions
>>>>>> are called in any context and nobody expect that they might sleep.
>>>>>
>>>>> Ack. I guess the argument is that "%pCr" is rare, and none of *those*
>>>>> users may care, but I do think that doing things wrong as-is.
>>>>>
>>>>> It's too subtle to have to know you're in a particular locking context
>>>>> when you use a particular %p modifier.
>>>>
>>>> Agreed. Removing the format seems to be the best approach. It looks like
>>>> only Geert has used it in the last few years and it hasn't been used
>>>> much otherwise.
>>>
>>> Indeed, just 3 users (the broadcom one isn't mine):
>>>      drivers/clk/renesas/renesas-cpg-mssr.c
>>>      drivers/thermal/broadcom/bcm2835_thermal.c
>>>      drivers/tty/serial/sh-sci.c
>>>
>>> Alternatively, can we have a special version __clk_get_rate() that just
>>> returns clk->core->rate?
>>> Or would that be too inaccurate in the presence of CLK_GET_RATE_NOCACHE?
>>> The function could still return 0 in case the flag is set.
>>
>> If it's only used in three locations, I think it would be better to
>> simply remove it from vsprintf() and have the three callers call
>> clk_get_rate() directly.

OK, patches sent.

Thanks for reporting!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-06-01  9:30 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-31  9:08 Can printk() sleep at runtime? Jia-Ju Bai
2018-05-31  9:45 ` Sergey Senozhatsky
2018-05-31 10:04 ` Petr Mladek
2018-05-31 14:32   ` Linus Torvalds
2018-05-31 15:19     ` Stephen Boyd
2018-05-31 16:42       ` Geert Uytterhoeven
2018-05-31 21:13         ` Steven Rostedt
2018-06-01  0:59           ` Jia-Ju Bai
2018-06-01  9:30             ` Geert Uytterhoeven

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).