Re: [PATCH 0/2] Disable forwarding of LRO skbs

[Kieran Mansley <kmansley@solarflare.com>]

On Thu, 2008-05-01 at 18:19 +0800, Herbert Xu wrote:
> David Miller <davem@davemloft.net> wrote:
> > From: Ben Hutchings <bhutchings@solarflare.com>
> > Date: Wed, 30 Apr 2008 22:48:46 +0100
> > 
> >> Large Receive Offload (LRO) destroys packet headers that should be
> >> preserved when forwarding.  Currently it also triggers a BUG() or WARN()
> >> in skb_gso_segment().  We should disable it wherever forwarding is
> >> enabled, and discard LRO skbs with a warning if it is turned back on.
> > 
> > Thanks for reposting your work, I'll take a closer look at these
> > two patches later today.
> 
> This patch completely breaks forwarding of GSO packets in a
> virtualised environment where we explicitly want to forward
> them as is to reduce per-packet overhead.  So if LRO is causing
> problems then I suggest we find a better way around that that
> does not penalise all forms of GSO.

If I remember rightly the nub of the matter as far as that is concerned
is that both LRO and GSO use the gso_size field in the SKB.  Normally
this overloading is fine as LRO is receive-side and GSO is send-side and
so the two code paths are different and react to the gso_size field
appropriately.  However, when an LRO packet is forwarded by a bridge to
another device it then appears on the send path and the gso_size field
being set (because it is an LRO packet) means it is interpreted as being
a GSO packet.  To some extent this should be OK - fragmenting it should
be really easy as it's already in what are likely to be appropriate
sized chunks in the frag_list - but the code at the moment generates
warnings and BUGs in this case as it's (understandably) not been written
to cope with both LRO and GSO packets.  There's no reason why it has to
be that way however.

Fundamentally I think it would be an improvement to have some definite
way to distinguish LRO and GSO packets, regardless of whether it can
cope with both.  If that were available, it would be possible to warn on
LRO packets without affecting the GSO packets being forwarded in
virtualised environements.

As an aside, the gso_size field isn't the only one that has a slightly
different meaning depending on whether the SKB was generated by a stack
or a device.  The ip_summed field is I think another one, and would
surely need special handling by  the bridge when forwarding packets.  If
it's already doing this, there's a precedent for fixing things up there.

Kieran