pair(4) crashes on strict alignment platforms

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

pair(4) crashes on strict alignment platforms

Stefan Sperling-5
Running the first set of example commands from the pair(4) man page
crashes the kernel on at least sparc64 and octeon.

           # ifconfig pair1 rdomain 1 10.1.1.1/24 up
           # ifconfig pair2 rdomain 2 10.1.1.2/24 up
           # ifconfig pair1 patch pair2
           # route -T 1 exec ping 10.1.1.2

A netcat<->telnet connection from 10.1.1.1 to 10.1.1.2 works.

It seems the problem only happens with ping, or short packets in general.
It looks like the crash is happening while processing the icmp echo reply.
This code in ip_input_if() calls m_pullup() which ends up setting m->m_data
to an unaligned address:

        if (m->m_len < sizeof (struct ip) &&
            (m = *mp = m_pullup(m, sizeof (struct ip))) == NULL) {
                ipstat_inc(ips_toosmall);
                goto bad;
        }
        ip = mtod(m, struct ip *);
        if (ip->ip_v != IPVERSION) {  // we crash here because ip is misaligned

Note that pair(4) has dequeued this mbuf from its send queue and doesn't
modify it except for resetting the packet header if it exists.

Trace from sparc64:

panic: trap type 0x34 (mem address not aligned): pc=11336d4 npc=11336d8 pstate=44820006<PRIV,IE>
Stopped at      db_enter+0x8:   nop
    TID    PID    UID     PRFLAGS     PFLAGS  CPU  COMMAND
*291733   6703      0     0x14000      0x200    0  softnet
trap(40016e6b890, 34, 11336d4, 44820006, 400023ff800, 8848) at trap+0x2e0
Lslowtrap_reenter(400023ace00, 0, ffffffffffffffff, 1c176d0, 40000, 1) at Lslowtrap_reenter+0xf8
ip_input_if(40016e6bb48, 40016e6bb54, 4, 0, 400023ff800, 8848) at ip_input_if+0x120
ipv4_input(400023ff800, 18184e8, ffffffffffffffff, 1c176d0, 40000, 1) at ipv4_input+0x3c
ether_input(400023ff800, 400023ace00, 0, 16545e8, aaaa, 8848) at ether_input+0xc8
if_input_process(400021607c0, 40016e6bde0, 131cb20, 1c176d0, 40000, 0) at if_input_process+0x11c
taskq_thread(4000216c080, 40002142fc0, 1758938, 16545e8, 0, 3b9ac800) at taskq_thread+0x6c
proc_trampoline(0, 0, 0, 0, 0, 0) at proc_trampoline+0x14
https://www.openbsd.org/ddb.html describes the minimum info required in bug reports.
Insufficient info makes it difficult to find and fix bugs.

Reply | Threaded
Open this post in threaded view
|

Re: pair(4) crashes on strict alignment platforms

David Gwynne-5
On Thu, Mar 08, 2018 at 07:44:13PM +0100, Stefan Sperling wrote:

> Running the first set of example commands from the pair(4) man page
> crashes the kernel on at least sparc64 and octeon.
>
>   # ifconfig pair1 rdomain 1 10.1.1.1/24 up
>   # ifconfig pair2 rdomain 2 10.1.1.2/24 up
>   # ifconfig pair1 patch pair2
>   # route -T 1 exec ping 10.1.1.2
>
> A netcat<->telnet connection from 10.1.1.1 to 10.1.1.2 works.
>
> It seems the problem only happens with ping, or short packets in general.
> It looks like the crash is happening while processing the icmp echo reply.
> This code in ip_input_if() calls m_pullup() which ends up setting m->m_data
> to an unaligned address:
>
> if (m->m_len < sizeof (struct ip) &&
>    (m = *mp = m_pullup(m, sizeof (struct ip))) == NULL) {
> ipstat_inc(ips_toosmall);
> goto bad;
> }
> ip = mtod(m, struct ip *);
> if (ip->ip_v != IPVERSION) {  // we crash here because ip is misaligned
>
> Note that pair(4) has dequeued this mbuf from its send queue and doesn't
> modify it except for resetting the packet header if it exists.
>
> Trace from sparc64:
>
> panic: trap type 0x34 (mem address not aligned): pc=11336d4 npc=11336d8 pstate=44820006<PRIV,IE>
> Stopped at      db_enter+0x8:   nop
>     TID    PID    UID     PRFLAGS     PFLAGS  CPU  COMMAND
> *291733   6703      0     0x14000      0x200    0  softnet
> trap(40016e6b890, 34, 11336d4, 44820006, 400023ff800, 8848) at trap+0x2e0
> Lslowtrap_reenter(400023ace00, 0, ffffffffffffffff, 1c176d0, 40000, 1) at Lslowtrap_reenter+0xf8
> ip_input_if(40016e6bb48, 40016e6bb54, 4, 0, 400023ff800, 8848) at ip_input_if+0x120
> ipv4_input(400023ff800, 18184e8, ffffffffffffffff, 1c176d0, 40000, 1) at ipv4_input+0x3c
> ether_input(400023ff800, 400023ace00, 0, 16545e8, aaaa, 8848) at ether_input+0xc8
> if_input_process(400021607c0, 40016e6bde0, 131cb20, 1c176d0, 40000, 0) at if_input_process+0x11c
> taskq_thread(4000216c080, 40002142fc0, 1758938, 16545e8, 0, 3b9ac800) at taskq_thread+0x6c
> proc_trampoline(0, 0, 0, 0, 0, 0) at proc_trampoline+0x14
> https://www.openbsd.org/ddb.html describes the minimum info required in bug reports.
> Insufficient info makes it difficult to find and fix bugs.
>

the diff below fixes the panic.

the problem is m_adj doesn't keep m_data updated when removing all
the data from one mbuf in a chain.

in more detail, when we read the ping packet from userland into
the kernel, it's put at the front of an mbuf, which is properly
aligned for an ip packet. pair is an ethernet interface, so to send
it an ethernet header is prepended. because the ip packet is at the
start of an mbuf we allocate a new one for the ethernet header.
that ends up being 6 bytes at the end of a new mbuf, with the end
of the ethernet header properly aligned for an ip packet.

pair then sends the packet back into the network stack, which m_adjs
the ethernet header off the front of the packet. m_adj never deletes
mbufs, it just sets their lengths to 0 if there's no more data left
in an mbuf. because the ethernet header is all thats in that first
mbuf it sets m_len to 0, but doesn't touch m_data.

the ip stack then tries to access the ip header in the mbuf chain.
because the first mbuf has 0 bytes in it it uses m_pulldown to get
to the ip header. m_pulldown goes to a lot of lengths to maintain
the alignment of the payload in an mbuf. because m_adj left m_data
where an ethernet header is, ie, 6 bytes with the end aligned for
a payload), m_pulldown puts the ip header where the ethernet header
was, which is 2 byte aligned.

netcat works because non-raw ip sockets have their headers allocated
with space at the start for link headers.

this diff makes m_adj update m_data in all paths.

another option could be to have m_pullup skip over mbufs with
m_len == 0 before finding the target alignment. or do both.

Index: uipc_mbuf.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
retrieving revision 1.253
diff -u -p -r1.253 uipc_mbuf.c
--- uipc_mbuf.c 16 Jan 2018 19:44:34 -0000 1.253
+++ uipc_mbuf.c 9 Mar 2018 01:34:05 -0000
@@ -812,11 +812,12 @@ m_adj(struct mbuf *mp, int req_len)
  while (m != NULL && len > 0) {
  if (m->m_len <= len) {
  len -= m->m_len;
+ m->m_data += m->m_len; /* move alignment */
  m->m_len = 0;
  m = m->m_next;
  } else {
- m->m_len -= len;
  m->m_data += len;
+ m->m_len -= len;
  len = 0;
  }
  }