Date:   Fri, 19 Jan 2018 17:11:29 +1100
From:   Daniel Axtens <dja@...ens.net>
To:     Pravin Shelar <pshelar@....org>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Manish.Chopra@...ium.com, ovs dev <dev@...nvswitch.org>
Subject: Re: [PATCH 0/3] Check gso_size of packets when forwarding

Daniel Axtens <dja@...ens.net> writes:

> Pravin Shelar <pshelar@....org> writes:
>
>> On Thu, Jan 18, 2018 at 5:08 AM, Daniel Axtens <dja@...ens.net> wrote:
>>> Pravin Shelar <pshelar@....org> writes:
>>>
>>>> On Mon, Jan 15, 2018 at 6:09 PM, Daniel Axtens <dja@...ens.net> wrote:
>>>>> 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.
>
> I do take the point that you and Jason are making: a more generic
> fix would  be good. I'm just not sure where to put it.

How about this? It puts the check in validate_xmit_skb(). (completely untested)

Ultimately I think we need to make a broader policy decision about who
is responsible for ensuring that packets are correctly sized - is it the
caller of dev_queue_xmit (as in bridges and openswitch) or is it lower
down the stack, such as validate_xmit_skb()? Making the caller check is
more efficient - packets created on the system are always going to fit
due to the checks in the protocol code, so rechecking is inefficient and
unnecessary. But checking later is more reliable as we don't have to
identify all forwarding paths.

Regards,
Daniel

diff --git a/net/core/dev.c b/net/core/dev.c
index 5af04be128c1..b8c3f33ece19 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1830,13 +1830,11 @@ static inline void net_timestamp_set(struct sk_buff *skb)
                        __net_timestamp(SKB);           \
        }                                               \
 
-bool is_skb_forwardable(const struct net_device *dev, const struct sk_buff *skb)
+static inline bool does_skb_fit_mtu(const struct net_device *dev,
+                                   const struct sk_buff *skb)
 {
        unsigned int len;
 
-       if (!(dev->flags & IFF_UP))
-               return false;
-
        len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
        if (skb->len <= len)
                return true;
@@ -1850,6 +1848,14 @@ bool is_skb_forwardable(const struct net_device *dev, const struct sk_buff *skb)
 
        return false;
 }
+
+bool is_skb_forwardable(const struct net_device *dev, const struct sk_buff *skb)
+{
+       if (!(dev->flags & IFF_UP))
+               return false;
+
+       return does_skb_fit_mtu(dev, skb);
+}
 EXPORT_SYMBOL_GPL(is_skb_forwardable);
 
 int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
@@ -3081,6 +3087,9 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
        if (unlikely(!skb))
                goto out_null;
 
+       if (unlikely(!does_skb_fit_mtu(dev, skb)))
+               goto out_kfree_skb;
+
        if (netif_needs_gso(skb, features)) {
                struct sk_buff *segs;
 



>
>
> Regards,
> Daniel

Powered by blists - more mailing lists