From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Torvalds Subject: Re: [PATCH] vsprintf: Do not break early boot with probing addresses Date: Thu, 9 May 2019 21:47:27 -0700 Message-ID: References: <20190509121923.8339-1-pmladek@suse.com> <20190510043200.GC15652@jagdpanzerIV> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="000000000000166f8f05888144be" Return-path: In-Reply-To: <20190510043200.GC15652@jagdpanzerIV> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@lists.ozlabs.org Sender: "Linuxppc-dev" List-Archive: List-Post: To: Sergey Senozhatsky Cc: Petr Mladek , linux-arch@vger.kernel.org, Heiko Carstens , linux-s390@vger.kernel.org, Rasmus Villemoes , linux-kernel@vger.kernel.org, Steven Rostedt , Michal Hocko , Sergey Senozhatsky , Stephen Rothwell , Andy Shevchenko , linuxppc-dev@lists.ozlabs.org, Martin Schwidefsky , "Tobin C . Harding" List-ID: --000000000000166f8f05888144be Content-Type: text/plain; charset="UTF-8" [ Sorry about html and mobile crud, I'm not at the computer right now ] How about we just undo the whole misguided probe_kernel_address() thing? The excuse for is was that it would avoid crashes. It turns out that was wrong, and that it just made things much worse. Honestly, we haven't really had the crashes that the logic was supposed to protect against in the first place, so by now it's clear that the whole thing was a stupid and horrible mistake in the first place. So let's admit that and just go back to the old sane model. Seriously, we've never needed that odd probing. It causes issues. It's wrong and pointless. Linus On Thu, May 9, 2019, 21:32 Sergey Senozhatsky < sergey.senozhatsky.work@gmail.com> wrote: > On (05/09/19 14:19), Petr Mladek wrote: > > 1. Report on Power: > > > > Kernel crashes very early during boot with with CONFIG_PPC_KUAP and > > CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG > > > > The problem is the combination of some new code called via printk(), > > check_pointer() which calls probe_kernel_read(). That then calls > > allow_user_access() (PPC_KUAP) and that uses mmu_has_feature() too early > > (before we've patched features). With the JUMP_LABEL debug enabled that > > causes us to call printk() & dump_stack() and we end up recursing and > > overflowing the stack. > > Hmm... hmm... PPC does an .opd-based symbol dereference, which > eventually probe_kernel_read()-s. So early printk(%pS) will do > > printk(%pS) > dereference_function_descriptor() > probe_kernel_address() > dump_stack() > printk(%pS) > dereference_function_descriptor() > probe_kernel_address() > dump_stack() > printk(%pS) > ... > > I'd say... that it's not vsprintf that we want to fix, it's > the idea that probe_kernel_address() can dump_stack() on any > platform. On some archs probe_kernel_address()->dump_stack() > is going nowhere: > dump_stack() does probe_kernel_address(), which calls dump_stack(), > which calls printk(%pS)->probe_kernel_address() again and again, > and again. > > -ss > --000000000000166f8f05888144be Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
[ Sorry about html and mobile crud, I&#= 39;m not at the computer right now ]

How a= bout we just undo the whole misguided probe_kernel_address() thing?

The excuse for is was that it would a= void crashes.

It turns o= ut that was wrong, and that it just made things much worse. Honestly, we ha= ven't really had the crashes that the logic was supposed to protect aga= inst in the first place, so by now it's clear that the whole thing was = a stupid and horrible mistake in the first place.
So let's admit that and just go back to the o= ld sane model.

Seriously= , we've never needed that odd probing. It causes issues. It's wrong= and pointless.

=C2=A0 = =C2=A0 =C2=A0 =C2=A0Linus

On Thu, May 9, 2019, 21:32 Sergey Senozhatsk= y <sergey.senozhats= ky.work@gmail.com> wrote:
On= (05/09/19 14:19), Petr Mladek wrote:
> 1. Report on Power:
>
> Kernel crashes very early during boot with with CONFIG_PPC_KUAP and > CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG
>
> The problem is the combination of some new code called via printk(), > check_pointer() which calls probe_kernel_read(). That then calls
> allow_user_access() (PPC_KUAP) and that uses mmu_has_feature() too ear= ly
> (before we've patched features). With the JUMP_LABEL debug enabled= that
> causes us to call printk() & dump_stack() and we end up recursing = and
> overflowing the stack.

Hmm... hmm... PPC does an .opd-based symbol dereference, which
eventually probe_kernel_read()-s. So early printk(%pS) will do

=C2=A0 =C2=A0 =C2=A0 =C2=A0 printk(%pS)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dereference_function_descriptor()
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 probe_kernel_address()
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dump_stack()
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 printk(%pS)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dereference_function_descri= ptor()
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 probe_kernel_address()
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dump_stack()
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 printk(%pS)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0...

I'd say... that it's not vsprintf that we want to fix, it's
the idea that probe_kernel_address() can dump_stack() on any
platform. On some archs probe_kernel_address()->dump_stack()
is going nowhere:
=C2=A0dump_stack() does probe_kernel_address(), which calls dump_stack(), =C2=A0which calls printk(%pS)->probe_kernel_address() again and again, =C2=A0and again.

=C2=A0 =C2=A0 =C2=A0 =C2=A0 -ss
--000000000000166f8f05888144be--