Disable forwarding of LRO skbs

Add skb_warn_if_lro() to test whether an skb was received with LRO and warn if so.

Change br_forward(), ip_forward() and ip6_forward() to call skb_warn_if_lro() and discard the skb if it returns true.

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.

From: Ben Hutchings <[email protected]>
Date: Wed, 30 Apr 2008 22:48:46 +0100

Quoted 4 lines

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.

From: Herbert Xu <[email protected]>
Date: Thu, 01 May 2008 18:19:54 +0800

Quoted 5 lines

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.

I assume you mean forwarding via a bridge?

I think we'll need to add a device attribute for what you want.

I still don't understand why we can't forward LRO packets. It should be no different to forwarding GSO packets if it's properly constructed.

Quoted 4 lines

I still don't understand why we can't forward LRO packets. It should be no different to forwarding GSO packets if it's properly constructed.

Nevermind, doing LRO on forwarded traffic is fundamentally broken because it breaks the end-to-end connection.

Ok, that brings us back to your virtualization issues.

So what we want to detect/forbid isn't the forwarding of GSO packets, but the LRO of packets which aren't destined for the local host.

I still don't understand why we can't forward LRO packets. It should be no different to forwarding GSO packets if it's properly constructed.

Perhaps the issue is that it's expensive to do that, and thus it could harm routing performance in such cases.

Quoted 8 lines · David Miller

From: Ben Hutchings <[email protected]>
Date: Wed, 30 Apr 2008 22:48:46 +0100

Quoted 4 lines

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.

Quoted 2 lines

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.

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.

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.

Quoted 4 lines · Ben Hutchings

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.

I don't think forwarding GSO packets is bad per se. However, doing LRO on forwarded traffic is broken because it breaks the end-to-end connection. So I agree that we should detect this but preferrably not based on the GSO flag as LRO isn't the only source of GSO traffic and the rest of them can be forwarded just fine.

I was going to raise the virtualisation issue because Kieran has been in contact with XenSource over this and they believed LRO was not a problem in Xen. So long as the bridges in dom0 each connect a single physical interface to other domains running paravirtualised Linux, this is probably true.

Yes, everything works fine in that case. It's only when LRO packets are forwarded to another physical interface that things go wrong.

Quoted 3 lines · Kieran Mansley

Yes, everything works fine in that case. It's only when LRO packets are forwarded to another physical interface that things go wrong.

Now this is definitely broken because it fiddles with TCP packets in the middle, but still it should *work* in as much as making the packet go out. Well except that you need to fix up the checksum so that it only contains the pseudo-header on output. That's probably what you need to address if we failed in avoiding LRO on forwarded traffic.

If I remember rightly it also gets upset when it comes to split the packet because the frag_list is already in use; it expects this not to be the case for GSO but it's valid for an LRO packet to do this I think. I don't think there's anything fundamentally difficult here, it's just that the code has never been written to cope.

Quoted 6 lines · Kieran Mansley

If I remember rightly it also gets upset when it comes to split the packet because the frag_list is already in use; it expects this not to be the case for GSO but it's valid for an LRO packet to do this I think. I don't think there's anything fundamentally difficult here, it's just that the code has never been written to cope.

Oh so LRO packets still preserve the original packet boundaries? That's even easier. We could just make the GSO output code detect the presence of frag_list and fragment based on that.

Depending on the driver, the packets may be received into skbs which are then attached to frag_list, or into page buffers which go in the frags array. If most packets can be merged by LRO, deferring skb allocation and using the frags array is a performance win.

Quoted 5 lines · Ben Hutchings

Depending on the driver, the packets may be received into skbs which are then attached to frag_list, or into page buffers which go in the frags array. If most packets can be merged by LRO, deferring skb allocation and using the frags array is a performance win.

But for each packet it's either going to be frag_list or page frags, right? So we could still fragment based on frag_list if it's present.

My understanding is that at the points I'm adding the check, we're only considering received skbs and gso_size will only be set if LRO was used.

Quoted 3 lines · Ben Hutchings

My understanding is that at the points I'm adding the check, we're only considering received skbs and gso_size will only be set if LRO was used.

Unfortunately that's not the case. The forwarding path can and does occur for GSO packets sent from other guests. The obvious case would be a GSO packet from a guest forwarded by the host (as opposed to bridged) to the outside world.

Quoted 9 lines · Herbert Xu

Quoted 3 lines · Ben Hutchings

My understanding is that at the points I'm adding the check, we're only considering received skbs and gso_size will only be set if LRO was used.

Unfortunately that's not the case. The forwarding path can and does occur for GSO packets sent from other guests. The obvious case would be a GSO packet from a guest forwarded by the host (as opposed to bridged) to the outside world.

Can we distinguish them by testing skb->ip_summed == CHECKSUM_PARTIAL? That is a requirement for using hardware segmentation offload (see netif_needs_gso()) but an skb resulting from LRO should have ip_summed set to either CHECKSUM_UNNECESSARY or CHECKSUM_NONE.

Quoted 5 lines

Can we distinguish them by testing skb->ip_summed == CHECKSUM_PARTIAL? That is a requirement for using hardware segmentation offload (see netif_needs_gso()) but an skb resulting from LRO should have ip_summed set to either CHECKSUM_UNNECESSARY or CHECKSUM_NONE.

Actually, it seems like there's a cleaner test: gso_size != 0 && gso_type == 0 (if we really want GSO then the latter must be set). I'll have to try that out.