>> On 20/01/2021, at 1:56 PM, Alexander Bluhm <[email protected]> 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);
>> }
>>
>
