Hi,
Thanks for the background.
I'm ok with your latest diff as-is. I prefer a slightly different
direction, see below, but not enough to object.
On Mon, 1 Feb 2021, Alexander Bluhm wrote:
> On Mon, Feb 01, 2021 at 08:08:56AM +1300, Richard Procter wrote:
> > - Might the rule disabling checksum offload for broadcasts on IFF_SIMPLEX
> > interfaces be weakened to disable checksum offload for all broadcast
> > packets instead?
>
> I just copied the condition from ether_resolve():
>
> /* If broadcasting on a simplex interface, loopback a copy */
> if (ISSET(m->m_flags, M_BCAST) &&
> ISSET(ifp->if_flags, IFF_SIMPLEX) &&
> !m->m_pkthdr.pf.routed) {
> struct mbuf *mcopy;
>
> /* XXX Should we input an unencrypted IPsec packet? */
> mcopy = m_copym(m, 0, M_COPYALL, M_NOWAIT);
> if (mcopy != NULL)
> if_input_local(ifp, mcopy, af);
> }
>
> The bug is triggered when the packet is processed by if_input_local().
>
> > This simplifies the logic, and shouldn't impact performance as
>
> Yes, performance does not matter. I just wanted to show that the
> code is necessary for simplex interfaces.
I think the log message and comment suffice to capture the rationale.
I see the following downside with the IFF_SIMPLEX term. The network layer
offloads its checksum responsibility to the link layer, as an
optimisation. The more the network layer accomodates the quirks of the
link layer in making its decision to offload, the greater the layer
violation.
Independently of this, there is good reason to compute /all/ broadcast
checksums at the network layer: it is unprofitable to offload them. And
avoiding unnecessary offload minimises what the link layer must handle on
the network's behalf, its scope for going wrong.
So, I would rather eliminate the coupling than document it. And I would
rather do that by eliminating unnecessary offload than disable it for each
unhandled edge case.
> > - what motivates the new '!m->m_pkthdr.pf.routed??? term?
>
> Just copied from ether_resolve(). It looks strange I don't know
> why it is there. I can leave it out in my check if you think this
> is clearer.
It appears to eliminate a local broadcast storm when doing route-to
the broadcast address:
> > if_ethersubr.c: revision 1.70
> > date: 2003/08/18 11:01:41; author: dhartmei; state: Exp; lines: +3
-2;
> > prevent looutput() feedback of broadcast/multicast packets if they are
> > pf routed. prevents a kernel lockup with some (non-sensical) route-to
> > r6ules. report and debugging by mpech@. ok itojun@, henning@, mpech@.
As software offload is always safer, it should be fine to omit the
!m->m_pkthdr.pf.routed term.
So given the above, my preference would be to prune the unnecessary
offload and weaken
+ if (ISSET(m->m_flags, M_BCAST) &&
+ ISSET(ifp->if_flags, IFF_SIMPLEX) &&
+ !m->m_pkthdr.pf.routed)
+ return (0);
to
+ if (ISSET(m->m_flags, M_BCAST))
+ return (0);
I like the addition of in_ifcap_cksum() as it can encapsulate the offload
policy.
cheers,
Richard.
>
> bluhm
>
