Re: [PATCH] Fix checksum bug for multicast/broadcast packets on postrouting hook

["David S. Miller" <davem@redhat.com>]

On Mon, 16 Feb 2004 08:45:47 -0500 (EST)
James Morris <jmorris@redhat.com> wrote:

> On Mon, 16 Feb 2004, Mika Penttilä wrote:
> 
> > James Morris wrote:
> > 
> > >No, it's not needed by all callers, and is just a does-one-thing helper 
> > >function.
> > >
> > >
> > It is needed for cloned skbs, so check for this should go in 
> > skb_checksum_help().
> 
> No, there is no need to bloat a simple helper function out with this 
> (and then need to handle OOM internally etc.), and it is not appropriate 
> for all callers.

I think Mika is in a way correct James.

Nobody may mangle packet data contents without unsharing SKB on output path.

But things are not so simple.  Just mucking with the checksum works because
things which usually clone such packets (TCP retransmit queue) regenerate all
headers and thus checksum fields upon each further use of such clones.

I am tempted just to punish nf_hook_slow()'s invocation of skb_checksum_help()
but that is not the right answer.

Let us look at some of the other users of skb_checksum_help().  The one in dev_queue_xmit()
and the IPSEC encapsulator xfrm drivers are doing so just to plug up a short lived race where
a route lookup points to a non-IPSEC checksumming hardware path, but we end up in the IPSEC
path or to a device which does not support hw checksumming.

One could argue that what we could do in this case is just drop the packet and let the route
relookup on retransmit get things right.  This reminds me that we should audit and if necessary
fix handling of the ethtool operations that turn on/off checksumming support, they would need
to flush all of the cached checksumming capability state if they not do so already.

I'm as confused as when we began this thread :-)

I believe it is possible to replace all skb_checksum_help() calls with a packet drop, except
for the nf_hook_slow() case.  It may be possible to instead push the nf_hook_slow() instance
simply deeper into the netfilter code so that the iterate/parse/match phase need not invoke
such a heavy operation.  Only if netfilter will actually do something with the packet would
we skb_checksum_help() or whatever.  And I even believe that most cases where the packet is
mangled do not need to keep from still letting the card checksum the packet.