Index | Thread | Search

From:
David Gwynne <loki@animata.net>
Subject:
Re: dhcpleased(8): fix pseudo csum from vio(4) offloading
To:
Alexander Bluhm <alexander.bluhm@gmx.net>
Cc:
Jan Klemkow <j.klemkow@wemelug.de>, Openbsd Tech <tech@openbsd.org>
Date:
Fri, 26 Jan 2024 06:44:26 +1000

Download raw body.

Thread
Has this been tested on any strict alignment architectures? bpf is supposed
to pad the header, but it's worth checking.

On Fri, 26 Jan 2024, 00:08 Alexander Bluhm, <alexander.bluhm@gmx.net> wrote:

> On Thu, Jan 25, 2024 at 02:49:30PM +0100, Jan Klemkow wrote:
> > On Sun, Jan 07, 2024 at 09:46:22PM +0100, Alexander Bluhm wrote:
> > > On Sun, Jan 07, 2024 at 01:38:23PM +0100, Jan Klemkow wrote:
> > > > we get packets with pseudo checksums from vio(4), since we use
> checksum
> > > > offloading.  Thus, programs which are dealing with raw packets like
> > > > dhcpleased(8) also need to check for this kind of checksums or return
> > > > errors.
> > >
> > > I think kernel should fix this.  If we have devices like vio(4)
> > > that report checksum OK, but we know that the ckecksum field is
> > > wrong, and we pass the packet header to user land, kernel should
> > > calculate correct checksum.  At least divert_packet() does a trick
> > > like this.  For raw sockets I would prefer something like this.
> > >
> > > Another possibility would be to pass a flag in bpf header that
> > > checksum is OK.  Then dhcpleased could skip the checksum check.
> > > That would also help tcpdump complaining about bad checksums in bpf
> > > packets.
> > >
> > > As dhcpleased uses bpf, I think the second option is the way to go.
> >
> > Sounds like a better approach.
> >
> > ok?
>
> OK bluhm@
>
> > Index: sys/net/bpf.c
> > ===================================================================
> > RCS file: /cvs/src/sys/net/bpf.c,v
> > diff -u -p -r1.221 bpf.c
> > --- sys/net/bpf.c     9 Mar 2023 05:56:58 -0000       1.221
> > +++ sys/net/bpf.c     25 Jan 2024 13:34:17 -0000
> > @@ -1397,6 +1397,8 @@ _bpf_mtap(caddr_t arg, const struct mbuf
> >                                       if (ISSET(mp->m_pkthdr.csum_flags,
> >                                           M_FLOWID))
> >                                               SET(tbh.bh_flags,
> BPF_F_FLOWID);
> > +                                     tbh.bh_csumflags =
> > +                                         mp->m_pkthdr.csum_flags;
> >
> >                                       m_microtime(mp, &tv);
> >                               } else
> > Index: sys/net/bpf.h
> > ===================================================================
> > RCS file: /cvs/src/sys/net/bpf.h,v
> > diff -u -p -r1.71 bpf.h
> > --- sys/net/bpf.h     9 Mar 2023 05:56:58 -0000       1.71
> > +++ sys/net/bpf.h     25 Jan 2024 13:34:17 -0000
> > @@ -161,6 +161,7 @@ struct bpf_hdr {
> >  #define BPF_F_DIR_IN         (BPF_DIRECTION_IN << BPF_F_DIR_SHIFT)
> >  #define BPF_F_DIR_OUT                (BPF_DIRECTION_OUT <<
> BPF_F_DIR_SHIFT)
> >       u_int8_t        bh_drops;
> > +     u_int16_t       bh_csumflags;   /* checksum flags */
> >  };
> >
> >  #ifdef _KERNEL
> > Index: sbin/dhcpleased/dhcpleased.h
> > ===================================================================
> > RCS file: /cvs/src/sbin/dhcpleased/dhcpleased.h,v
> > diff -u -p -r1.15 dhcpleased.h
> > --- sbin/dhcpleased/dhcpleased.h      25 Nov 2023 12:00:39 -0000
> 1.15
> > +++ sbin/dhcpleased/dhcpleased.h      25 Jan 2024 13:34:18 -0000
> > @@ -287,6 +287,7 @@ struct imsg_propose_rdns {
> >  struct imsg_dhcp {
> >       uint32_t                if_index;
> >       ssize_t                 len;
> > +     uint16_t                csumflags;
> >       uint8_t                 ether_align[2];
> >       uint8_t                 packet[1500];
> >  };
> > Index: sbin/dhcpleased/engine.c
> > ===================================================================
> > RCS file: /cvs/src/sbin/dhcpleased/engine.c,v
> > diff -u -p -r1.41 engine.c
> > --- sbin/dhcpleased/engine.c  14 Dec 2023 09:58:37 -0000      1.41
> > +++ sbin/dhcpleased/engine.c  25 Jan 2024 13:34:18 -0000
> > @@ -24,6 +24,7 @@
> >  #include <sys/socket.h>
> >  #include <sys/syslog.h>
> >  #include <sys/uio.h>
> > +#include <sys/mbuf.h>
> >
> >  #include <net/if.h>
> >  #include <net/route.h>
> > @@ -788,7 +789,8 @@ parse_dhcp(struct dhcpleased_iface *ifac
> >       if (rem < (size_t)ip->ip_hl << 2)
> >               goto too_short;
> >
> > -     if (wrapsum(checksum((uint8_t *)ip, ip->ip_hl << 2, 0)) != 0) {
> > +     if ((dhcp->csumflags & M_IPV4_CSUM_IN_OK) == 0 &&
> > +         wrapsum(checksum((uint8_t *)ip, ip->ip_hl << 2, 0)) != 0) {
> >               log_warnx("%s: bad IP checksum", __func__);
> >               return;
> >       }
> > @@ -834,16 +836,19 @@ parse_dhcp(struct dhcpleased_iface *ifac
> >       p += sizeof(*udp);
> >       rem -= sizeof(*udp);
> >
> > -     usum = udp->uh_sum;
> > -     udp->uh_sum = 0;
> > -
> > -     sum = wrapsum(checksum((uint8_t *)udp, sizeof(*udp), checksum(p,
> rem,
> > -         checksum((uint8_t *)&ip->ip_src, 2 * sizeof(ip->ip_src),
> > -         IPPROTO_UDP + ntohs(udp->uh_ulen)))));
> > -
> > -     if (usum != 0 && usum != sum) {
> > -             log_warnx("%s: bad UDP checksum", __func__);
> > -             return;
> > +     if ((dhcp->csumflags & M_UDP_CSUM_IN_OK) == 0) {
> > +             usum = udp->uh_sum;
> > +             udp->uh_sum = 0;
> > +
> > +             sum = wrapsum(checksum((uint8_t *)udp, sizeof(*udp),
> > +                 checksum(p, rem,
> > +                 checksum((uint8_t *)&ip->ip_src, 2 *
> sizeof(ip->ip_src),
> > +                 IPPROTO_UDP + ntohs(udp->uh_ulen)))));
> > +
> > +             if (usum != 0 && usum != sum) {
> > +                     log_warnx("%s: bad UDP checksum", __func__);
> > +                     return;
> > +             }
> >       }
> >
> >       if (log_getverbose() > 1) {
> > Index: sbin/dhcpleased/frontend.c
> > ===================================================================
> > RCS file: /cvs/src/sbin/dhcpleased/frontend.c,v
> > diff -u -p -r1.32 frontend.c
> > --- sbin/dhcpleased/frontend.c        14 Dec 2023 09:58:37 -0000
> 1.32
> > +++ sbin/dhcpleased/frontend.c        25 Jan 2024 13:34:18 -0000
> > @@ -890,6 +890,7 @@ bpf_receive(int fd, short events, void *
> >               }
> >               memcpy(&imsg_dhcp.packet, p + hdr->bh_hdrlen,
> hdr->bh_caplen);
> >               imsg_dhcp.len = hdr->bh_caplen;
> > +             imsg_dhcp.csumflags = hdr->bh_csumflags;
> >               frontend_imsg_compose_engine(IMSG_DHCP, 0, 0, &imsg_dhcp,
> >                   sizeof(imsg_dhcp));
> >   cont:
>
>