Re: [BUG] from x86: Support kmap_local() forced debugging

Re: [BUG] from x86: Support kmap_local() forced debugging

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 4491 bytes --]

On Wed, Jan 6, 2021 at 3:01 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> I triggered the following crash on x86_32 by simply doing a:
>
> (ssh'ing into the box)
>
>   # head -100 /tmp/output-file
>
> Where the /tmp/output-file was the output of a trace-cmd report.
> Even after rebooting and not running the tracing code, simply doing the
> head command still crashed.

The code decodes to

   0:   3b 5d e8                cmp    -0x18(%ebp),%ebx
   3:   0f 47 5d e8             cmova  -0x18(%ebp),%ebx
   7:   c7 45 e0 00 00 00 00    movl   $0x0,-0x20(%ebp)
   e:   8b 7d e0                mov    -0x20(%ebp),%edi
  11:   39 7d e8                cmp    %edi,-0x18(%ebp)
  14:   76 3a                   jbe    0x50
  16:   8b 45 d4                mov    -0x2c(%ebp),%eax
  19:   e8 a4 e4 ff ff          call   0xffffe4c2
  1e:   8b 55 e4                mov    -0x1c(%ebp),%edx
  21:   03 55 e0                add    -0x20(%ebp),%edx
  24:   89 d9                   mov    %ebx,%ecx
  26:   01 c6                   add    %eax,%esi
  28:   89 d7                   mov    %edx,%edi
  2a:*  f3 a4                   rep movsb %ds:(%esi),%es:(%edi)
 <-- trapping instruction
  2c:   e8 c9 e4 ff ff          call   0xffffe4fa
  31:   01 5d e0                add    %ebx,-0x20(%ebp)
  34:   8b 5d e8                mov    -0x18(%ebp),%ebx
  37:   b8 00 10 00 00          mov    $0x1000,%eax
  3c:   2b 5d e0                sub    -0x20(%ebp),%ebx

and while it would be good to see the output of
scripts/decode_stacktrace.sh, I strongly suspect that the above is

                                vaddr = kmap_atomic(p);
                                memcpy(to + copied, vaddr + p_off, p_len);
                                kunmap_atomic(vaddr);

(although I wonder how/why the heck you've enabled
CC_OPTIMIZE_FOR_SIZE=y, which is what causes "memcpy()" to be done as
that "rep movsb". I thought we disabled it because it's so bad on most
cpus).

So that first "call" instruction is the kmap_atomic(), the "rep movs"
is the memcpy(), and the "call" instruction immediately after is the
kunmap_atomic().

Anyway, you can see vaddr in register state:

        EAX: fff57000

so we've kmapped that one page at fff57000, but we're accessing past
it into the next page:

> BUG: unable to handle page fault for address: fff58000

with the current source address being (ESI: fff58000) and we still
have 248 bytes to go (ECX: 000000f8) even though we've already
overflowed into the next page.

You can see the original count still (EBX: 000005a8), so it really
looks like that skb_frag_foreach_page() logic

                        skb_frag_foreach_page(f,
                                              skb_frag_off(f) + offset - start,
                                              copy, p, p_off, p_len, copied) {
                                vaddr = kmap_atomic(p);
                                memcpy(to + copied, vaddr + p_off, p_len);
                                kunmap_atomic(vaddr);
                        }

must be wrong, and doesn't handle the "each page" part properly. It
must have started in the middle of the page, and p_len (that 0x5a8)
was wrong.

IOW, it really looks like p_off + p_len had the value 0x10f8, which is
larger than one page. And looking at the code, in
skb_frag_foreach_page(), I see:

             p_off = (f_off) & (PAGE_SIZE - 1),                         \
             p_len = skb_frag_must_loop(p) ?                            \
             min_t(u32, f_len, PAGE_SIZE - p_off) : f_len,              \

where that "min_t(u32, f_len, PAGE_SIZE - p_off)" looks correct, but
then presumably skb_frag_must_loop() must be wrong.

Oh, and when I look at that, I see

    static inline bool skb_frag_must_loop(struct page *p)
    {
    #if defined(CONFIG_HIGHMEM)
            if (PageHighMem(p))
                    return true;
    #endif
            return false;
    }

and that is no longer true. With the kmap debugging, even non-highmem
pages need that "do one page at a time" code, because even non-highmem
pages get remapped by kmap().

IOW, I think the patch to fix this might be something like the attached.

I wonder whether there is other code that "knows" about kmap() only
affecting PageHighmem() pages thing that is no longer true.

Looking at some other code, skb_gro_reset_offset() looks suspiciously
like it also thinks highmem pages are special.

Adding the networking people involved in this area to the cc too.

               Linus

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 544 bytes --]

 include/linux/skbuff.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 333bcdc39635..c858adfb5a82 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -366,7 +366,7 @@ static inline void skb_frag_size_sub(skb_frag_t *frag, int delta)
 static inline bool skb_frag_must_loop(struct page *p)
 {
 #if defined(CONFIG_HIGHMEM)
-	if (PageHighMem(p))
+	if (IS_ENABLED(CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP) || PageHighMem(p))
 		return true;
 #endif
 	return false;

Re: [BUG] from x86: Support kmap_local() forced debugging

[Steven Rostedt]

On Wed, 6 Jan 2021 17:03:48 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> (although I wonder how/why the heck you've enabled
> CC_OPTIMIZE_FOR_SIZE=y, which is what causes "memcpy()" to be done as
> that "rep movsb". I thought we disabled it because it's so bad on most
> cpus).

Why?

Because to test x86_32, I have a Fedora Core 13 (yes 13!) partition
(baremetal) that I use. And the .config I use for it hasn't changed
since that time ;-) (except to add new features that I want to test on
x86_32).

