Re: m_pullup alingment crash armv7 sparc64
On Fri, Feb 04, 2022 at 07:27:38PM +1000, David Gwynne wrote: > On Thu, Feb 03, 2022 at 11:39:50PM +0100, Alexander Bluhm wrote: > > On Thu, Feb 03, 2022 at 10:22:46PM +1000, David Gwynne wrote: > > > bpf should know better than this. it has all the information it needs to > > > align the payload properly, it just doesnt make enough of an effort. can > > > you try the diff below? > > > > Diff seems to work. regress/sbin/slaacd passes. > > I have started a full regress run on armv7 and sparc64. Regress passed. > tun(4) uses EMSGSIZE for this, but errno(2) says EMSGSIZE means > "Message too long". EINVAL sounds good. > > want me to do it now? As you move the code around, you can easily adopt the error code now. OK bluhm@
Re: m_pullup alingment crash armv7 sparc64
On Thu, Feb 03, 2022 at 11:39:50PM +0100, Alexander Bluhm wrote:
> On Thu, Feb 03, 2022 at 10:22:46PM +1000, David Gwynne wrote:
> > bpf should know better than this. it has all the information it needs to
> > align the payload properly, it just doesnt make enough of an effort. can
> > you try the diff below?
>
> Diff seems to work. regress/sbin/slaacd passes.
> I have started a full regress run on armv7 and sparc64.
cool.
>
> > + if (len < hlen)
> > + return (EPERM);
>
> This is not a permission problem. Maybe EINVAL.
this is preveserving the existing semantics of this chunk:
@@ -232,10 +247,6 @@ bpf_movein(struct uio *uio, struct bpf_d
goto bad;
}
- if (m->m_len < hlen) {
- error = EPERM;
- goto bad;
- }
/*
* Make room for link header, and copy it to sockaddr
*/
i was going to follow this diff up with another one to tweak it.
tun(4) uses EMSGSIZE for this, but errno(2) says EMSGSIZE means
"Message too long". EINVAL sounds good.
want me to do it now?
>
> > + /*
> > + * Get the length of the payload so we can align it properly.
> > +*/
>
> Whitespace misaligned.
yep.
> > + if (mlen > MAXMCLBYTES)
> > + return (EIO);
>
> Should we use EMSGSIZE like in bpfwrite() if the data does not fit.
yes. want me to do that now too?
>
> > + mlen = max(max_linkhdr, hlen) + roundup(alen, sizeof(long));
> ...
> > + m_align(m, alen); /* Align the payload. */
>
> sizeof(long) seems the correct alignment for mbuf. In rev 1.237
> of m_pullup() you introduced ALIGNBYTES. This is architecture
> dependent and the only place in the network stack where it is used.
> Should it also be sizeof(long) there?
i probably did that when i was adding ALIGNED_POINTER checks to ethernet
tunnel drivers.
my initial thought is it doesnt save m_pullup any work, so it's
probably better to use sizeof(long) so there's a better chance that
accesses to words in a pulled up payload are aligned. even if an
arch doesnt fault on unaligned access it still goes faster with
aligned accesses. if m_pullup is going to move data around it may as
well move it to the best place possible.
i'll spit a diff out for that next.
Index: bpf.c
===
RCS file: /cvs/src/sys/net/bpf.c,v
retrieving revision 1.210
diff -u -p -r1.210 bpf.c
--- bpf.c 16 Jan 2022 06:27:14 - 1.210
+++ bpf.c 4 Feb 2022 09:25:40 -
@@ -144,7 +144,7 @@ bpf_movein(struct uio *uio, struct bpf_d
struct mbuf *m;
struct m_tag *mtag;
int error;
- u_int hlen;
+ u_int hlen, alen, mlen;
u_int len;
u_int linktype;
u_int slen;
@@ -198,23 +198,38 @@ bpf_movein(struct uio *uio, struct bpf_d
return (EIO);
}
- if (uio->uio_resid > MAXMCLBYTES)
- return (EIO);
len = uio->uio_resid;
+ if (len < hlen)
+ return (EPERM);
- MGETHDR(m, M_WAIT, MT_DATA);
- m->m_pkthdr.ph_ifidx = 0;
- m->m_pkthdr.len = len - hlen;
+ /*
+* Get the length of the payload so we can align it properly.
+*/
+ alen = len - hlen;
+
+ /*
+* Allocate enough space for headers and the aligned payload.
+*/
+ mlen = max(max_linkhdr, hlen) + roundup(alen, sizeof(long));
+
+ if (mlen > MAXMCLBYTES)
+ return (EIO);
- if (len > MHLEN) {
- MCLGETL(m, M_WAIT, len);
+ MGETHDR(m, M_WAIT, MT_DATA);
+ if (mlen > MHLEN) {
+ MCLGETL(m, M_WAIT, mlen);
if ((m->m_flags & M_EXT) == 0) {
error = ENOBUFS;
goto bad;
}
}
+
+ m_align(m, alen); /* Align the payload. */
+ m->m_data -= hlen;
+
+ m->m_pkthdr.ph_ifidx = 0;
+ m->m_pkthdr.len = len;
m->m_len = len;
- *mp = m;
error = uiomove(mtod(m, caddr_t), len, uio);
if (error)
@@ -232,10 +247,6 @@ bpf_movein(struct uio *uio, struct bpf_d
goto bad;
}
- if (m->m_len < hlen) {
- error = EPERM;
- goto bad;
- }
/*
* Make room for link header, and copy it to sockaddr
*/
@@ -249,8 +260,10 @@ bpf_movein(struct uio *uio, struct bpf_d
sockp->sa_family = ntohl(af);
} else
memcpy(sockp->sa_data, m->m_data, hlen);
+
+ m->m_pkthdr.len -= hlen;
m->m_len -= hlen;
- m->m_data += hlen; /* XXX */
+ m->m_data += hlen;
}
/*
@@ -260,6 +273,7 @@ bpf_movein(struct uio *uio, struct bpf_d
*(u_int *)(mtag + 1) = linktype;
m_tag_prepend(m, mtag);
+ *mp = m;
return (0);
bad:
m_freem(m);
Re: m_pullup alingment crash armv7 sparc64
On Thu, Feb 03, 2022 at 10:22:46PM +1000, David Gwynne wrote: > bpf should know better than this. it has all the information it needs to > align the payload properly, it just doesnt make enough of an effort. can > you try the diff below? Diff seems to work. regress/sbin/slaacd passes. I have started a full regress run on armv7 and sparc64. > + if (len < hlen) > + return (EPERM); This is not a permission problem. Maybe EINVAL. > + /* > + * Get the length of the payload so we can align it properly. > + */ Whitespace misaligned. > + if (mlen > MAXMCLBYTES) > + return (EIO); Should we use EMSGSIZE like in bpfwrite() if the data does not fit. > + mlen = max(max_linkhdr, hlen) + roundup(alen, sizeof(long)); ... > + m_align(m, alen); /* Align the payload. */ sizeof(long) seems the correct alignment for mbuf. In rev 1.237 of m_pullup() you introduced ALIGNBYTES. This is architecture dependent and the only place in the network stack where it is used. Should it also be sizeof(long) there? bluhm
Re: m_pullup alingment crash armv7 sparc64
On Wed, Feb 02, 2022 at 10:30:24PM +0100, Alexander Bluhm wrote:
> Hi,
>
> With the new m_pullup() code, I see crashes on armv7 and sparc64.
> regress/sbin/slaacd sends some IPv6 ND6 packets over pair(4) from
> scapy. It crashes after m_pullup() in ipv6_check().
>
> run-parse-ra
> ifconfig pair1 destroy 2>/dev/null || true
> ifconfig pair2 destroy 2>/dev/null || true
> ifconfig pair1 rdomain 1 10.11.12.1/24 up
> ifconfig pair2 rdomain 1 10.11.12.2/24 up
> ifconfig pair1 rdomain 1 patch pair2
> ifconfig pair1 inet6 rdomain 1 eui64
> ifconfig pair2 inet6 rdomain 1 eui64
> ifconfig pair2 inet6 rdomain 1 autoconf
> route -nq -T 1 add -host default 10.11.12.1 -reject
> route -T1 exec python3 -B -u /usr/src/regress/sbin/slaacd/process_ra.py
> pair1 pair2 /dev/slaacd.sock
> Timeout, server ot21 not responding.
>
> panic: trap type 0x34 (mem address not aligned): pc=14a1a7c npc=14a1a80
> pstate=99820006
> Stopped at db_enter+0x8: nop
> TIDPIDUID PRFLAGS PFLAGS CPU COMMAND
> *317234 41858 0 0x14000 0x2000 softnet
> 416946 63125 0 0x14000 0x2001 systq
> trap(40025e91650, 34, 14a1a7c, 99820006, 180, 3b9ac800) at trap+0x328
> Lslowtrap_reenter(40005090d00, 0, fffe, 1832866, 6000, 6) at
> Lslowtrap_reenter+0xf8
> ipv6_check(40005364000, 40005090d00, 0, 2018000, e0018000, 1a) at
> ipv6_check+0x21c
> ip6_input_if(40025e91a68, 40025e91a74, 29, 0, 40005364000, 3b9ac800) at
> ip6_input_if+0x7c
> ipv6_input(40005364000, 198a2f0, 40005090d00, 1, e, 0) at ipv6_input+0x3c
> ether_input(40005364000, 40005090d00, 20, 1780210, 0, 1780210) at
> ether_input+0x274
> if_input_process(40005364000, 40025e91ce0, 20, 1780210, 0, 6) at
> if_input_process+0x44
> ifiq_process(40005364410, 40025e91dc8, 1cd6288, 1cc1840, 1c00, 6) at
> ifiq_process+0x68
> taskq_thread(4000432e080, 40004300810, 1775908, 480, 180, 3b9ac800) at
> taskq_thread+0x7c
> proc_trampoline(0, 0, 0, 0, 0, 0) at proc_trampoline+0x14
>
> bpfwrite() calls bpf_movein() which fills the mbuf with these
> wonderful comments:
>
> case DLT_EN10MB:
> sockp->sa_family = AF_UNSPEC;
> /* XXX Would MAXLINKHDR be better? */
> hlen = ETHER_HDR_LEN;
> ...
> m->m_data += hlen; /* XXX */
>
> Note that ETHER_HDR_LEN is 14, so the mbuf is not 4 byte aligned.
> Next ether_encap() makes room for an aligned ehternet header. This
> is 16 bytes and does not fit. So we prepend a new empty mbuf.
>
> m = m_prepend(m, ETHER_ALIGN + sizeof(eh), M_DONTWAIT);
>
> When we reach ip6_input(), the ethernet header was added and removed.
> So we still have an empty mbuf in front of an mbuf with an unaligned
> IPv6 header.
>
> New m_pullup() code preserves the alingment of the second mbuf data.
> During review we agreed that this might be a good idea. Mbuf
> alingnmet is very fragile, but it works unless you change anything.
> So back to the old behavior, align the data at the beginning of the
> empty mbuf.
>
> ok?
bpf should know better than this. it has all the information it needs to
align the payload properly, it just doesnt make enough of an effort. can
you try the diff below?
Index: bpf.c
===
RCS file: /cvs/src/sys/net/bpf.c,v
retrieving revision 1.210
diff -u -p -r1.210 bpf.c
--- bpf.c 16 Jan 2022 06:27:14 - 1.210
+++ bpf.c 3 Feb 2022 12:21:33 -
@@ -144,7 +144,7 @@ bpf_movein(struct uio *uio, struct bpf_d
struct mbuf *m;
struct m_tag *mtag;
int error;
- u_int hlen;
+ u_int hlen, alen, mlen;
u_int len;
u_int linktype;
u_int slen;
@@ -198,23 +198,38 @@ bpf_movein(struct uio *uio, struct bpf_d
return (EIO);
}
- if (uio->uio_resid > MAXMCLBYTES)
- return (EIO);
len = uio->uio_resid;
+ if (len < hlen)
+ return (EPERM);
- MGETHDR(m, M_WAIT, MT_DATA);
- m->m_pkthdr.ph_ifidx = 0;
- m->m_pkthdr.len = len - hlen;
+ /*
+ * Get the length of the payload so we can align it properly.
+*/
+ alen = len - hlen;
+
+ /*
+* Allocate enough space for headers and the aligned payload.
+*/
+ mlen = max(max_linkhdr, hlen) + roundup(alen, sizeof(long));
+
+ if (mlen > MAXMCLBYTES)
+ return (EIO);
- if (len > MHLEN) {
- MCLGETL(m, M_WAIT, len);
+ MGETHDR(m, M_WAIT, MT_DATA);
+ if (mlen > MHLEN) {
+ MCLGETL(m, M_WAIT, mlen);
if ((m->m_flags & M_EXT) == 0) {
error = ENOBUFS;
goto bad;
}
}
+
+ m_align(m, alen); /* Align the payload. */
+ m->m_data -= hlen;
+
+ m->m_pkthdr.ph_ifidx = 0;
+ m->m_pkthdr.len = len;
m->m_len = len;
- *mp = m;
error = ui
m_pullup alingment crash armv7 sparc64
Hi,
With the new m_pullup() code, I see crashes on armv7 and sparc64.
regress/sbin/slaacd sends some IPv6 ND6 packets over pair(4) from
scapy. It crashes after m_pullup() in ipv6_check().
run-parse-ra
ifconfig pair1 destroy 2>/dev/null || true
ifconfig pair2 destroy 2>/dev/null || true
ifconfig pair1 rdomain 1 10.11.12.1/24 up
ifconfig pair2 rdomain 1 10.11.12.2/24 up
ifconfig pair1 rdomain 1 patch pair2
ifconfig pair1 inet6 rdomain 1 eui64
ifconfig pair2 inet6 rdomain 1 eui64
ifconfig pair2 inet6 rdomain 1 autoconf
route -nq -T 1 add -host default 10.11.12.1 -reject
route -T1 exec python3 -B -u /usr/src/regress/sbin/slaacd/process_ra.py pair1
pair2 /dev/slaacd.sock
Timeout, server ot21 not responding.
panic: trap type 0x34 (mem address not aligned): pc=14a1a7c npc=14a1a80
pstate=99820006
Stopped at db_enter+0x8: nop
TIDPIDUID PRFLAGS PFLAGS CPU COMMAND
*317234 41858 0 0x14000 0x2000 softnet
416946 63125 0 0x14000 0x2001 systq
trap(40025e91650, 34, 14a1a7c, 99820006, 180, 3b9ac800) at trap+0x328
Lslowtrap_reenter(40005090d00, 0, fffe, 1832866, 6000, 6) at
Lslowtrap_reenter+0xf8
ipv6_check(40005364000, 40005090d00, 0, 2018000, e0018000, 1a) at
ipv6_check+0x21c
ip6_input_if(40025e91a68, 40025e91a74, 29, 0, 40005364000, 3b9ac800) at
ip6_input_if+0x7c
ipv6_input(40005364000, 198a2f0, 40005090d00, 1, e, 0) at ipv6_input+0x3c
ether_input(40005364000, 40005090d00, 20, 1780210, 0, 1780210) at
ether_input+0x274
if_input_process(40005364000, 40025e91ce0, 20, 1780210, 0, 6) at
if_input_process+0x44
ifiq_process(40005364410, 40025e91dc8, 1cd6288, 1cc1840, 1c00, 6) at
ifiq_process+0x68
taskq_thread(4000432e080, 40004300810, 1775908, 480, 180, 3b9ac800) at
taskq_thread+0x7c
proc_trampoline(0, 0, 0, 0, 0, 0) at proc_trampoline+0x14
bpfwrite() calls bpf_movein() which fills the mbuf with these
wonderful comments:
case DLT_EN10MB:
sockp->sa_family = AF_UNSPEC;
/* XXX Would MAXLINKHDR be better? */
hlen = ETHER_HDR_LEN;
...
m->m_data += hlen; /* XXX */
Note that ETHER_HDR_LEN is 14, so the mbuf is not 4 byte aligned.
Next ether_encap() makes room for an aligned ehternet header. This
is 16 bytes and does not fit. So we prepend a new empty mbuf.
m = m_prepend(m, ETHER_ALIGN + sizeof(eh), M_DONTWAIT);
When we reach ip6_input(), the ethernet header was added and removed.
So we still have an empty mbuf in front of an mbuf with an unaligned
IPv6 header.
New m_pullup() code preserves the alingment of the second mbuf data.
During review we agreed that this might be a good idea. Mbuf
alingnmet is very fragile, but it works unless you change anything.
So back to the old behavior, align the data at the beginning of the
empty mbuf.
ok?
bluhm
Index: kern/uipc_mbuf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_mbuf.c,v
retrieving revision 1.280
diff -u -p -r1.280 uipc_mbuf.c
--- kern/uipc_mbuf.c18 Jan 2022 12:38:21 - 1.280
+++ kern/uipc_mbuf.c2 Feb 2022 20:02:50 -
@@ -955,7 +955,7 @@ m_pullup(struct mbuf *m0, int len)
if (!M_READONLY(m0) && len <= tail - head) {
/* we can copy everything into the first mbuf */
if (m0->m_len == 0) {
- m0->m_data = head;
+ m0->m_data = M_DATABUF(m0);
} else if (len > tail - mtod(m0, caddr_t)) {
/* need to memmove to make space at the end */
memmove(head, mtod(m0, caddr_t), m0->m_len);
