netdev - Re: [PATCH v2.29] 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] [day] [month] [year] [list]

Message-ID: <CAEP_g=8DmRM7C6OtVjQXS5t7ofOZANWTu_h4xfZSP9RNga97BQ@mail.gmail.com>
Date:	Thu, 23 May 2013 09:46:25 -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>, Ravi K <rkerur@...il.com>,
	Isaku Yamahata <yamahata@...inux.co.jp>,
	Ben Pfaff <blp@...ira.com>,
	Jarno Rajahalme <jarno.rajahalme@....com>
Subject: Re: [PATCH v2.29] datapath: Add basic MPLS support to kernel

On Wed, May 22, 2013 at 6:38 PM, Simon Horman <horms@...ge.net.au> wrote:
> On Tue, May 21, 2013 at 09:07:06AM -0700, Jesse Gross wrote:
>> On Mon, May 20, 2013 at 5:55 PM, Simon Horman <horms@...ge.net.au> wrote:
>> > On Fri, May 17, 2013 at 04:14:56PM -0700, Jesse Gross wrote:
>> >> On Fri, May 17, 2013 at 12:06 AM, Simon Horman <horms@...ge.net.au> wrote:
>> >> > +static int push_mpls(struct sk_buff *skb,
>> >> > +                    const struct ovs_action_push_mpls *mpls)
>> >> > +{
>> >> > +       __be32 *new_mpls_lse;
>> >> > +       int err;
>> >> > +
>> >> > +       err = make_writable(skb, skb->mac_len + MPLS_HLEN);
>> >> > +       if (unlikely(err))
>> >> > +               return err;
>> >> > +
>> >> > +       skb_push(skb, MPLS_HLEN);
>> >>
>> >> What happens if there isn't enough headroom?
>> >
>> > Good point. How about this?
>> >
>> >         if (unlikely(skb->data<skb->head))
>> >                 return -EINVAL;
>> >         skb_push(skb, MPLS_HLEN);
>>
>> The amount of headroom is an internal kernel property so we can't
>> reject actions on the basis of it. We need to expand the headroom,
>> similar to __vlan_put_tag().
>
> Thanks. I have changed the code around and it is now as follows.
> I am a little unsure if the combination of make_writable() and
> skb_cow_head() is sensible.
>
>         err = make_writable(skb, skb->mac_len + MPLS_HLEN);
>         if (unlikely(err))
>                 return err;
>
>         if (skb_cow_head(skb, MPLS_HLEN) < 0)
>                 return -ENOMEM;
>
>         skb_push(skb, MPLS_HLEN);

I would reverse the order of make_writable() and skb_cow_head() to
avoid possibly having to reallocate twice. I think you also don't need
to add MPLS_HLEN to the length in make_writable() because that
function is ensuring that existing packet data can be modified and the
MPLS label isn't there yet.
--
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