[ovs-dev] [PATCH 4/4] User-Space MPLS actions and matches

[Simon Horman]

On Thu, Oct 11, 2012 at 11:14:40AM +0900, Simon Horman wrote:

[snip]

> Below is a revised version of the patch which:
> 
> * Addresses the concerns you raise above, other
>  than the default Ethertype for push_mpls()
> * Incorporates the user-space datapath code as there
>   seem to be mutual dependencies between that and
>   the other user-space code.
> 
> I have pushed this and much more minor changes to the other
> patches in the series to the devel/mpls branch of
> git://github.com/horms/openvswitch.git
> (HEAD = 8b8c4af22ba5c1bddd1dc455fd8a5820cc3e2812)
> 
> I would repost the entire series, but as you haven't completed
> your review of this patch I thought it would be best to just
> post an update inline.

In the course of adding code to track the MPLS stack depth,
as suggested by Yamahata-san, I found some problems which
I have noted inline.

> 
> ----------------------------------------------------------------
> 
> From: Simon Horman <horms at verge.net.au>
> 
> User-Space MPLS actions and matches
> 
> This patch implements use-space datapath and non-datapath code
> to match and use the datapath API set out in Leo Alterman's patch
> "user-space datapath: Add basic MPLS support to kernel".
> 
> The resulting MPLS implementation supports:
> * Pushing a single MPLS label
> * Poping a single MPLS label
> * Modifying an MPLS lable using set-field or load actions
>   that act on the label value, tc and bos bit.
> * There is no support for manipulating the TTL
>   this is considered future work.
> 
> The single-level push pop limitation is implemented by processing
> push, pop and set-field/load actions in order and discarding information
> that would require multiple levels of push/pop to be supported.
> 
> e.g.
>    push,push -> the first push is discarded
>    pop,pop -> the first pop is discarded
> 
> This patch is based heavily on work by Ravi K.
> 
> Cc: Isaku Yamahata <yamahata at valinux.co.jp>
> Cc: Ravi K <rkerur at gmail.com>
> Signed-off-by: Simon Horman <horms at verge.net.au>

[snip]

> diff --git a/lib/flow.c b/lib/flow.c
> index 76d2340..5291e19 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -93,6 +93,28 @@ pull_icmpv6(struct ofpbuf *packet)
>  }
>  
>  static void
> +parse_remaining_mpls(struct ofpbuf *b)
> +{
> +    /* Proceed with parsing remaining MPLS headers. */
> +    struct mpls_hdr *mh;
> +    while ((mh = ofpbuf_try_pull(b, sizeof *mh)) &&
> +           (mh->mpls_lse & htonl(MPLS_BOS_MASK))) {
> +        ;
> +    }
> +}

I believe the logic above is (still!) wrong. I think that the
BoS check should be inverted yielding.

static void
parse_remaining_mpls(struct ofpbuf *b, struct flow *flow)
{
    /* Proceed with parsing remaining MPLS headers. */
    struct mpls_hdr *mh;
    while ((mh = ofpbuf_try_pull(b, sizeof *mh)) &&
           !(mh->mpls_lse & htonl(MPLS_BOS_MASK))) {
        flow->mpls_depth++;
    }
}

[snip]

> diff --git a/lib/packets.c b/lib/packets.c
> index 16f4fe6..0b85cf9 100644
> --- a/lib/packets.c
> +++ b/lib/packets.c

[snip]

> @@ -211,6 +212,361 @@ eth_pop_vlan(struct ofpbuf *packet)

[snip]

The dec_mpls_ttl(), copy_mpls_ttl_in() and copy_mpls_ttl_out() functions
below are not used by this patchset and I will remove them from the next
revision of this patch.

