[ovs-dev] [PATCH 1/4] datapath: Add basic MPLS support to kernel

[Isaku Yamahata]

On Wed, Oct 10, 2012 at 07:31:25PM +0900, Simon Horman wrote:
> On Wed, Oct 10, 2012 at 11:57:15AM +0900, Isaku Yamahata wrote:
> > On Tue, Oct 09, 2012 at 04:08:32PM +0900, Simon Horman wrote:
> > > +static int push_mpls(struct sk_buff *skb, const struct ovs_action_push_mpls *mpls)
> > > +{
> > > +	u32 l2_size;
> > > +	__be32 *new_mpls_label;
> > > +
> > > +	if (skb_cow_head(skb, MPLS_HLEN) < 0) {
> > > +		kfree_skb(skb);
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	l2_size = skb_cb_mpls_stack_offset(skb);
> > > +	skb_push(skb, MPLS_HLEN);
> > > +	memmove(skb_mac_header(skb) - MPLS_HLEN, skb_mac_header(skb), l2_size);
> > > +	skb_reset_mac_header(skb);
> > > +
> > > +	new_mpls_label = (__be32 *)(skb_mac_header(skb) + l2_size);
> > > +	*new_mpls_label = mpls->mpls_label;
> > > +
> > > +	set_ethertype(skb, mpls->mpls_ethertype);
> > > +	return 0;
> > > +}
> > > +
> > > +static int pop_mpls(struct sk_buff *skb, const __be16 *ethertype)
> > > +{
> > > +	__be16 current_ethertype = get_ethertype(skb);
> > > +	if (current_ethertype == htons(ETH_P_MPLS_UC) ||
> > > +		current_ethertype == htons(ETH_P_MPLS_MC)) {
> > > +		u32 l2_size = skb_cb_mpls_stack_offset(skb);
> > > +
> > > +		memmove(skb_mac_header(skb) + MPLS_HLEN, skb_mac_header(skb), l2_size);
> > > +
> > > +		skb_pull(skb, MPLS_HLEN);
> > > +		skb_reset_mac_header(skb);
> > > +
> > > +		set_ethertype(skb, *ethertype);
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +static int set_mpls(struct sk_buff *skb, const __be32 *mpls_label)
> > > +{
> > > +	__be16 current_ethertype = get_ethertype(skb);
> > > +	if (current_ethertype == htons(ETH_P_MPLS_UC) ||
> > > +		current_ethertype == htons(ETH_P_MPLS_MC)) {
> > > +		memcpy(skb_cb_mpls_stack(skb), mpls_label, sizeof(__be32));
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > 
> > push_mpls and set_mpls overwrite BOS bit. So it can produce corrupted packet.
> > Is this by design?
> 
> My personal opinion is that it is up to the caller of set_mpls()
> to provide a valid label. However, I'm happy to discuss other ideas.

I see. I have no objection. I just wanted to make sure that it's by design.
-- 
yamahata