netdev - Re: [PATCH v3 net-next 2/3] openvswitch: Use is_skb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 29 Nov 2016 23:23:34 -0800
From:   Pravin Shelar <pshelar@....org>
To:     Jarno Rajahalme <jarno@....org>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jiri Benc <jbenc@...hat.com>, Eric Garver <e@...g.me>
Subject: Re: [PATCH v3 net-next 2/3] openvswitch: Use is_skb_forwardable() for
 length check.

On Tue, Nov 29, 2016 at 3:30 PM, Jarno Rajahalme <jarno@....org> wrote:
> Use is_skb_forwardable() instead of an explicit length check.  This
> gets around the apparent MTU check failure in OVS test cases when
> skb->protocol is not properly set in case of non-accelerated VLAN
> skbs.
>
> Suggested-by: Pravin Shelar <pshelar@....org>
> Fixes: 5108bbaddc ("openvswitch: add processing of L3 packets")
> Signed-off-by: Jarno Rajahalme <jarno@....org>
> ---
> v3: New patch suggested by Pravin.
>
> net/openvswitch/vport.c | 38 ++++++++++++++------------------------
>  1 file changed, 14 insertions(+), 24 deletions(-)
>
> diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
> index b6c8524..076b39f 100644
> --- a/net/openvswitch/vport.c
> +++ b/net/openvswitch/vport.c

>  void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto)
>  {
> -       int mtu = vport->dev->mtu;
> -
>         switch (vport->dev->type) {
>         case ARPHRD_NONE:
>                 if (mac_proto == MAC_PROTO_ETHERNET) {
> @@ -504,11 +485,20 @@ void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto)
>                 goto drop;
>         }
>
> -       if (unlikely(packet_length(skb, vport->dev) > mtu &&
> -                    !skb_is_gso(skb))) {
> -               net_warn_ratelimited("%s: dropped over-mtu packet: %d > %d\n",
> -                                    vport->dev->name,
> -                                    packet_length(skb, vport->dev), mtu);
> +       if (unlikely(!is_skb_forwardable(vport->dev, skb))) {

This would easy to read if you inverse the if condition here.

> +               /* Log only if the device is up. */
> +               if (vport->dev->flags & IFF_UP) {

since is_skb_forwardable() is checking for IFF_UP we can remove same
check from internal-device send() op.

> +                       unsigned int length = skb->len
> +                               - vport->dev->hard_header_len;
> +
> +                       if (!skb_vlan_tag_present(skb)
> +                           && eth_type_vlan(skb->protocol))
> +                               length -= VLAN_HLEN;
> +
> +                       net_warn_ratelimited("%s: dropped over-mtu packet %d > %d\n",
> +                                            vport->dev->name, length,
> +                                            vport->dev->mtu);
> +               }
>                 vport->dev->stats.tx_errors++;
>                 goto drop;
>         }
> --
> 2.1.4
>