Re: [PATCH 0/3] Check gso_size of packets when forwarding

Pravin Shelar [off-list ref] writes:

On Thu, Jan 18, 2018 at 5:28 PM, Daniel Axtens [off-list ref] wrote:

quoted

Pravin Shelar [off-list ref] writes:

quoted

On Thu, Jan 18, 2018 at 5:08 AM, Daniel Axtens [off-list ref] wrote:

quoted

Pravin Shelar [off-list ref] writes:

quoted

On Mon, Jan 15, 2018 at 6:09 PM, Daniel Axtens [off-list ref] wrote:

quoted

When regular packets are forwarded, we validate their size against the
MTU of the destination device. However, when GSO packets are
forwarded, we do not validate their size against the MTU. We
implicitly assume that when they are segmented, the resultant packets
will be correctly sized.

This is not always the case.

We observed a case where a packet received on an ibmveth device had a
GSO size of around 10kB. This was forwarded by Open vSwitch to a bnx2x
device, where it caused a firmware assert. This is described in detail
at [0] and was the genesis of this series. Rather than fixing it in
the driver, this series fixes the forwarding path.

Are there any other possible forwarding path in networking stack? TC
is one subsystem that could forward such a packet to the bnx2x device,
how is that handled ?

So far I have only looked at bridges, openvswitch and macvlan. In
general, if the code uses dev_forward_skb() it should automatically be
fine as that invokes is_skb_forwardable(), which we patch.

But there are other ways to forward packets, e.g tc-mirred or bpf
redirect. We need to handle all these cases rather than fixing one at
a time. As Jason suggested netif_needs_gso() looks like good function
to validate if a device is capable of handling given GSO packet.

I am not entirely sure this is a better solution.

The biggest reason I am uncomfortable with this is that if
netif_needs_gso() returns true, the skb will be segmented. The segment
sizes will be based on gso_size. Since gso_size is greater than the MTU,
the resulting segments will themselves be over-MTU. Those over-MTU
segments will then be passed to the network card. I think we should not
be creating over-MTU segments; we should instead be dropping the packet
and logging.

Would this oversized segment cause the firmware assert?
If this solves the assert issue then I do not see much value in adding
checks in fast-path just for logging.

No - I tested this (or rather, as I don't have direct access to a bnx2x
card, this was tested on my behalf): as long as the packet is not a GSO
packet, it doesn't cause the crash. So we *could* segment them, I just
think that knowingly creating invalid segments is not particularly
pleasant.

Do you think my approach in a later email which checks and drops without
logging is sufficient? It is the same GSO check, and also checks non-GSO
packets against the MTU.

Regards,
Daniel