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 -0000 1.210
+++ bpf.c 4 Feb 2022 09:25:40 -0000
@@ -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);
