On Sat, Oct 19, 2013 at 04:43:07PM +0200, Alexander Bluhm wrote:
> On Fri, Oct 18, 2013 at 03:27:09PM -0400, Lawrence Teo wrote:
> > Back in August I sent a diff to fix ICMP checksum calculation in
> > in_proto_cksum_out() and in_delayed_cksum() in cases where the ICMP
> > checksum field is not in the first mbuf of an mbuf chain (original post
> > at http://marc.info/?l=openbsd-tech&m=137571298511653&w=2 ).
> >
> > bluhm@ replied on tech@ with the following feedback:
> >
> > On Fri, Aug 09, 2013 at 02:21:29AM +0200, Alexander Bluhm wrote:
> > > On Mon, Aug 05, 2013 at 10:28:57AM -0400, Lawrence Teo wrote:
> > > > Index: ip_output.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/netinet/ip_output.c,v
> > > > retrieving revision 1.244
> > > > diff -U5 -p -r1.244 ip_output.c
> > > > --- ip_output.c 31 Jul 2013 15:41:52 -0000 1.244
> > > > +++ ip_output.c 5 Aug 2013 02:44:20 -0000
> > > > @@ -2058,25 +2058,35 @@ ip_mloopback(struct ifnet *ifp, struct m
> > > > */
> > > > void
> > > > in_delayed_cksum(struct mbuf *m)
> > > > {
> > > > struct ip *ip;
> > > > - u_int16_t csum, offset;
> > > > + u_int16_t csum = 0, offset;
> > > >
> > > > ip = mtod(m, struct ip *);
> > > > offset = ip->ip_hl << 2;
> > > > +
> > > > + if (ip->ip_p == IPPROTO_ICMP)
> > > > + if (m_copyback(m, offset + offsetof(struct icmp,
> > > > icmp_cksum),
> > > > + sizeof(csum), &csum, M_NOWAIT))
> > > > + return;
> > >
> > > The code at the end of this function tries to avoid the m_copyback()
> > > in the common case unless (offset + sizeof(u_int16_t)) > m->m_len).
> > > Do we want this optimization here?
> > >
> > > bluhm
> >
> > Here's my revised diff that preserves that optimization so that existing
> > behavior won't change in the common case where the protocol checksum
> > field is in the first mbuf.
> >
> > The new diff also implements similar logic in in6_proto_cksum_out().
> >
> > Comments/feedback appreciated. :)
>
> Is there any good reason why you pass u_int8_t proto, u_int16_t
> p_off to the in_delayed_cksum() now? What was wrong with the switch
> in your previous diff? I think splitting the offset calculation
> between in_delayed_cksum() and in_proto_cksum_out() doesn't make
> it better.
>
> The optimisation is fine now.
>
> bluhm
Thank you for the feedback.
Passing u_int8_t proto to in_delayed_cksum() was inspired by
in6_proto_cksum_out(), which passes the protocol (nxt) to
in6_delayed_cksum(). This allows in6_delayed_cksum() to check if the
actual protocol matches the expected protocol at the beginning of the
function before even doing the delayed checksum calculation. It seems
like a good optimization so I borrowed the logic from there.
Passing in u_int16_t p_off takes advantage of the fact that an mbuf with
the M_TCP_CSUM_OUT flag represents a TCP packet (and M_UDP_CSUM_OUT
represents UDP, etc.), so we can pass in the protocol-specific checksum
offset to in_delayed_cksum() without having to figure out the offset
from the protocol again in in_delayed_cksum().
Having said all this, now that Henning's commit is in, I think there's
an opportunity to do things differently by setting the ICMP checksum
field to 0 somewhere in Henning's new block of code at the top of
in_proto_cksum_out(). I'll try to come up with a diff for discussion.
Thanks,
Lawrence
> > Index: netinet/in.h
> > ===================================================================
> > RCS file: /cvs/src/sys/netinet/in.h,v
> > retrieving revision 1.97
> > diff -u -p -u -p -r1.97 in.h
> > --- netinet/in.h 9 Oct 2013 09:33:43 -0000 1.97
> > +++ netinet/in.h 16 Oct 2013 15:14:39 -0000
> > @@ -835,7 +835,7 @@ int in_broadcast(struct in_addr, stru
> > int in_canforward(struct in_addr);
> > int in_cksum(struct mbuf *, int);
> > int in4_cksum(struct mbuf *, u_int8_t, int, int);
> > -void in_delayed_cksum(struct mbuf *);
> > +void in_delayed_cksum(struct mbuf *, u_int8_t, u_int16_t);
> > int in_localaddr(struct in_addr, u_int);
> > void in_socktrim(struct sockaddr_in *);
> > char *inet_ntoa(struct in_addr);
> > Index: netinet/ip_output.c
> > ===================================================================
> > RCS file: /cvs/src/sys/netinet/ip_output.c,v
> > retrieving revision 1.247
> > diff -u -p -u -p -r1.247 ip_output.c
> > --- netinet/ip_output.c 18 Oct 2013 09:04:03 -0000 1.247
> > +++ netinet/ip_output.c 18 Oct 2013 15:14:04 -0000
> > @@ -2050,34 +2050,35 @@ ip_mloopback(struct ifnet *ifp, struct m
> > * Process a delayed payload checksum calculation.
> > */
> > void
> > -in_delayed_cksum(struct mbuf *m)
> > +in_delayed_cksum(struct mbuf *m, u_int8_t proto, u_int16_t p_off)
> > {
> > struct ip *ip;
> > - u_int16_t csum, offset;
> > + u_int16_t csum = 0, hlen, *p = NULL;
> >
> > ip = mtod(m, struct ip *);
> > - offset = ip->ip_hl << 2;
> > - csum = in4_cksum(m, 0, offset, m->m_pkthdr.len - offset);
> > - if (csum == 0 && ip->ip_p == IPPROTO_UDP)
> > - csum = 0xffff;
> > -
> > - switch (ip->ip_p) {
> > - case IPPROTO_TCP:
> > - offset += offsetof(struct tcphdr, th_sum);
> > - break;
> > -
> > - case IPPROTO_UDP:
> > - offset += offsetof(struct udphdr, uh_sum);
> > - break;
> > -
> > - default:
> > + if (ip->ip_p != proto)
> > return;
> > +
> > + hlen = ip->ip_hl << 2;
> > + p_off += hlen;
> > + if ((p_off + sizeof(u_int16_t)) <= m->m_len)
> > + p = (u_int16_t *)(mtod(m, caddr_t) + p_off);
> > +
> > + if (proto == IPPROTO_ICMP) {
> > + if (p)
> > + *p = 0;
> > + else if (m_copyback(m, p_off, sizeof(csum), &csum, M_NOWAIT))
> > + return;
> > }
> >
> > - if ((offset + sizeof(u_int16_t)) > m->m_len)
> > - m_copyback(m, offset, sizeof(csum), &csum, M_NOWAIT);
> > + csum = in4_cksum(m, 0, hlen, m->m_pkthdr.len - hlen);
> > + if (csum == 0 && proto == IPPROTO_UDP)
> > + csum = 0xffff;
> > +
> > + if (p)
> > + *p = csum;
> > else
> > - *(u_int16_t *)(mtod(m, caddr_t) + offset) = csum;
> > + m_copyback(m, p_off, sizeof(csum), &csum, M_NOWAIT);
> > }
> >
> > void
> > @@ -2086,25 +2087,20 @@ in_proto_cksum_out(struct mbuf *m, struc
> > if (m->m_pkthdr.csum_flags & M_TCP_CSUM_OUT) {
> > if (!ifp || !(ifp->if_capabilities & IFCAP_CSUM_TCPv4) ||
> > ifp->if_bridgeport != NULL) {
> > - in_delayed_cksum(m);
> > + in_delayed_cksum(m, IPPROTO_TCP,
> > + offsetof(struct tcphdr, th_sum));
> > m->m_pkthdr.csum_flags &= ~M_TCP_CSUM_OUT; /* Clear */
> > }
> > } else if (m->m_pkthdr.csum_flags & M_UDP_CSUM_OUT) {
> > if (!ifp || !(ifp->if_capabilities & IFCAP_CSUM_UDPv4) ||
> > ifp->if_bridgeport != NULL) {
> > - in_delayed_cksum(m);
> > + in_delayed_cksum(m, IPPROTO_UDP,
> > + offsetof(struct udphdr, uh_sum));
> > m->m_pkthdr.csum_flags &= ~M_UDP_CSUM_OUT; /* Clear */
> > }
> > } else if (m->m_pkthdr.csum_flags & M_ICMP_CSUM_OUT) {
> > - struct ip *ip = mtod(m, struct ip *);
> > - int hlen;
> > - struct icmp *icp;
> > -
> > - hlen = ip->ip_hl << 2;
> > - icp = (struct icmp *)(mtod(m, caddr_t) + hlen);
> > - icp->icmp_cksum = 0;
> > - icp->icmp_cksum = in4_cksum(m, 0, hlen,
> > - ntohs(ip->ip_len) - hlen);
> > + in_delayed_cksum(m, IPPROTO_ICMP,
> > + offsetof(struct icmp, icmp_cksum));
> > m->m_pkthdr.csum_flags &= ~M_ICMP_CSUM_OUT; /* Clear */
> > }
> > }
> > Index: netinet6/ip6_output.c
> > ===================================================================
> > RCS file: /cvs/src/sys/netinet6/ip6_output.c,v
> > retrieving revision 1.144
> > diff -u -p -u -p -r1.144 ip6_output.c
> > --- netinet6/ip6_output.c 17 Oct 2013 16:27:46 -0000 1.144
> > +++ netinet6/ip6_output.c 18 Oct 2013 15:06:23 -0000
> > @@ -126,7 +126,7 @@ int ip6_splithdr(struct mbuf *, struct i
> > int ip6_getpmtu(struct route_in6 *, struct route_in6 *,
> > struct ifnet *, struct in6_addr *, u_long *, int *);
> > int copypktopts(struct ip6_pktopts *, struct ip6_pktopts *, int);
> > -void in6_delayed_cksum(struct mbuf *, u_int8_t);
> > +void in6_delayed_cksum(struct mbuf *, u_int8_t, u_int16_t);
> > void in6_proto_cksum_out(struct mbuf *, struct ifnet *);
> >
> > /* Context for non-repeating IDs */
> > @@ -3173,44 +3173,35 @@ ip6_randomid_init(void)
> > * Process a delayed payload checksum calculation.
> > */
> > void
> > -in6_delayed_cksum(struct mbuf *m, u_int8_t nxt)
> > +in6_delayed_cksum(struct mbuf *m, u_int8_t nxt, u_int16_t p_off)
> > {
> > int nxtp, offset;
> > - u_int16_t csum;
> > + u_int16_t csum = 0, *p = NULL;
> >
> > offset = ip6_lasthdr(m, 0, IPPROTO_IPV6, &nxtp);
> > if (offset <= 0 || nxtp != nxt)
> > /* If the desired next protocol isn't found, punt. */
> > return;
> >
> > + p_off += offset;
> > + if ((p_off + sizeof(u_int16_t)) <= m->m_len)
> > + p = (u_int16_t *)(mtod(m, caddr_t) + p_off);
> > +
> > if (nxt == IPPROTO_ICMPV6) {
> > - struct icmp6_hdr *icmp6;
> > - icmp6 = (struct icmp6_hdr *)(mtod(m, caddr_t) + offset);
> > - icmp6->icmp6_cksum = 0;
> > + if (p)
> > + *p = 0;
> > + else if (m_copyback(m, p_off, sizeof(csum), &csum, M_NOWAIT))
> > + return;
> > }
> >
> > csum = (u_int16_t)(in6_cksum(m, nxt, offset, m->m_pkthdr.len - offset));
> > + if (csum == 0 && nxt == IPPROTO_UDP)
> > + csum = 0xffff;
> >
> > - switch (nxt) {
> > - case IPPROTO_TCP:
> > - offset += offsetof(struct tcphdr, th_sum);
> > - break;
> > -
> > - case IPPROTO_UDP:
> > - offset += offsetof(struct udphdr, uh_sum);
> > - if (csum == 0)
> > - csum = 0xffff;
> > - break;
> > -
> > - case IPPROTO_ICMPV6:
> > - offset += offsetof(struct icmp6_hdr, icmp6_cksum);
> > - break;
> > - }
> > -
> > - if ((offset + sizeof(u_int16_t)) > m->m_len)
> > - m_copyback(m, offset, sizeof(csum), &csum, M_NOWAIT);
> > + if (p)
> > + *p = csum;
> > else
> > - *(u_int16_t *)(mtod(m, caddr_t) + offset) = csum;
> > + m_copyback(m, p_off, sizeof(csum), &csum, M_NOWAIT);
> > }
> >
> > void
> > @@ -3219,17 +3210,20 @@ in6_proto_cksum_out(struct mbuf *m, stru
> > if (m->m_pkthdr.csum_flags & M_TCP_CSUM_OUT) {
> > if (!ifp || !(ifp->if_capabilities & IFCAP_CSUM_TCPv6) ||
> > ifp->if_bridgeport != NULL) {
> > - in6_delayed_cksum(m, IPPROTO_TCP);
> > + in6_delayed_cksum(m, IPPROTO_TCP,
> > + offsetof(struct tcphdr, th_sum));
> > m->m_pkthdr.csum_flags &= ~M_TCP_CSUM_OUT; /* Clear */
> > }
> > } else if (m->m_pkthdr.csum_flags & M_UDP_CSUM_OUT) {
> > if (!ifp || !(ifp->if_capabilities & IFCAP_CSUM_UDPv6) ||
> > ifp->if_bridgeport != NULL) {
> > - in6_delayed_cksum(m, IPPROTO_UDP);
> > + in6_delayed_cksum(m, IPPROTO_UDP,
> > + offsetof(struct udphdr, uh_sum));
> > m->m_pkthdr.csum_flags &= ~M_UDP_CSUM_OUT; /* Clear */
> > }
> > } else if (m->m_pkthdr.csum_flags & M_ICMP_CSUM_OUT) {
> > - in6_delayed_cksum(m, IPPROTO_ICMPV6);
> > + in6_delayed_cksum(m, IPPROTO_ICMPV6,
> > + offsetof(struct icmp6_hdr, icmp6_cksum));
> > m->m_pkthdr.csum_flags &= ~M_ICMP_CSUM_OUT; /* Clear */
> > }
> > }
>