Anyway, I'll test out your patch. Thanks for investigating this.

-- Steve

Re: [BUG] from x86: Support kmap_local() forced debugging

[Steven Rostedt]

On Wed, 6 Jan 2021 17:03:48 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -366,7 +366,7 @@ static inline void skb_frag_size_sub(skb_frag_t *frag, int delta)
>  static inline bool skb_frag_must_loop(struct page *p)
>  {
>  #if defined(CONFIG_HIGHMEM)
> -	if (PageHighMem(p))
> +	if (IS_ENABLED(CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP) || PageHighMem(p))
>  		return true;
>  #endif
>  	return false;

I applied this and I haven't been able to crash it again.

Thanks,

-- Steve

Re: [BUG] from x86: Support kmap_local() forced debugging

[Jakub Kicinski]

On Wed, 6 Jan 2021 17:03:48 -0800 Linus Torvalds wrote:
> I wonder whether there is other code that "knows" about kmap() only
> affecting PageHighmem() pages thing that is no longer true.
> 
> Looking at some other code, skb_gro_reset_offset() looks suspiciously
> like it also thinks highmem pages are special.
> 
> Adding the networking people involved in this area to the cc too.

Thanks for the detailed analysis! skb_gro_reset_offset() checks if
kernel can read data in the fragments directly as an optimization, 
in case the entire header is in a fragment. 

IIUC DEBUG_KMAP_LOCAL_FORCE_MAP only affects the mappings from
explicit kmap calls, which GRO won't make - it will fall back to
pulling the header out of the fragment and end up in skb_copy_bits(),
i.e. the loop you fixed. So GRO should be good. I think..

Re: [BUG] from x86: Support kmap_local() forced debugging

[Willem de Bruijn]

On Wed, Jan 6, 2021 at 8:49 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 6 Jan 2021 17:03:48 -0800 Linus Torvalds wrote:
> > I wonder whether there is other code that "knows" about kmap() only
> > affecting PageHighmem() pages thing that is no longer true.
> >
> > Looking at some other code, skb_gro_reset_offset() looks suspiciously
> > like it also thinks highmem pages are special.
> >
> > Adding the networking people involved in this area to the cc too.
>
> Thanks for the detailed analysis! skb_gro_reset_offset() checks if
> kernel can read data in the fragments directly as an optimization,
> in case the entire header is in a fragment.
>
> IIUC DEBUG_KMAP_LOCAL_FORCE_MAP only affects the mappings from
> explicit kmap calls, which GRO won't make - it will fall back to
> pulling the header out of the fragment and end up in skb_copy_bits(),
> i.e. the loop you fixed. So GRO should be good. I think..

Agreed. That code in skb_gro_reset_offset skips the GRO frag0
optimization in various cases, including if the first fragment is in
high mem.

That specific check goes back to the introduction of the frag0
optimization in commit 86911732d399 ("gro: Avoid copying headers of
unmerged packets"), at the time in helper skb_gro_header().

Very glad to hear that the fix addresses the crash in
skb_frag_foreach_page. Thanks!

Re: [BUG] from x86: Support kmap_local() forced debugging

[Willem de Bruijn]

On Wed, Jan 6, 2021 at 9:11 PM Willem de Bruijn <willemb@google.com> wrote:
>
> On Wed, Jan 6, 2021 at 8:49 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Wed, 6 Jan 2021 17:03:48 -0800 Linus Torvalds wrote:
> > > I wonder whether there is other code that "knows" about kmap() only
> > > affecting PageHighmem() pages thing that is no longer true.
> > >
> > > Looking at some other code, skb_gro_reset_offset() looks suspiciously
> > > like it also thinks highmem pages are special.
> > >
> > > Adding the networking people involved in this area to the cc too.

But there are three other kmap_atomic callers under net/ that do not
loop at all, so assume non-compound pages. In esp_output_head,
esp6_output_head and skb_seq_read. The first two directly use
skb_page_frag_refill, which can allocate compound (but not
__GFP_HIGHMEM) pages, and the third can be inserted with
netfilter xt_string in the path of tcp transmit skbs, which can also
have compound pages. I think that these could similarly access
data beyond the end of the kmap_atomic mapped page. I'll take
a closer look.

Re: [BUG] from x86: Support kmap_local() forced debugging

[Linus Torvalds]

On Wed, Jan 6, 2021 at 8:45 PM Willem de Bruijn <willemb@google.com> wrote:
>
> But there are three other kmap_atomic callers under net/ that do not
> loop at all, so assume non-compound pages. In esp_output_head,
> esp6_output_head and skb_seq_read. The first two directly use
> skb_page_frag_refill, which can allocate compound (but not
> __GFP_HIGHMEM) pages, and the third can be inserted with
> netfilter xt_string in the path of tcp transmit skbs, which can also
> have compound pages. I think that these could similarly access
> data beyond the end of the kmap_atomic mapped page. I'll take
> a closer look.

Thanks.

Note that I have flushed my random one-liner patch from my system, and
expect to get a proper fix through the normal networking pulls.

And _if_ the networking people feel that my one-liner was the proper
fix, you can use it and add my sign-off if you want to, but it really
was more of a "this is the quick ugly fix for testing" rather than
anything else.

          Linus

Re: [BUG] from x86: Support kmap_local() forced debugging

[Steven Rostedt]

On Thu, 7 Jan 2021 11:47:02 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, Jan 6, 2021 at 8:45 PM Willem de Bruijn <willemb@google.com> wrote:
> >
> > But there are three other kmap_atomic callers under net/ that do not
> > loop at all, so assume non-compound pages. In esp_output_head,
> > esp6_output_head and skb_seq_read. The first two directly use
> > skb_page_frag_refill, which can allocate compound (but not
> > __GFP_HIGHMEM) pages, and the third can be inserted with
> > netfilter xt_string in the path of tcp transmit skbs, which can also
> > have compound pages. I think that these could similarly access
> > data beyond the end of the kmap_atomic mapped page. I'll take
> > a closer look.  
> 
> Thanks.
> 
> Note that I have flushed my random one-liner patch from my system, and
> expect to get a proper fix through the normal networking pulls.
> 
> And _if_ the networking people feel that my one-liner was the proper
> fix, you can use it and add my sign-off if you want to, but it really
> was more of a "this is the quick ugly fix for testing" rather than
> anything else.
> 

Please add:

  Link: https://lore.kernel.org/linux-mm/20210106180132.41dc249d@gandalf.local.home/
  Reported-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

And if you take Linus's patch, please add my:

  Tested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

and if you come up with another patch, please send it to me for testing.

Thanks!

-- Steve

Re: [BUG] from x86: Support kmap_local() forced debugging

[Willem de Bruijn]

On Thu, Jan 7, 2021 at 3:53 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu, 7 Jan 2021 11:47:02 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > On Wed, Jan 6, 2021 at 8:45 PM Willem de Bruijn <willemb@google.com> wrote:
> > >
> > > But there are three other kmap_atomic callers under net/ that do not
> > > loop at all, so assume non-compound pages. In esp_output_head,
> > > esp6_output_head and skb_seq_read. The first two directly use
> > > skb_page_frag_refill, which can allocate compound (but not
> > > __GFP_HIGHMEM) pages, and the third can be inserted with
> > > netfilter xt_string in the path of tcp transmit skbs, which can also
> > > have compound pages. I think that these could similarly access
> > > data beyond the end of the kmap_atomic mapped page. I'll take
> > > a closer look.
> >
> > Thanks.
> >
> > Note that I have flushed my random one-liner patch from my system, and
> > expect to get a proper fix through the normal networking pulls.
> >
> > And _if_ the networking people feel that my one-liner was the proper
> > fix, you can use it and add my sign-off if you want to, but it really
> > was more of a "this is the quick ugly fix for testing" rather than
> > anything else.

I do think it is the proper fix as is. If no one else has comments, I
can submit it through the net tree.

It won't address the other issues that became apparent only as a
result of this. I'm preparing separate patches for those.

> Please add:
>
>   Link: https://lore.kernel.org/linux-mm/20210106180132.41dc249d@gandalf.local.home/
>   Reported-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>
> And if you take Linus's patch, please add my:
>
>   Tested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>
> and if you come up with another patch, please send it to me for testing.
>
> Thanks!

Will do, thanks.