> +
> +/* Decrement MPLS TTL from the packet.
> + * 'packet->l2_5' must initially point to 'packet''s MPLS Label stack. */
> +void
> +dec_mpls_ttl(struct ofpbuf *packet, uint8_t new_ttl)
> +{
> +    ovs_be16 eth_type = htons(0);
> +    struct eth_header *eh = packet->data;
> +    struct mpls_hdr *mh = packet->l2_5;
> +
> +    if (packet->size < sizeof *eh) {
> +        return;
> +    }
> +
> +    /* Packet type should be mpls to decrement ttl. */
> +    eth_type = get_ethertype(packet);
> +
> +    if (eth_type == htons(ETH_TYPE_MPLS) ||
> +        eth_type == htons(ETH_TYPE_MPLS_MCAST)) {
> +
> +        /* Update decremented ttl into mpls header. */
> +        set_mpls_lse_ttl(&mh->mpls_lse, htonl(new_ttl << MPLS_TTL_SHIFT));
> +    }
> +}
> +
> +/* Copy MPLS TTL from the packet either ipv4/ipv6.
> + * 'packet->l2_5' must initially point to 'packet''s MPLS Label stack. */
> +void
> +copy_mpls_ttl_in(struct ofpbuf *packet, uint8_t new_ttl)
> +{
> +    struct eth_header *eh = packet->data;
> +    struct mpls_hdr *mh = packet->l2_5;
> +    struct ip_header *ih = packet->l3;
> +    struct ip6_hdr *ih6 = packet->l3;
> +    ovs_be16 eth_type = htons(0);
> +    size_t hdr_size = sizeof *eh + sizeof *mh + sizeof *ih;
> +
> +    if (packet->size < hdr_size) {
> +        return;
> +    }
> +
> +    /* Packet type should be mpls to copy ttl to l3. */
> +    eth_type = get_ethertype(packet);
> +    if (eth_type == htons(ETH_TYPE_MPLS) ||
> +        eth_type == htons(ETH_TYPE_MPLS_MCAST)) {
> +
> +        /* If bottom of the stack handle IP checksum. */
> +        if (mh->mpls_lse & htonl(MPLS_BOS_MASK)) {
> +            if (IP_VER(ih->ip_ihl_ver) == IP_VERSION) {
> +                /* Change the ip checksum. */
> +                uint8_t *field = &ih->ip_ttl;
> +                ih->ip_csum = recalc_csum16(ih->ip_csum,
> +                                 htons(*field << 8), htons(new_ttl << 8));
> +                ih->ip_ttl = new_ttl;
> +            } else if (IP6_VER(ih6->ip6_vfc) == IP6_VERSION) {
> +                ih6->ip6_hlim = new_ttl;
> +            }
> +        } else {
> +            struct mpls_hdr *mh2;
> +            mh2 = (struct mpls_hdr *)((char *) packet->l2_5 + sizeof *mh);
> +            set_mpls_lse_ttl(&mh2->mpls_lse, htonl(new_ttl << MPLS_TTL_SHIFT));
> +        }
> +    }
> +}
> +
> +/* Copy MPLS TTL to the packet layer3 only ipv4/ipv6.
> + * 'packet->l2_5' must initially point to 'packet''s MPLS Label stack. */
> +void
> +copy_mpls_ttl_out(struct ofpbuf *packet, uint8_t new_ttl)
> +{
> +    struct eth_header *eh = packet->data;
> +    struct mpls_hdr *mh = packet->l2_5;
> +    struct ip_header *ih = packet->l3;
> +    struct ip6_hdr   *ih6 = packet->l3;
> +    ovs_be16 eth_type = htons(0);
> +    size_t hdr_size = sizeof *eh + sizeof *mh + sizeof *ih;
> +
> +    /* TTL sent from ofproto-dpif.c is not the correct one,
> +     * hence ignore it. */
> +    if (packet->size < hdr_size) {
> +        return;
> +    }
> +
> +    /* Packet type should me mpls to copy ttl from l3. */
> +    eth_type = get_ethertype(packet);
> +    if (eth_type == htons(ETH_TYPE_MPLS) ||
> +        eth_type == htons(ETH_TYPE_MPLS_MCAST)) {
> +
> +        /* If bottom of the stack copy from l3. */
> +        if (mh->mpls_lse & htonl(MPLS_BOS_MASK)) {
> +            uint8_t nh_ttl;
> +            /* Get ipv4 or ipv6 or default ttl. */
> +            if (IP_VER(ih->ip_ihl_ver) == IP_VERSION) {
> +                nh_ttl = ih->ip_ttl;
> +            } else if (IP6_VER(ih6->ip6_vfc) == IP6_VERSION) {
> +                nh_ttl = ih6->ip6_hlim;
> +            } else {
> +                nh_ttl = 64; /* Default ttl for non-IP. */
> +            }
> +            set_mpls_lse_ttl(&mh->mpls_lse, htonl(nh_ttl << MPLS_TTL_SHIFT));
> +        } else {
> +            struct mpls_hdr *mh2;
> +            mh2 = (struct mpls_hdr *)((char *) packet->l2_5 + sizeof *mh);
> +            new_ttl = mpls_lse_to_ttl(mh2->mpls_lse);
> +            set_mpls_lse_ttl(&mh->mpls_lse, htonl(new_ttl << MPLS_TTL_SHIFT));
> +        }
> +    }
> +}

[snip]