>> On 20/01/2021, at 1:56 PM, Alexander Bluhm <alexander.bl...@gmx.net> wrote: >> >> Hi, >> >> Simplex interfaces reinject broadcast packets back into the IP >> stack. As this is a software features, no hardware checksumming >> occurs. So local broadcast packets are dropped with wrong checksum >> if the underlying hardware supports checksumming. >> >> Do software checksumming in ip_output() if the copy of a broadcast >> packet will be delivered locally. Put the logic into a separate >> in_ifcap_cksum() function. >> >> Found by regress/sys/kern/sosplice/loop which fails on some machines.
Hi, - Might the rule disabling checksum offload for broadcasts on IFF_SIMPLEX interfaces be weakened to disable checksum offload for all broadcast packets instead? This simplifies the logic, and shouldn’t impact performance as 0) IFCAP_CSUM_* /\ !IFF_SIMPLEX devices appear to be rare[0] and in any case 1) broadcasts are not high-throughput. It should be safe, as broadcast checksums across bridges are already done in software. (I wonder if IFF_SIMPLEX is a relic of another age and deserves to be removed at some point; the rare !IFF_SIMPLEX device drivers could presumably filter out their own received broadcasts. The rest of the stack would then have a simpler invariant to work with.) - what motivates the new '!m->m_pkthdr.pf.routed’ term? best, Richard. [0] A coarse test for IFF_BROADCAST /\ !IFF_SIMPLEX devices /usr/src/sys $ grep -r IFF_BROADCAST | grep -v IFF_SIMPLEX | grep ' = ‘ arch/octeon/dev/if_ogx.c: ifp->if_flags = IFF_BROADCAST | IFF_MULTICAST; arch/sgi/hpc/if_sq.c: ifp->if_flags = IFF_BROADCAST | IFF_MULTICAST; net/if_bpe.c: ifp->if_flags = IFF_BROADCAST | IFF_MULTICAST; net/if_wg.c: ifp->if_flags = IFF_BROADCAST | IFF_MULTICAST | IFF_NOARP; net/if_vlan.c: ifp->if_flags = IFF_BROADCAST | IFF_MULTICAST; >> >> ok? >> >> bluhm >> >> Index: netinet/ip_output.c >> =================================================================== >> RCS file: /mount/openbsd/cvs/src/sys/netinet/ip_output.c,v >> retrieving revision 1.361 >> diff -u -p -r1.361 ip_output.c >> --- netinet/ip_output.c 16 Jan 2021 07:58:12 -0000 1.361 >> +++ netinet/ip_output.c 20 Jan 2021 00:27:12 -0000 >> @@ -79,6 +79,7 @@ void ip_mloopback(struct ifnet *, struct >> static __inline u_int16_t __attribute__((__unused__)) >> in_cksum_phdr(u_int32_t, u_int32_t, u_int32_t); >> void in_delayed_cksum(struct mbuf *); >> +int in_ifcap_cksum(struct mbuf *, struct ifnet *, int); >> >> #ifdef IPSEC >> struct tdb * >> @@ -458,8 +459,7 @@ sendit: >> */ >> if (ntohs(ip->ip_len) <= mtu) { >> ip->ip_sum = 0; >> - if ((ifp->if_capabilities & IFCAP_CSUM_IPv4) && >> - (ifp->if_bridgeidx == 0)) >> + if (in_ifcap_cksum(m, ifp, IFCAP_CSUM_IPv4)) >> m->m_pkthdr.csum_flags |= M_IPV4_CSUM_OUT; >> else { >> ipstat_inc(ips_outswcsum); >> @@ -716,9 +716,7 @@ ip_fragment(struct mbuf *m, struct ifnet >> m->m_pkthdr.ph_ifidx = 0; >> mhip->ip_off = htons((u_int16_t)mhip->ip_off); >> mhip->ip_sum = 0; >> - if ((ifp != NULL) && >> - (ifp->if_capabilities & IFCAP_CSUM_IPv4) && >> - (ifp->if_bridgeidx == 0)) >> + if (in_ifcap_cksum(m, ifp, IFCAP_CSUM_IPv4)) >> m->m_pkthdr.csum_flags |= M_IPV4_CSUM_OUT; >> else { >> ipstat_inc(ips_outswcsum); >> @@ -737,9 +735,7 @@ ip_fragment(struct mbuf *m, struct ifnet >> ip->ip_len = htons((u_int16_t)m->m_pkthdr.len); >> ip->ip_off |= htons(IP_MF); >> ip->ip_sum = 0; >> - if ((ifp != NULL) && >> - (ifp->if_capabilities & IFCAP_CSUM_IPv4) && >> - (ifp->if_bridgeidx == 0)) >> + if (in_ifcap_cksum(m, ifp, IFCAP_CSUM_IPv4)) >> m->m_pkthdr.csum_flags |= M_IPV4_CSUM_OUT; >> else { >> ipstat_inc(ips_outswcsum); >> @@ -1849,15 +1845,15 @@ 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) || >> - ip->ip_hl != 5 || ifp->if_bridgeidx != 0) { >> + if (!in_ifcap_cksum(m, ifp, IFCAP_CSUM_TCPv4) || >> + ip->ip_hl != 5) { >> tcpstat_inc(tcps_outswcsum); >> in_delayed_cksum(m); >> 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) || >> - ip->ip_hl != 5 || ifp->if_bridgeidx != 0) { >> + if (!in_ifcap_cksum(m, ifp, IFCAP_CSUM_UDPv4) || >> + ip->ip_hl != 5) { >> udpstat_inc(udps_outswcsum); >> in_delayed_cksum(m); >> m->m_pkthdr.csum_flags &= ~M_UDP_CSUM_OUT; /* Clear */ >> @@ -1866,4 +1862,19 @@ in_proto_cksum_out(struct mbuf *m, struc >> in_delayed_cksum(m); >> m->m_pkthdr.csum_flags &= ~M_ICMP_CSUM_OUT; /* Clear */ >> } >> +} >> + >> +int >> +in_ifcap_cksum(struct mbuf *m, struct ifnet *ifp, int ifcap) >> +{ >> + if ((ifp == NULL) || >> + !ISSET(ifp->if_capabilities, ifcap) || >> + (ifp->if_bridgeidx != 0)) >> + return (0); >> + /* Simplex interface sends packet back without hardware cksum. */ >> + if (ISSET(m->m_flags, M_BCAST) && >> + ISSET(ifp->if_flags, IFF_SIMPLEX) && >> + !m->m_pkthdr.pf.routed) >> + return (0); >> + return (1); >> } >> >