netdev - Re: [PATCH net v3] openvswitch: Fix egress tunnel info.

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]

Date:	Fri, 23 Oct 2015 12:47:09 -0700
From:	Pravin Shelar <pshelar@...ira.com>
To:	Jiri Benc <jbenc@...hat.com>
Cc:	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net v3] openvswitch: Fix egress tunnel info.

On Fri, Oct 23, 2015 at 11:22 AM, Jiri Benc <jbenc@...hat.com> wrote:
> On Fri, 23 Oct 2015 10:30:24 -0700, Pravin Shelar wrote:
...
>
>> > This doesn't do what the name suggests and is, actually, ovs specific.
>> > The ip_tunnel_info can be provided as a part of lwtstate and this
>> > function should handle that case, too. This is not a problem for
>> > net.git, as the function just returns EINVAL in such case, but should
>> > be addressed for net-next.git. As ovs is currently the only user, I'd
>> > be also fine with just a comment stating that, so it's clear for future
>> > users of this function that it needs to be extended before it can be
>> > used out of ovs.
>> >
>>
>> I considered lwstate, but I am reluctant to add this complexity
>> without a usecase.
>
> I don't think it's complex. See the quick and dirty patch below (that
> has even a negative diffstat):
>
> diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
> index ce009710120c..3a1d3966a79e 100644
> --- a/include/net/dst_metadata.h
> +++ b/include/net/dst_metadata.h
> @@ -60,36 +60,25 @@ static inline struct metadata_dst *tun_rx_dst(int md_size)
>         return tun_dst;
>  }
>
> -static inline struct metadata_dst *tun_dst_unclone(struct sk_buff *skb)
> +static inline struct ip_tunnel_info *skb_tunnel_info_unclone(struct sk_buff *skb)
>  {
> -       struct metadata_dst *md_dst = skb_metadata_dst(skb);
> -       int md_size = md_dst->u.tun_info.options_len;
> +       struct metadata_dst *dst;
> +       struct ip_tunnel_info *info = skb_tunnel_info(skb);
>         struct metadata_dst *new_md;
>
> -       if (!md_dst)
> +       if (!info)
>                 return ERR_PTR(-EINVAL);
> -
> -       new_md = metadata_dst_alloc(md_size, GFP_ATOMIC);
> +       new_md = metadata_dst_alloc(info->options_len, GFP_ATOMIC);
>         if (!new_md)
>                 return ERR_PTR(-ENOMEM);
>
> -       memcpy(&new_md->u.tun_info, &md_dst->u.tun_info,
> -              sizeof(struct ip_tunnel_info) + md_size);
> +       memcpy(&new_md->u.tun_info, info,
> +              sizeof(struct ip_tunnel_info) + info->options_len);
>         skb_dst_drop(skb);
>         dst_hold(&new_md->dst);
>         skb_dst_set(skb, &new_md->dst);
> -       return new_md;
> -}
> -
This is not complete code. I found couple of issues with it.

This code does not copy lwtunnel_state state into new dst.
And it is converting lwtunnel dst into metadata dst.

> -static inline struct ip_tunnel_info *skb_tunnel_info_unclone(struct sk_buff *skb)
> -{
> -       struct metadata_dst *dst;
> -
> -       dst = tun_dst_unclone(skb);
> -       if (IS_ERR(dst))
> -               return NULL;
>
> -       return &dst->u.tun_info;
> +       return info;
>  }
>
--
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