On Sun, Apr 11, 2021 at 10:14 PM Guenter Roeck [off-list ref] wrote:
Qemu test results:
total: 460 pass: 459 fail: 1
Failed tests:
sh:rts7751r2dplus_defconfig:ata:net,virtio-net:rootfs
The failure bisects to commit 0f6925b3e8da ("virtio_net: Do not pull payload in
skb->head"). It is a spurious problem - the test passes roughly every other
time. When the failure is seen, udhcpc fails to get an IP address and aborts
with SIGTERM. So far I have only seen this with the "sh" architecture.
Hmm. Let's add in some more of the people involved in that commit, and
also netdev.
Nothing in there looks like it should have any interaction with
architecture, so that "it happens on sh" sounds odd, but maybe it's
some particular interaction with the qemu environment.
Linus
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2021-04-12 16:40:28
On Mon, Apr 12, 2021 at 09:28:28AM -0700, Linus Torvalds wrote:
On Sun, Apr 11, 2021 at 10:14 PM Guenter Roeck [off-list ref] wrote:
quoted
Qemu test results:
total: 460 pass: 459 fail: 1
Failed tests:
sh:rts7751r2dplus_defconfig:ata:net,virtio-net:rootfs
The failure bisects to commit 0f6925b3e8da ("virtio_net: Do not pull payload in
skb->head"). It is a spurious problem - the test passes roughly every other
time. When the failure is seen, udhcpc fails to get an IP address and aborts
with SIGTERM. So far I have only seen this with the "sh" architecture.
Hmm. Let's add in some more of the people involved in that commit, and
also netdev.
Nothing in there looks like it should have any interaction with
architecture, so that "it happens on sh" sounds odd, but maybe it's
some particular interaction with the qemu environment.
Linus
Yea Eric's been trying to debug this already. Let's give him a bit more time...
--
MST
From: Eric Dumazet <edumazet@google.com> Date: 2021-04-12 16:48:09
On Mon, Apr 12, 2021 at 6:28 PM Linus Torvalds
[off-list ref] wrote:
On Sun, Apr 11, 2021 at 10:14 PM Guenter Roeck [off-list ref] wrote:
quoted
Qemu test results:
total: 460 pass: 459 fail: 1
Failed tests:
sh:rts7751r2dplus_defconfig:ata:net,virtio-net:rootfs
The failure bisects to commit 0f6925b3e8da ("virtio_net: Do not pull payload in
skb->head"). It is a spurious problem - the test passes roughly every other
time. When the failure is seen, udhcpc fails to get an IP address and aborts
with SIGTERM. So far I have only seen this with the "sh" architecture.
Hmm. Let's add in some more of the people involved in that commit, and
also netdev.
Nothing in there looks like it should have any interaction with
architecture, so that "it happens on sh" sounds odd, but maybe it's
some particular interaction with the qemu environment.
Yes, maybe.
I spent few hours on this, and suspect a buggy memcpy() implementation
on SH, but this was not conclusive.
By pulling one extra byte, the problem goes away.
Strange thing is that the udhcpc process does not go past sendto().
From: Eric Dumazet <edumazet@google.com> Date: 2021-04-12 16:50:46
On Mon, Apr 12, 2021 at 6:31 PM Eric Dumazet [off-list ref] wrote:
On Mon, Apr 12, 2021 at 6:28 PM Linus Torvalds
[off-list ref] wrote:
quoted
On Sun, Apr 11, 2021 at 10:14 PM Guenter Roeck [off-list ref] wrote:
quoted
Qemu test results:
total: 460 pass: 459 fail: 1
Failed tests:
sh:rts7751r2dplus_defconfig:ata:net,virtio-net:rootfs
The failure bisects to commit 0f6925b3e8da ("virtio_net: Do not pull payload in
skb->head"). It is a spurious problem - the test passes roughly every other
time. When the failure is seen, udhcpc fails to get an IP address and aborts
with SIGTERM. So far I have only seen this with the "sh" architecture.
Hmm. Let's add in some more of the people involved in that commit, and
also netdev.
Nothing in there looks like it should have any interaction with
architecture, so that "it happens on sh" sounds odd, but maybe it's
some particular interaction with the qemu environment.
Yes, maybe.
I spent few hours on this, and suspect a buggy memcpy() implementation
on SH, but this was not conclusive.
By pulling one extra byte, the problem goes away.
Strange thing is that the udhcpc process does not go past sendto().
This is the patch working around the issue. Unfortunately I was not
able to root-cause it (I really suspect something on SH)
virtnet_info *vi,
/* Copy all frame if it fits skb->head, otherwise
* we let virtio_net_hdr_to_skb() and GRO pull headers as needed.
+ *
+ * Apparently, pulling only the Ethernet Header triggers a bug
on qemu-system-sh4.
+ * Since GRO aggregation really cares of IPv4/IPv6, pull 20 bytes
+ * more to work around this bug : These 20 bytes can not belong
+ * to UDP/TCP payload.
+ * As a bonus, this makes GRO slightly faster for IPv4 (one less copy).
*/
if (len <= skb_tailroom(skb))
copy = len;
else
- copy = ETH_HLEN + metasize;
+ copy = ETH_HLEN + sizeof(struct iphdr) + metasize;
skb_put_data(skb, p, copy);
if (metasize) {
On Mon, Apr 12, 2021 at 6:28 PM Linus Torvalds
[off-list ref] wrote:
quoted
On Sun, Apr 11, 2021 at 10:14 PM Guenter Roeck [off-list ref] wrote:
quoted
Qemu test results:
total: 460 pass: 459 fail: 1
Failed tests:
sh:rts7751r2dplus_defconfig:ata:net,virtio-net:rootfs
The failure bisects to commit 0f6925b3e8da ("virtio_net: Do not pull payload in
skb->head"). It is a spurious problem - the test passes roughly every other
time. When the failure is seen, udhcpc fails to get an IP address and aborts
with SIGTERM. So far I have only seen this with the "sh" architecture.
Hmm. Let's add in some more of the people involved in that commit, and
also netdev.
Nothing in there looks like it should have any interaction with
architecture, so that "it happens on sh" sounds odd, but maybe it's
some particular interaction with the qemu environment.
Yes, maybe.
I spent few hours on this, and suspect a buggy memcpy() implementation
on SH, but this was not conclusive.
I replaced all memcpy() calls in skbuff.h with calls to
static inline void __my_memcpy(unsigned char *to, const unsigned char *from,
unsigned int len)
{
while (len--)
*to++ = *from++;
}
That made no difference, so unless you have some other memcpy() in mind that
seems to be unlikely.
By pulling one extra byte, the problem goes away.
Strange thing is that the udhcpc process does not go past sendto().
I have been trying to debug that one. Unfortunately gdb doesn't work with sh,
so I can't use it to debug the problem. I'll spend some more time on this today.
Thanks,
Guenter
From: Eric Dumazet <edumazet@google.com> Date: 2021-04-12 17:38:58
On Mon, Apr 12, 2021 at 7:31 PM Guenter Roeck [off-list ref] wrote:
On 4/12/21 9:31 AM, Eric Dumazet wrote:
quoted
On Mon, Apr 12, 2021 at 6:28 PM Linus Torvalds
[off-list ref] wrote:
quoted
On Sun, Apr 11, 2021 at 10:14 PM Guenter Roeck [off-list ref] wrote:
quoted
Qemu test results:
total: 460 pass: 459 fail: 1
Failed tests:
sh:rts7751r2dplus_defconfig:ata:net,virtio-net:rootfs
The failure bisects to commit 0f6925b3e8da ("virtio_net: Do not pull payload in
skb->head"). It is a spurious problem - the test passes roughly every other
time. When the failure is seen, udhcpc fails to get an IP address and aborts
with SIGTERM. So far I have only seen this with the "sh" architecture.
Hmm. Let's add in some more of the people involved in that commit, and
also netdev.
Nothing in there looks like it should have any interaction with
architecture, so that "it happens on sh" sounds odd, but maybe it's
some particular interaction with the qemu environment.
Yes, maybe.
I spent few hours on this, and suspect a buggy memcpy() implementation
on SH, but this was not conclusive.
I replaced all memcpy() calls in skbuff.h with calls to
static inline void __my_memcpy(unsigned char *to, const unsigned char *from,
unsigned int len)
{
while (len--)
*to++ = *from++;
}
That made no difference, so unless you have some other memcpy() in mind that
seems to be unlikely.
By pulling one extra byte, the problem goes away.
Strange thing is that the udhcpc process does not go past sendto().
I have been trying to debug that one. Unfortunately gdb doesn't work with sh,
so I can't use it to debug the problem. I'll spend some more time on this today.
Yes, I think this is the real issue here. This smells like some memory
corruption.
In my traces, packet is correctly received in AF_PACKET queue.
I have checked the skb is well formed.
But the user space seems to never call poll() and recvmsg() on this
af_packet socket.
Yes, I think this is the real issue here. This smells like some memory
corruption.
In my traces, packet is correctly received in AF_PACKET queue.
I have checked the skb is well formed.
But the user space seems to never call poll() and recvmsg() on this
af_packet socket.
After sprinkling the kernel with debug messages:
424 00:01:33.674181 sendto(6, "E\0\1H\0\0\0\0@\21y\246\0\0\0\0\377\377\377\377\0D\0C\00148\346\1\1\6\0\246\336\333\v\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0RT\0\
424 00:01:33.693873 close(6) = 0
424 00:01:33.694652 fcntl64(5, F_SETFD, FD_CLOEXEC) = 0
424 00:01:33.695213 clock_gettime64(CLOCK_MONOTONIC, 0x7be18a18) = -1 EFAULT (Bad address)
424 00:01:33.695889 write(2, "udhcpc: clock_gettime(MONOTONIC) failed\n", 40) = -1 EFAULT (Bad address)
424 00:01:33.697311 exit_group(1) = ?
424 00:01:33.698346 +++ exited with 1 +++
I only see that after adding debug messages in the kernel, so I guess there must be
a heisenbug somehere.
Anyway, indeed, I see (another kernel debug message):
__do_sys_clock_gettime: Returning -EFAULT on address 0x7bacc9a8
So udhcpc doesn't even try to read the reply because it crashes after sendto()
when trying to read the current time. Unless I am missing something, that means
that the problem happens somewhere on the send side.
To make things even more interesting, it looks like the failing system call
isn't always clock_gettime().
Guenter
From: Eric Dumazet <edumazet@google.com> Date: 2021-04-13 09:24:38
On Mon, Apr 12, 2021 at 10:05 PM Guenter Roeck [off-list ref] wrote:
On 4/12/21 10:38 AM, Eric Dumazet wrote:
[ ... ]
quoted
Yes, I think this is the real issue here. This smells like some memory
corruption.
In my traces, packet is correctly received in AF_PACKET queue.
I have checked the skb is well formed.
But the user space seems to never call poll() and recvmsg() on this
af_packet socket.
After sprinkling the kernel with debug messages:
424 00:01:33.674181 sendto(6, "E\0\1H\0\0\0\0@\21y\246\0\0\0\0\377\377\377\377\0D\0C\00148\346\1\1\6\0\246\336\333\v\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0RT\0\
424 00:01:33.693873 close(6) = 0
424 00:01:33.694652 fcntl64(5, F_SETFD, FD_CLOEXEC) = 0
424 00:01:33.695213 clock_gettime64(CLOCK_MONOTONIC, 0x7be18a18) = -1 EFAULT (Bad address)
424 00:01:33.695889 write(2, "udhcpc: clock_gettime(MONOTONIC) failed\n", 40) = -1 EFAULT (Bad address)
424 00:01:33.697311 exit_group(1) = ?
424 00:01:33.698346 +++ exited with 1 +++
I only see that after adding debug messages in the kernel, so I guess there must be
a heisenbug somehere.
Anyway, indeed, I see (another kernel debug message):
__do_sys_clock_gettime: Returning -EFAULT on address 0x7bacc9a8
So udhcpc doesn't even try to read the reply because it crashes after sendto()
when trying to read the current time. Unless I am missing something, that means
that the problem happens somewhere on the send side.
To make things even more interesting, it looks like the failing system call
isn't always clock_gettime().
Guenter
I think GRO fast path has never worked on SUPERH. Probably SUPERH has
never used a fast NIC (10Gbit+)
The following hack fixes the issue.
From: Eric Dumazet <edumazet@google.com> Date: 2021-04-13 10:43:41
On Tue, Apr 13, 2021 at 11:24 AM Eric Dumazet [off-list ref] wrote:
quoted hunk
On Mon, Apr 12, 2021 at 10:05 PM Guenter Roeck [off-list ref] wrote:
quoted
On 4/12/21 10:38 AM, Eric Dumazet wrote:
[ ... ]
quoted
Yes, I think this is the real issue here. This smells like some memory
corruption.
In my traces, packet is correctly received in AF_PACKET queue.
I have checked the skb is well formed.
But the user space seems to never call poll() and recvmsg() on this
af_packet socket.
After sprinkling the kernel with debug messages:
424 00:01:33.674181 sendto(6, "E\0\1H\0\0\0\0@\21y\246\0\0\0\0\377\377\377\377\0D\0C\00148\346\1\1\6\0\246\336\333\v\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0RT\0\
424 00:01:33.693873 close(6) = 0
424 00:01:33.694652 fcntl64(5, F_SETFD, FD_CLOEXEC) = 0
424 00:01:33.695213 clock_gettime64(CLOCK_MONOTONIC, 0x7be18a18) = -1 EFAULT (Bad address)
424 00:01:33.695889 write(2, "udhcpc: clock_gettime(MONOTONIC) failed\n", 40) = -1 EFAULT (Bad address)
424 00:01:33.697311 exit_group(1) = ?
424 00:01:33.698346 +++ exited with 1 +++
I only see that after adding debug messages in the kernel, so I guess there must be
a heisenbug somehere.
Anyway, indeed, I see (another kernel debug message):
__do_sys_clock_gettime: Returning -EFAULT on address 0x7bacc9a8
So udhcpc doesn't even try to read the reply because it crashes after sendto()
when trying to read the current time. Unless I am missing something, that means
that the problem happens somewhere on the send side.
To make things even more interesting, it looks like the failing system call
isn't always clock_gettime().
Guenter
I think GRO fast path has never worked on SUPERH. Probably SUPERH has
never used a fast NIC (10Gbit+)
The following hack fixes the issue.
@@ -27,7 +27,7 @@ static unsigned long se_multi;valid!*/staticintse_usermode=UM_WARN|UM_FIXUP;/* 0: no warning 1: print a warning message, disabled by default */-staticintse_kernmode_warn;+staticintse_kernmode_warn=1;core_param(alignment,se_usermode,int,0600);
*tsk, insn_size_t insn,
(void *)instruction_pointer(regs), insn);
else if (se_kernmode_warn)
pr_notice_ratelimited("Fixing up unaligned kernel access "
- "in \"%s\" pid=%d pc=0x%p ins=0x%04hx\n",
+ "in \"%s\" pid=%d pc=%px ins=0x%04hx\n",
tsk->comm, task_pid_nr(tsk),
(void *)instruction_pointer(regs), insn);
}
I now see something of interest :
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc2e ins=0x6236
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc2e ins=0x6236
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc30 ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc30 ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
So basically the frag0 idea only works if drivers respect NET_IP_ALIGN
(So that IP header is 4-byte aligned)
It seems either virtio_net or qemu does not respect the contract.
A possible generic fix would then be :
From: Eric Dumazet <edumazet@google.com> Date: 2021-04-13 12:47:08
On Tue, Apr 13, 2021 at 12:43 PM Eric Dumazet [off-list ref] wrote:
quoted hunk
On Tue, Apr 13, 2021 at 11:24 AM Eric Dumazet [off-list ref] wrote:
quoted
On Mon, Apr 12, 2021 at 10:05 PM Guenter Roeck [off-list ref] wrote:
quoted
On 4/12/21 10:38 AM, Eric Dumazet wrote:
[ ... ]
quoted
Yes, I think this is the real issue here. This smells like some memory
corruption.
In my traces, packet is correctly received in AF_PACKET queue.
I have checked the skb is well formed.
But the user space seems to never call poll() and recvmsg() on this
af_packet socket.
After sprinkling the kernel with debug messages:
424 00:01:33.674181 sendto(6, "E\0\1H\0\0\0\0@\21y\246\0\0\0\0\377\377\377\377\0D\0C\00148\346\1\1\6\0\246\336\333\v\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0RT\0\
424 00:01:33.693873 close(6) = 0
424 00:01:33.694652 fcntl64(5, F_SETFD, FD_CLOEXEC) = 0
424 00:01:33.695213 clock_gettime64(CLOCK_MONOTONIC, 0x7be18a18) = -1 EFAULT (Bad address)
424 00:01:33.695889 write(2, "udhcpc: clock_gettime(MONOTONIC) failed\n", 40) = -1 EFAULT (Bad address)
424 00:01:33.697311 exit_group(1) = ?
424 00:01:33.698346 +++ exited with 1 +++
I only see that after adding debug messages in the kernel, so I guess there must be
a heisenbug somehere.
Anyway, indeed, I see (another kernel debug message):
__do_sys_clock_gettime: Returning -EFAULT on address 0x7bacc9a8
So udhcpc doesn't even try to read the reply because it crashes after sendto()
when trying to read the current time. Unless I am missing something, that means
that the problem happens somewhere on the send side.
To make things even more interesting, it looks like the failing system call
isn't always clock_gettime().
Guenter
I think GRO fast path has never worked on SUPERH. Probably SUPERH has
never used a fast NIC (10Gbit+)
The following hack fixes the issue.
@@ -27,7 +27,7 @@ static unsigned long se_multi;valid!*/staticintse_usermode=UM_WARN|UM_FIXUP;/* 0: no warning 1: print a warning message, disabled by default */-staticintse_kernmode_warn;+staticintse_kernmode_warn=1;core_param(alignment,se_usermode,int,0600);
*tsk, insn_size_t insn,
(void *)instruction_pointer(regs), insn);
else if (se_kernmode_warn)
pr_notice_ratelimited("Fixing up unaligned kernel access "
- "in \"%s\" pid=%d pc=0x%p ins=0x%04hx\n",
+ "in \"%s\" pid=%d pc=%px ins=0x%04hx\n",
tsk->comm, task_pid_nr(tsk),
(void *)instruction_pointer(regs), insn);
}
I now see something of interest :
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc2e ins=0x6236
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc2e ins=0x6236
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc30 ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc30 ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
So basically the frag0 idea only works if drivers respect NET_IP_ALIGN
(So that IP header is 4-byte aligned)
It seems either virtio_net or qemu does not respect the contract.
A possible generic fix would then be :
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2021-04-13 12:52:42
On Tue, Apr 13, 2021 at 02:45:46PM +0200, Eric Dumazet wrote:
On Tue, Apr 13, 2021 at 12:43 PM Eric Dumazet [off-list ref] wrote:
quoted
On Tue, Apr 13, 2021 at 11:24 AM Eric Dumazet [off-list ref] wrote:
quoted
On Mon, Apr 12, 2021 at 10:05 PM Guenter Roeck [off-list ref] wrote:
quoted
On 4/12/21 10:38 AM, Eric Dumazet wrote:
[ ... ]
quoted
Yes, I think this is the real issue here. This smells like some memory
corruption.
In my traces, packet is correctly received in AF_PACKET queue.
I have checked the skb is well formed.
But the user space seems to never call poll() and recvmsg() on this
af_packet socket.
After sprinkling the kernel with debug messages:
424 00:01:33.674181 sendto(6, "E\0\1H\0\0\0\0@\21y\246\0\0\0\0\377\377\377\377\0D\0C\00148\346\1\1\6\0\246\336\333\v\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0RT\0\
424 00:01:33.693873 close(6) = 0
424 00:01:33.694652 fcntl64(5, F_SETFD, FD_CLOEXEC) = 0
424 00:01:33.695213 clock_gettime64(CLOCK_MONOTONIC, 0x7be18a18) = -1 EFAULT (Bad address)
424 00:01:33.695889 write(2, "udhcpc: clock_gettime(MONOTONIC) failed\n", 40) = -1 EFAULT (Bad address)
424 00:01:33.697311 exit_group(1) = ?
424 00:01:33.698346 +++ exited with 1 +++
I only see that after adding debug messages in the kernel, so I guess there must be
a heisenbug somehere.
Anyway, indeed, I see (another kernel debug message):
__do_sys_clock_gettime: Returning -EFAULT on address 0x7bacc9a8
So udhcpc doesn't even try to read the reply because it crashes after sendto()
when trying to read the current time. Unless I am missing something, that means
that the problem happens somewhere on the send side.
To make things even more interesting, it looks like the failing system call
isn't always clock_gettime().
Guenter
I think GRO fast path has never worked on SUPERH. Probably SUPERH has
never used a fast NIC (10Gbit+)
The following hack fixes the issue.
@@ -27,7 +27,7 @@ static unsigned long se_multi;valid!*/staticintse_usermode=UM_WARN|UM_FIXUP;/* 0: no warning 1: print a warning message, disabled by default */-staticintse_kernmode_warn;+staticintse_kernmode_warn=1;core_param(alignment,se_usermode,int,0600);
*tsk, insn_size_t insn,
(void *)instruction_pointer(regs), insn);
else if (se_kernmode_warn)
pr_notice_ratelimited("Fixing up unaligned kernel access "
- "in \"%s\" pid=%d pc=0x%p ins=0x%04hx\n",
+ "in \"%s\" pid=%d pc=%px ins=0x%04hx\n",
tsk->comm, task_pid_nr(tsk),
(void *)instruction_pointer(regs), insn);
}
I now see something of interest :
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc2e ins=0x6236
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc2e ins=0x6236
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc30 ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc30 ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
Fixing up unaligned kernel access in "udhcpc" pid=91 pc=8c43fc3a ins=0x6636
So basically the frag0 idea only works if drivers respect NET_IP_ALIGN
(So that IP header is 4-byte aligned)
It seems either virtio_net or qemu does not respect the contract.
A possible generic fix would then be :
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2021-04-13 12:57:27
On Mon, Apr 12, 2021 at 06:47:07PM +0200, Eric Dumazet wrote:
quoted hunk
On Mon, Apr 12, 2021 at 6:31 PM Eric Dumazet [off-list ref] wrote:
quoted
On Mon, Apr 12, 2021 at 6:28 PM Linus Torvalds
[off-list ref] wrote:
quoted
On Sun, Apr 11, 2021 at 10:14 PM Guenter Roeck [off-list ref] wrote:
quoted
Qemu test results:
total: 460 pass: 459 fail: 1
Failed tests:
sh:rts7751r2dplus_defconfig:ata:net,virtio-net:rootfs
The failure bisects to commit 0f6925b3e8da ("virtio_net: Do not pull payload in
skb->head"). It is a spurious problem - the test passes roughly every other
time. When the failure is seen, udhcpc fails to get an IP address and aborts
with SIGTERM. So far I have only seen this with the "sh" architecture.
Hmm. Let's add in some more of the people involved in that commit, and
also netdev.
Nothing in there looks like it should have any interaction with
architecture, so that "it happens on sh" sounds odd, but maybe it's
some particular interaction with the qemu environment.
Yes, maybe.
I spent few hours on this, and suspect a buggy memcpy() implementation
on SH, but this was not conclusive.
By pulling one extra byte, the problem goes away.
Strange thing is that the udhcpc process does not go past sendto().
This is the patch working around the issue. Unfortunately I was not
able to root-cause it (I really suspect something on SH)
virtnet_info *vi,
/* Copy all frame if it fits skb->head, otherwise
* we let virtio_net_hdr_to_skb() and GRO pull headers as needed.
+ *
+ * Apparently, pulling only the Ethernet Header triggers a bug
on qemu-system-sh4.
+ * Since GRO aggregation really cares of IPv4/IPv6, pull 20 bytes
+ * more to work around this bug : These 20 bytes can not belong
+ * to UDP/TCP payload.
+ * As a bonus, this makes GRO slightly faster for IPv4 (one less copy).
*/
Question: do we still want to do this for performance reasons?
We also have the hdr_len coming from the device which is
just skb_headlen on the host.
From: Eric Dumazet <edumazet@google.com> Date: 2021-04-13 13:27:56
On Tue, Apr 13, 2021 at 2:57 PM Michael S. Tsirkin [off-list ref] wrote:
On Mon, Apr 12, 2021 at 06:47:07PM +0200, Eric Dumazet wrote:
quoted
On Mon, Apr 12, 2021 at 6:31 PM Eric Dumazet [off-list ref] wrote:
quoted
On Mon, Apr 12, 2021 at 6:28 PM Linus Torvalds
[off-list ref] wrote:
quoted
On Sun, Apr 11, 2021 at 10:14 PM Guenter Roeck [off-list ref] wrote:
quoted
Qemu test results:
total: 460 pass: 459 fail: 1
Failed tests:
sh:rts7751r2dplus_defconfig:ata:net,virtio-net:rootfs
The failure bisects to commit 0f6925b3e8da ("virtio_net: Do not pull payload in
skb->head"). It is a spurious problem - the test passes roughly every other
time. When the failure is seen, udhcpc fails to get an IP address and aborts
with SIGTERM. So far I have only seen this with the "sh" architecture.
Hmm. Let's add in some more of the people involved in that commit, and
also netdev.
Nothing in there looks like it should have any interaction with
architecture, so that "it happens on sh" sounds odd, but maybe it's
some particular interaction with the qemu environment.
Yes, maybe.
I spent few hours on this, and suspect a buggy memcpy() implementation
on SH, but this was not conclusive.
By pulling one extra byte, the problem goes away.
Strange thing is that the udhcpc process does not go past sendto().
This is the patch working around the issue. Unfortunately I was not
able to root-cause it (I really suspect something on SH)
virtnet_info *vi,
/* Copy all frame if it fits skb->head, otherwise
* we let virtio_net_hdr_to_skb() and GRO pull headers as needed.
+ *
+ * Apparently, pulling only the Ethernet Header triggers a bug
on qemu-system-sh4.
+ * Since GRO aggregation really cares of IPv4/IPv6, pull 20 bytes
+ * more to work around this bug : These 20 bytes can not belong
+ * to UDP/TCP payload.
+ * As a bonus, this makes GRO slightly faster for IPv4 (one less copy).
*/
Question: do we still want to do this for performance reasons?
We also have the hdr_len coming from the device which is
just skb_headlen on the host.
Well, putting 20 bytes in skb->head will disable frag0 optimization.
The change would only benefit to sh architecture :)
About hdr_len, I suppose we could try it, with appropriate safety checks.
From: Eric Dumazet <edumazet@google.com> Date: 2021-04-13 13:34:04
On Tue, Apr 13, 2021 at 3:27 PM Eric Dumazet [off-list ref] wrote:
On Tue, Apr 13, 2021 at 2:57 PM Michael S. Tsirkin [off-list ref] wrote:
quoted
On Mon, Apr 12, 2021 at 06:47:07PM +0200, Eric Dumazet wrote:
quoted
On Mon, Apr 12, 2021 at 6:31 PM Eric Dumazet [off-list ref] wrote:
quoted
On Mon, Apr 12, 2021 at 6:28 PM Linus Torvalds
[off-list ref] wrote:
quoted
On Sun, Apr 11, 2021 at 10:14 PM Guenter Roeck [off-list ref] wrote:
quoted
Qemu test results:
total: 460 pass: 459 fail: 1
Failed tests:
sh:rts7751r2dplus_defconfig:ata:net,virtio-net:rootfs
The failure bisects to commit 0f6925b3e8da ("virtio_net: Do not pull payload in
skb->head"). It is a spurious problem - the test passes roughly every other
time. When the failure is seen, udhcpc fails to get an IP address and aborts
with SIGTERM. So far I have only seen this with the "sh" architecture.
Hmm. Let's add in some more of the people involved in that commit, and
also netdev.
Nothing in there looks like it should have any interaction with
architecture, so that "it happens on sh" sounds odd, but maybe it's
some particular interaction with the qemu environment.
Yes, maybe.
I spent few hours on this, and suspect a buggy memcpy() implementation
on SH, but this was not conclusive.
By pulling one extra byte, the problem goes away.
Strange thing is that the udhcpc process does not go past sendto().
This is the patch working around the issue. Unfortunately I was not
able to root-cause it (I really suspect something on SH)
virtnet_info *vi,
/* Copy all frame if it fits skb->head, otherwise
* we let virtio_net_hdr_to_skb() and GRO pull headers as needed.
+ *
+ * Apparently, pulling only the Ethernet Header triggers a bug
on qemu-system-sh4.
+ * Since GRO aggregation really cares of IPv4/IPv6, pull 20 bytes
+ * more to work around this bug : These 20 bytes can not belong
+ * to UDP/TCP payload.
+ * As a bonus, this makes GRO slightly faster for IPv4 (one less copy).
*/
Question: do we still want to do this for performance reasons?
We also have the hdr_len coming from the device which is
just skb_headlen on the host.
Well, putting 20 bytes in skb->head will disable frag0 optimization.
The change would only benefit to sh architecture :)
About hdr_len, I suppose we could try it, with appropriate safety checks.
I have added traces, hdr_len seems to be 0 with the qemu-system-sh4 I am using.
Have I understood you correctly ?
@@ -399,9 +399,10 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,hdr_padded_len=sizeof(structpadded_vnet_hdr);/* hdr_valid means no XDP, so we can copy the vnet header */-if(hdr_valid)+if(hdr_valid){memcpy(hdr,p,hdr_len);-+pr_err("hdr->hdr_len=%u\n",hdr->hdr.hdr_len);+}len-=hdr_len;offset+=hdr_padded_len;p+=hdr_padded_len;
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2021-04-13 13:40:27
On Tue, Apr 13, 2021 at 03:33:40PM +0200, Eric Dumazet wrote:
quoted hunk
On Tue, Apr 13, 2021 at 3:27 PM Eric Dumazet [off-list ref] wrote:
quoted
On Tue, Apr 13, 2021 at 2:57 PM Michael S. Tsirkin [off-list ref] wrote:
quoted
On Mon, Apr 12, 2021 at 06:47:07PM +0200, Eric Dumazet wrote:
quoted
On Mon, Apr 12, 2021 at 6:31 PM Eric Dumazet [off-list ref] wrote:
quoted
On Mon, Apr 12, 2021 at 6:28 PM Linus Torvalds
[off-list ref] wrote:
quoted
On Sun, Apr 11, 2021 at 10:14 PM Guenter Roeck [off-list ref] wrote:
quoted
Qemu test results:
total: 460 pass: 459 fail: 1
Failed tests:
sh:rts7751r2dplus_defconfig:ata:net,virtio-net:rootfs
The failure bisects to commit 0f6925b3e8da ("virtio_net: Do not pull payload in
skb->head"). It is a spurious problem - the test passes roughly every other
time. When the failure is seen, udhcpc fails to get an IP address and aborts
with SIGTERM. So far I have only seen this with the "sh" architecture.
Hmm. Let's add in some more of the people involved in that commit, and
also netdev.
Nothing in there looks like it should have any interaction with
architecture, so that "it happens on sh" sounds odd, but maybe it's
some particular interaction with the qemu environment.
Yes, maybe.
I spent few hours on this, and suspect a buggy memcpy() implementation
on SH, but this was not conclusive.
By pulling one extra byte, the problem goes away.
Strange thing is that the udhcpc process does not go past sendto().
This is the patch working around the issue. Unfortunately I was not
able to root-cause it (I really suspect something on SH)
virtnet_info *vi,
/* Copy all frame if it fits skb->head, otherwise
* we let virtio_net_hdr_to_skb() and GRO pull headers as needed.
+ *
+ * Apparently, pulling only the Ethernet Header triggers a bug
on qemu-system-sh4.
+ * Since GRO aggregation really cares of IPv4/IPv6, pull 20 bytes
+ * more to work around this bug : These 20 bytes can not belong
+ * to UDP/TCP payload.
+ * As a bonus, this makes GRO slightly faster for IPv4 (one less copy).
*/
Question: do we still want to do this for performance reasons?
We also have the hdr_len coming from the device which is
just skb_headlen on the host.
Well, putting 20 bytes in skb->head will disable frag0 optimization.
The change would only benefit to sh architecture :)
About hdr_len, I suppose we could try it, with appropriate safety checks.
I have added traces, hdr_len seems to be 0 with the qemu-system-sh4 I am using.
Have I understood you correctly ?
@@ -399,9 +399,10 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,hdr_padded_len=sizeof(structpadded_vnet_hdr);/* hdr_valid means no XDP, so we can copy the vnet header */-if(hdr_valid)+if(hdr_valid){memcpy(hdr,p,hdr_len);-+pr_err("hdr->hdr_len=%u\n",hdr->hdr.hdr_len);+}len-=hdr_len;offset+=hdr_padded_len;p+=hdr_padded_len;
Depends on how you connect qemu on the host. It's filled by host tap,
see virtio_net_hdr_from_skb. If you are using slirp that just zero-fills
it.
--
MST
From: Eric Dumazet <edumazet@google.com> Date: 2021-04-13 13:42:40
On Tue, Apr 13, 2021 at 3:38 PM Michael S. Tsirkin [off-list ref] wrote:
On Tue, Apr 13, 2021 at 03:33:40PM +0200, Eric Dumazet wrote:
quoted
On Tue, Apr 13, 2021 at 3:27 PM Eric Dumazet [off-list ref] wrote:
quoted
On Tue, Apr 13, 2021 at 2:57 PM Michael S. Tsirkin [off-list ref] wrote:
quoted
On Mon, Apr 12, 2021 at 06:47:07PM +0200, Eric Dumazet wrote:
quoted
On Mon, Apr 12, 2021 at 6:31 PM Eric Dumazet [off-list ref] wrote:
quoted
On Mon, Apr 12, 2021 at 6:28 PM Linus Torvalds
[off-list ref] wrote:
quoted
On Sun, Apr 11, 2021 at 10:14 PM Guenter Roeck [off-list ref] wrote:
quoted
Qemu test results:
total: 460 pass: 459 fail: 1
Failed tests:
sh:rts7751r2dplus_defconfig:ata:net,virtio-net:rootfs
The failure bisects to commit 0f6925b3e8da ("virtio_net: Do not pull payload in
skb->head"). It is a spurious problem - the test passes roughly every other
time. When the failure is seen, udhcpc fails to get an IP address and aborts
with SIGTERM. So far I have only seen this with the "sh" architecture.
Hmm. Let's add in some more of the people involved in that commit, and
also netdev.
Nothing in there looks like it should have any interaction with
architecture, so that "it happens on sh" sounds odd, but maybe it's
some particular interaction with the qemu environment.
Yes, maybe.
I spent few hours on this, and suspect a buggy memcpy() implementation
on SH, but this was not conclusive.
By pulling one extra byte, the problem goes away.
Strange thing is that the udhcpc process does not go past sendto().
This is the patch working around the issue. Unfortunately I was not
able to root-cause it (I really suspect something on SH)
virtnet_info *vi,
/* Copy all frame if it fits skb->head, otherwise
* we let virtio_net_hdr_to_skb() and GRO pull headers as needed.
+ *
+ * Apparently, pulling only the Ethernet Header triggers a bug
on qemu-system-sh4.
+ * Since GRO aggregation really cares of IPv4/IPv6, pull 20 bytes
+ * more to work around this bug : These 20 bytes can not belong
+ * to UDP/TCP payload.
+ * As a bonus, this makes GRO slightly faster for IPv4 (one less copy).
*/
Question: do we still want to do this for performance reasons?
We also have the hdr_len coming from the device which is
just skb_headlen on the host.
Well, putting 20 bytes in skb->head will disable frag0 optimization.
The change would only benefit to sh architecture :)
About hdr_len, I suppose we could try it, with appropriate safety checks.
I have added traces, hdr_len seems to be 0 with the qemu-system-sh4 I am using.
Have I understood you correctly ?
@@ -399,9 +399,10 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,hdr_padded_len=sizeof(structpadded_vnet_hdr);/* hdr_valid means no XDP, so we can copy the vnet header */-if(hdr_valid)+if(hdr_valid){memcpy(hdr,p,hdr_len);-+pr_err("hdr->hdr_len=%u\n",hdr->hdr.hdr_len);+}len-=hdr_len;offset+=hdr_padded_len;p+=hdr_padded_len;
Depends on how you connect qemu on the host. It's filled by host tap,
see virtio_net_hdr_from_skb. If you are using slirp that just zero-fills
it.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2021-04-13 13:46:51
On Tue, Apr 13, 2021 at 03:42:24PM +0200, Eric Dumazet wrote:
On Tue, Apr 13, 2021 at 3:38 PM Michael S. Tsirkin [off-list ref] wrote:
quoted
On Tue, Apr 13, 2021 at 03:33:40PM +0200, Eric Dumazet wrote:
quoted
On Tue, Apr 13, 2021 at 3:27 PM Eric Dumazet [off-list ref] wrote:
quoted
On Tue, Apr 13, 2021 at 2:57 PM Michael S. Tsirkin [off-list ref] wrote:
quoted
On Mon, Apr 12, 2021 at 06:47:07PM +0200, Eric Dumazet wrote:
quoted
On Mon, Apr 12, 2021 at 6:31 PM Eric Dumazet [off-list ref] wrote:
quoted
On Mon, Apr 12, 2021 at 6:28 PM Linus Torvalds
[off-list ref] wrote:
quoted
On Sun, Apr 11, 2021 at 10:14 PM Guenter Roeck [off-list ref] wrote:
quoted
Qemu test results:
total: 460 pass: 459 fail: 1
Failed tests:
sh:rts7751r2dplus_defconfig:ata:net,virtio-net:rootfs
The failure bisects to commit 0f6925b3e8da ("virtio_net: Do not pull payload in
skb->head"). It is a spurious problem - the test passes roughly every other
time. When the failure is seen, udhcpc fails to get an IP address and aborts
with SIGTERM. So far I have only seen this with the "sh" architecture.
Hmm. Let's add in some more of the people involved in that commit, and
also netdev.
Nothing in there looks like it should have any interaction with
architecture, so that "it happens on sh" sounds odd, but maybe it's
some particular interaction with the qemu environment.
Yes, maybe.
I spent few hours on this, and suspect a buggy memcpy() implementation
on SH, but this was not conclusive.
By pulling one extra byte, the problem goes away.
Strange thing is that the udhcpc process does not go past sendto().
This is the patch working around the issue. Unfortunately I was not
able to root-cause it (I really suspect something on SH)
virtnet_info *vi,
/* Copy all frame if it fits skb->head, otherwise
* we let virtio_net_hdr_to_skb() and GRO pull headers as needed.
+ *
+ * Apparently, pulling only the Ethernet Header triggers a bug
on qemu-system-sh4.
+ * Since GRO aggregation really cares of IPv4/IPv6, pull 20 bytes
+ * more to work around this bug : These 20 bytes can not belong
+ * to UDP/TCP payload.
+ * As a bonus, this makes GRO slightly faster for IPv4 (one less copy).
*/
Question: do we still want to do this for performance reasons?
We also have the hdr_len coming from the device which is
just skb_headlen on the host.
Well, putting 20 bytes in skb->head will disable frag0 optimization.
The change would only benefit to sh architecture :)
About hdr_len, I suppose we could try it, with appropriate safety checks.
I have added traces, hdr_len seems to be 0 with the qemu-system-sh4 I am using.
Have I understood you correctly ?
@@ -399,9 +399,10 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,hdr_padded_len=sizeof(structpadded_vnet_hdr);/* hdr_valid means no XDP, so we can copy the vnet header */-if(hdr_valid)+if(hdr_valid){memcpy(hdr,p,hdr_len);-+pr_err("hdr->hdr_len=%u\n",hdr->hdr.hdr_len);+}len-=hdr_len;offset+=hdr_padded_len;p+=hdr_padded_len;
Depends on how you connect qemu on the host. It's filled by host tap,
see virtio_net_hdr_from_skb. If you are using slirp that just zero-fills
it.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2021-04-13 13:58:34
On Tue, Apr 13, 2021 at 03:42:24PM +0200, Eric Dumazet wrote:
On Tue, Apr 13, 2021 at 3:38 PM Michael S. Tsirkin [off-list ref] wrote:
quoted
On Tue, Apr 13, 2021 at 03:33:40PM +0200, Eric Dumazet wrote:
quoted
On Tue, Apr 13, 2021 at 3:27 PM Eric Dumazet [off-list ref] wrote:
quoted
On Tue, Apr 13, 2021 at 2:57 PM Michael S. Tsirkin [off-list ref] wrote:
quoted
On Mon, Apr 12, 2021 at 06:47:07PM +0200, Eric Dumazet wrote:
quoted
On Mon, Apr 12, 2021 at 6:31 PM Eric Dumazet [off-list ref] wrote:
quoted
On Mon, Apr 12, 2021 at 6:28 PM Linus Torvalds
[off-list ref] wrote:
quoted
On Sun, Apr 11, 2021 at 10:14 PM Guenter Roeck [off-list ref] wrote:
quoted
Qemu test results:
total: 460 pass: 459 fail: 1
Failed tests:
sh:rts7751r2dplus_defconfig:ata:net,virtio-net:rootfs
The failure bisects to commit 0f6925b3e8da ("virtio_net: Do not pull payload in
skb->head"). It is a spurious problem - the test passes roughly every other
time. When the failure is seen, udhcpc fails to get an IP address and aborts
with SIGTERM. So far I have only seen this with the "sh" architecture.
Hmm. Let's add in some more of the people involved in that commit, and
also netdev.
Nothing in there looks like it should have any interaction with
architecture, so that "it happens on sh" sounds odd, but maybe it's
some particular interaction with the qemu environment.
Yes, maybe.
I spent few hours on this, and suspect a buggy memcpy() implementation
on SH, but this was not conclusive.
By pulling one extra byte, the problem goes away.
Strange thing is that the udhcpc process does not go past sendto().
This is the patch working around the issue. Unfortunately I was not
able to root-cause it (I really suspect something on SH)
virtnet_info *vi,
/* Copy all frame if it fits skb->head, otherwise
* we let virtio_net_hdr_to_skb() and GRO pull headers as needed.
+ *
+ * Apparently, pulling only the Ethernet Header triggers a bug
on qemu-system-sh4.
+ * Since GRO aggregation really cares of IPv4/IPv6, pull 20 bytes
+ * more to work around this bug : These 20 bytes can not belong
+ * to UDP/TCP payload.
+ * As a bonus, this makes GRO slightly faster for IPv4 (one less copy).
*/
Question: do we still want to do this for performance reasons?
We also have the hdr_len coming from the device which is
just skb_headlen on the host.
Well, putting 20 bytes in skb->head will disable frag0 optimization.
The change would only benefit to sh architecture :)
About hdr_len, I suppose we could try it, with appropriate safety checks.
I have added traces, hdr_len seems to be 0 with the qemu-system-sh4 I am using.
Have I understood you correctly ?
@@ -399,9 +399,10 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,hdr_padded_len=sizeof(structpadded_vnet_hdr);/* hdr_valid means no XDP, so we can copy the vnet header */-if(hdr_valid)+if(hdr_valid){memcpy(hdr,p,hdr_len);-+pr_err("hdr->hdr_len=%u\n",hdr->hdr.hdr_len);+}len-=hdr_len;offset+=hdr_padded_len;p+=hdr_padded_len;
Depends on how you connect qemu on the host. It's filled by host tap,
see virtio_net_hdr_from_skb. If you are using slirp that just zero-fills
it.
I do something like this (macvtap):
sudo ip link del macvtap0
sudo ip link add link enp0s25 name macvtap0 type macvtap mode bridge
#sudo ip link add link wlp3s0 name macvtap0 type macvtap mode bridge
sudo ip link set macvtap0 address 52:54:00:12:34:56 up
index=`cat /sys/class/net/macvtap0/ifindex`
sudo chgrp mst /dev/tap$index
sudo chmod g+rw /dev/tap$index
and then
-netdev tap,fds=6,id=net0,vhost=on -device virtio-net,netdev=net0 \
6<>/dev/tap$index