netdev - Re: [PATCH net-next v10 3/5] openvswitch: add support to push and pop mpls for layer3 packets

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: <CAOrHB_B3phbNub9sdnCM0EFDSL3fRHB-unKjUgjFMX5ZHqM3Dw@mail.gmail.com>
Date:	Tue, 7 Jun 2016 15:45:42 -0700
From:	pravin shelar <pshelar@....org>
To:	Simon Horman <simon.horman@...ronome.com>
Cc:	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	ovs dev <dev@...nvswitch.org>
Subject: Re: [PATCH net-next v10 3/5] openvswitch: add support to push and pop
 mpls for layer3 packets

On Mon, Jun 6, 2016 at 7:51 PM, Simon Horman <simon.horman@...ronome.com> wrote:
> On Thu, Jun 02, 2016 at 03:02:00PM -0700, pravin shelar wrote:
>> On Wed, Jun 1, 2016 at 11:24 PM, Simon Horman
>> <simon.horman@...ronome.com> wrote:
>> > Allow push and pop mpls actions to act on layer 3 packets by teaching
>> > them not to access non-existent L2 headers of such packets.
>> >
>> > Signed-off-by: Simon Horman <simon.horman@...ronome.com>
>> > ---
>> > v10
>> > * Limit scope of hdr in {push,pop}_mpls()
>> >
>> > v9
>> > * New Patch
>> > ---
>> >  net/openvswitch/actions.c | 19 ++++++++++++-------
>> >  1 file changed, 12 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> > index 9a3eb7a0ebf4..15f130e4c22b 100644
>> > --- a/net/openvswitch/actions.c
>> > +++ b/net/openvswitch/actions.c
>> > @@ -172,7 +172,8 @@ static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
>> >
>> >         skb_postpush_rcsum(skb, new_mpls_lse, MPLS_HLEN);
>> >
>> > -       update_ethertype(skb, eth_hdr(skb), mpls->mpls_ethertype);
>> > +       if (skb->mac_len)
>> > +               update_ethertype(skb, eth_hdr(skb), mpls->mpls_ethertype);
>> We can move all ethernet related code in this if block. for example memmove().
>
> My assumption is that the memmove() does nothing if skb->mac_len is zero
> and from my point of view it seems clean to leave it where it is unless
> the code around it also moves.
>
> Is there other code you think could/should be moved into the
> if (skb->mac_len) block?
>
yes, the code is correct. But I think grouping l2 related updated
together makes code easier to follow.