On Wed, Jan 6, 2021 at 3:01 PM Steven Rostedt [off-list ref] 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
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-01-07 01:17:58
On Wed, 6 Jan 2021 17:03:48 -0800
Linus Torvalds [off-list ref] 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
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-01-07 01:50:16
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..
From: Willem de Bruijn <willemb@google.com> Date: 2021-01-07 02:12:48
On Wed, Jan 6, 2021 at 8:49 PM Jakub Kicinski [off-list ref] wrote:
On Wed, 6 Jan 2021 17:03:48 -0800 Linus Torvalds wrote:
quoted
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!
From: Willem de Bruijn <willemb@google.com> Date: 2021-01-07 04:46:34
On Wed, Jan 6, 2021 at 9:11 PM Willem de Bruijn [off-list ref] wrote:
On Wed, Jan 6, 2021 at 8:49 PM Jakub Kicinski [off-list ref] wrote:
quoted
On Wed, 6 Jan 2021 17:03:48 -0800 Linus Torvalds wrote:
quoted
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.
On Wed, Jan 6, 2021 at 8:45 PM Willem de Bruijn [off-list ref] 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
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-01-07 20:53:41
On Thu, 7 Jan 2021 11:47:02 -0800
Linus Torvalds [off-list ref] wrote:
On Wed, Jan 6, 2021 at 8:45 PM Willem de Bruijn [off-list ref] wrote:
quoted
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) [off-list ref]
And if you take Linus's patch, please add my:
Tested-by: Steven Rostedt (VMware) [off-list ref]
and if you come up with another patch, please send it to me for testing.
Thanks!
-- Steve
From: Willem de Bruijn <willemb@google.com> Date: 2021-01-07 21:09:18
On Thu, Jan 7, 2021 at 3:53 PM Steven Rostedt [off-list ref] wrote:
On Thu, 7 Jan 2021 11:47:02 -0800
Linus Torvalds [off-list ref] wrote:
quoted
On Wed, Jan 6, 2021 at 8:45 PM Willem de Bruijn [off-list ref] wrote:
quoted
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) [off-list ref]
And if you take Linus's patch, please add my:
Tested-by: Steven Rostedt (VMware) [off-list ref]
and if you come up with another patch, please send it to me for testing.
Thanks!