* David S. Miller <20050124164049.3b939791.davem@xxxxxxxxxxxxx> 2005-01-24 16:40
> On Tue, 25 Jan 2005 11:07:59 +1100
> Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> wrote:
>
> > On Tue, Jan 25, 2005 at 12:45:15AM +0100, Thomas Graf wrote:
> > >
> > > I don't quite understand how this solves the problem. How could
> > > ip_summed be non zero after ip_forward? The earliest possible call
> > > to ip_fragment is in postrouting. Please correct me if I'm wrong.
> >
> > ip_forward only sets ip_summed for the packet at the head. It does
> > not clear ip_summed for the fragments themselves.
You are indeed right, I actually thought it would be like this first but
tricked myself into not believeing in this anymore because it would have
meant that every fragmented sequence could crash a kernel with one of
the drivers setting ip_summed to HW and the card not being able to do
the checksummin itself, so I thought I'd have missed some magic.
As it seems, it is really the case, a first peeks shows that the
following drivers set ip_summed to HW under some circumstances:
acenic, he, hamachi, starfire, sungem, happy meal
> However, if we forward this thing, only the head SKB in the
> fraglist has it's skb->ip_summed value reset. Then ip_output()
> sees that the head skb has a length greater than the dst_pmtu()
> and we get to ip_fragment() which is where Herbert's fix is
> located.
> We want to preserve local IP protocol input usage of CHECKSUM_HW
> information on such frag list SKBs, so the location to fix this
> is in fact ip_fragment() and not any of the code in ip_fragment.c
Agreed.
I've been talking to David on IRC and we have been able to reproduce the
bug by sending a sequence of fragmented ip packets to a host behind
the router. The only objection I had with this patch was, that I found
it too unlikely that his box wouldn't receive fragmented packets more
often, but as it seems it is really is this simple.
Still, one problem remains, even if ip_summed was incorrect,
skb_checksum_help should never be called in dev_queue_xmit for the
acenic driver since it sets NETIF_F_IP_CSUM. For some reason, features
is set to 0 while checking for it.
|