netdev - Re: [PATCH v2.57] datapath: Add basic MPLS support to kernel

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]

Message-ID: <CAEP_g=-X=T2hJoXTV5ptZRvf1WPgYa1xusczwSR1bUYAZ5yaqA@mail.gmail.com>
Date:	Mon, 19 May 2014 18:34:05 -0700
From:	Jesse Gross <jesse@...ira.com>
To:	Simon Horman <horms@...ge.net.au>
Cc:	"dev@...nvswitch.org" <dev@...nvswitch.org>,
	netdev <netdev@...r.kernel.org>,
	Pravin B Shelar <pshelar@...ira.com>,
	Ben Pfaff <blp@...ira.com>, Ravi K <rkerur@...il.com>,
	Joe Stringer <joe@...d.net.nz>
Subject: Re: [PATCH v2.57] datapath: Add basic MPLS support to kernel

I have some miscellaneous comments on things that I noticed, all of
which are pretty small. I will probably have a few more tomorrow but
my hope is that we can get this in soon.

On Thu, May 15, 2014 at 4:07 PM, Simon Horman <horms@...ge.net.au> wrote:
> diff --git a/datapath/actions.c b/datapath/actions.c
> index 603c7cb..7c3ae0c 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> +static int push_mpls(struct sk_buff *skb,
> +                    const struct ovs_action_push_mpls *mpls)
> +{
> +       __be32 *new_mpls_lse;
> +       struct ethhdr *hdr;
> +
> +       if (skb_cow_head(skb, MPLS_HLEN) < 0) {
> +               kfree_skb(skb);
> +               return -ENOMEM;
> +       }

I think it would be better to not free the skb on error here - it
introduces an exception case in the call to push_mpls() that would be
otherwise handled if we didn't free it.

When we set the EtherType at the bottom of the function, I don't think
that it is correct in the presence of VLAN tags because it will set
the outer EtherType.

> +static int pop_mpls(struct sk_buff *skb, const __be16 ethertype)
> +{
> +       struct ethhdr *hdr;
> +       int err;
> +
> +       if (unlikely(skb->len < skb->mac_len + MPLS_HLEN))
> +               return -EINVAL;

I don't think this check is necessary since we would have rejected
such packets at flow validation time.

> +static int set_mpls(struct sk_buff *skb, const __be32 *mpls_lse)
> +{
> +       __be32 *stack = (__be32 *)mac_header_end(skb);
> +       int err;
> +
> +       err = make_writable(skb, skb->mac_len + MPLS_HLEN);
> +       if (unlikely(err))
> +               return err;
> +
> +       if (skb->ip_summed == CHECKSUM_COMPLETE) {
> +               __be32 diff[] = { ~(*stack), *mpls_lse };
> +               skb->csum = ~csum_partial((char *)diff, sizeof(diff),
> +                                         ~skb->csum);
> +       }

Is it possible to use csum_replace4() to simplify this?

> @@ -701,6 +815,8 @@ int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb, bool recirc)
>                 goto out_loop;
>         }
>
> +       ovs_skb_init_inner_protocol(skb);

I think we talked about some time ago but it seems like this will get
reset by recirculation (although maybe it's unlikely that
recirculation will get used on output).

Also, maybe I'm missing something but I don't see where we actually
set the inner protocol.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html