Re: net: vrf: Handle ipv6 multicast and link-local addresses

Re: net: vrf: Handle ipv6 multicast and link-local addresses

[Geert Uytterhoeven]

On Wed, Jul 27, 2016 at 11:52 PM, Linux Kernel Mailing List
<linux-kernel@vger.kernel.org> wrote:
> Web:        https://git.kernel.org/torvalds/c/9ff74384600aeecba34ebdacbbde0627489ff601
> Commit:     9ff74384600aeecba34ebdacbbde0627489ff601
> Parent:     ba46ee4c0ed122fa14aa2f5d6994c166a01ae2c0
> Refname:    refs/heads/master
> Author:     David Ahern <dsa@cumulusnetworks.com>
> AuthorDate: Mon Jun 13 13:44:19 2016 -0700
> Committer:  David S. Miller <davem@davemloft.net>
> CommitDate: Wed Jun 15 12:34:34 2016 -0700
>
>     net: vrf: Handle ipv6 multicast and link-local addresses
>
>     IPv6 multicast and link-local addresses require special handling by the
>     VRF driver:
>     1. Rather than using the VRF device index and full FIB lookups,
>        packets to/from these addresses should use direct FIB lookups based on
>        the VRF device table.
>
>     2. fail sends/receives on a VRF device to/from a multicast address
>        (e.g, make ping6 ff02::1%<vrf> fail)
>
>     3. move the setting of the flow oif to the first dst lookup and revert
>        the change in icmpv6_echo_reply made in ca254490c8dfd ("net: Add VRF
>        support to IPv6 stack"). Linklocal/mcast addresses require use of the
>        skb->dev.
>
>     With this change connections into and out of a VRF enslaved device work
>     for multicast and link-local addresses work (icmp, tcp, and udp)
>     e.g.,
>
>     1. packets into VM with VRF config:
>         ping6 -c3 fe80::e0:f9ff:fe1c:b974%br1
>         ping6 -c3 ff02::1%br1
>
>         ssh -6 fe80::e0:f9ff:fe1c:b974%br1
>
>     2. packets going out a VRF enslaved device:
>         ping6 -c3 fe80::18f8:83ff:fe4b:7a2e%eth1
>         ping6 -c3 ff02::1%eth1
>         ssh -6 root@fe80::18f8:83ff:fe4b:7a2e%eth1
>
>     Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
>  drivers/net/vrf.c       | 98 ++++++++++++++++++++++++++++++++++++++++++++++---
>  include/net/ip6_route.h |  2 +
>  net/ipv6/icmp.c         |  2 +-
>  net/ipv6/route.c        |  5 ++-
>  4 files changed, 99 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
> index d2ce76c..0b5b3c2 100644
> --- a/drivers/net/vrf.c
> +++ b/drivers/net/vrf.c
> @@ -785,9 +785,63 @@ out:
>         return rc;
>  }
>
> +static struct rt6_info *vrf_ip6_route_lookup(struct net *net,
> +                                            const struct net_device *dev,
> +                                            struct flowi6 *fl6,
> +                                            int ifindex,
> +                                            int flags)
> +{
> +       struct net_vrf *vrf = netdev_priv(dev);
> +       struct fib6_table *table = NULL;
> +       struct rt6_info *rt6;
> +
> +       rcu_read_lock();
> +
> +       /* fib6_table does not have a refcnt and can not be freed */
> +       rt6 = rcu_dereference(vrf->rt6);
> +       if (likely(rt6))
> +               table = rt6->rt6i_table;
> +
> +       rcu_read_unlock();
> +
> +       if (!table)
> +               return NULL;
> +
> +       return ip6_pol_route(net, table, ifindex, fl6, flags);
> +}
> +
> +static void vrf_ip6_input_dst(struct sk_buff *skb, struct net_device *vrf_dev,
> +                             int ifindex)
> +{
> +       const struct ipv6hdr *iph = ipv6_hdr(skb);
> +       struct flowi6 fl6 = {
> +               .daddr          = iph->daddr,
> +               .saddr          = iph->saddr,
> +               .flowlabel      = ip6_flowinfo(iph),

The above assignment causes the following compiler warning with
m68k-linux-gnu-gcc-4.1:

    drivers/net/vrf.c: In function ‘vrf_ip6_input_dst’:
    drivers/net/vrf.c:870: warning: initialized field with
side-effects overwritten
    drivers/net/vrf.c:870: warning: (near initialization for ‘fl6’)

Unfortunately I have no idea what it means, nor do I see what's wrong
with the code.

> +               .flowi6_mark    = skb->mark,
> +               .flowi6_proto   = iph->nexthdr,
> +               .flowi6_iif     = ifindex,
> +       };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: net: vrf: Handle ipv6 multicast and link-local addresses

[David Ahern]

On 8/3/16 1:57 PM, Geert Uytterhoeven wrote:
>> +static void vrf_ip6_input_dst(struct sk_buff *skb, struct net_device *vrf_dev,
>> > +                             int ifindex)
>> > +{
>> > +       const struct ipv6hdr *iph = ipv6_hdr(skb);
>> > +       struct flowi6 fl6 = {
>> > +               .daddr          = iph->daddr,
>> > +               .saddr          = iph->saddr,
>> > +               .flowlabel      = ip6_flowinfo(iph),
> The above assignment causes the following compiler warning with
> m68k-linux-gnu-gcc-4.1:
> 
>     drivers/net/vrf.c: In function ‘vrf_ip6_input_dst’:
>     drivers/net/vrf.c:870: warning: initialized field with
> side-effects overwritten
>     drivers/net/vrf.c:870: warning: (near initialization for ‘fl6’)
> 
> Unfortunately I have no idea what it means, nor do I see what's wrong
> with the code.

no idea. Fields are initialized once and left and right data types are the same.

Can you remove one line at a time? Line 870 is ".flowi6_proto   = iph->nexthdr," but all of the flowi6 macros are unique references to unique fields in flowi_common. The flowlabel line you point out is a unique field as well.

Can you run pahole on file that did compile? e.g.,

pahole -C 'flowi6' net/ipv6/route.o

and get the common struct too:

pahole -C 'flowi_common' net/ipv6/route.o

> 
>> > +               .flowi6_mark    = skb->mark,
>> > +               .flowi6_proto   = iph->nexthdr,
>> > +               .flowi6_iif     = ifindex,
>> > +       };

Re: net: vrf: Handle ipv6 multicast and link-local addresses

[Geert Uytterhoeven]

On Wed, Aug 3, 2016 at 10:11 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
> On 8/3/16 1:57 PM, Geert Uytterhoeven wrote:
>>> +static void vrf_ip6_input_dst(struct sk_buff *skb, struct net_device *vrf_dev,
>>> > +                             int ifindex)
>>> > +{
>>> > +       const struct ipv6hdr *iph = ipv6_hdr(skb);
>>> > +       struct flowi6 fl6 = {
>>> > +               .daddr          = iph->daddr,
>>> > +               .saddr          = iph->saddr,
>>> > +               .flowlabel      = ip6_flowinfo(iph),
>> The above assignment causes the following compiler warning with
>> m68k-linux-gnu-gcc-4.1:
>>
>>     drivers/net/vrf.c: In function ‘vrf_ip6_input_dst’:
>>     drivers/net/vrf.c:870: warning: initialized field with
>> side-effects overwritten
>>     drivers/net/vrf.c:870: warning: (near initialization for ‘fl6’)
>>
>> Unfortunately I have no idea what it means, nor do I see what's wrong
>> with the code.
>
> no idea. Fields are initialized once and left and right data types are the same.
>
> Can you remove one line at a time? Line 870 is ".flowi6_proto   = iph->nexthdr," but all of the flowi6 macros are unique references to unique fields in flowi_common. The flowlabel line you point out is a unique field as well.

The only thing that seems to matter is assigning the result of the call to
ip6_flowinfo() to .flowlabel. Assigning a constant makes the warning go away.

Yeah, the 870 line number is funny, as it doesn't point to the offending line.

> Can you run pahole on file that did compile? e.g.,
>
> pahole -C 'flowi6' net/ipv6/route.o
>
> and get the common struct too:
>
> pahole -C 'flowi_common' net/ipv6/route.o

No output. Perhaps pahole doesn't play well with cross-compiling?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: net: vrf: Handle ipv6 multicast and link-local addresses

[David Ahern]

On 8/3/16 2:27 PM, Geert Uytterhoeven wrote:
> On Wed, Aug 3, 2016 at 10:11 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
>> On 8/3/16 1:57 PM, Geert Uytterhoeven wrote:
>>>> +static void vrf_ip6_input_dst(struct sk_buff *skb, struct net_device *vrf_dev,
>>>>> +                             int ifindex)
>>>>> +{
>>>>> +       const struct ipv6hdr *iph = ipv6_hdr(skb);
>>>>> +       struct flowi6 fl6 = {
>>>>> +               .daddr          = iph->daddr,
>>>>> +               .saddr          = iph->saddr,
>>>>> +               .flowlabel      = ip6_flowinfo(iph),
>>> The above assignment causes the following compiler warning with
>>> m68k-linux-gnu-gcc-4.1:
>>>
>>>     drivers/net/vrf.c: In function ‘vrf_ip6_input_dst’:
>>>     drivers/net/vrf.c:870: warning: initialized field with
>>> side-effects overwritten
>>>     drivers/net/vrf.c:870: warning: (near initialization for ‘fl6’)
>>>
>>> Unfortunately I have no idea what it means, nor do I see what's wrong
>>> with the code.
>>
>> no idea. Fields are initialized once and left and right data types are the same.
>>
>> Can you remove one line at a time? Line 870 is ".flowi6_proto   = iph->nexthdr," but all of the flowi6 macros are unique references to unique fields in flowi_common. The flowlabel line you point out is a unique field as well.
> 
> The only thing that seems to matter is assigning the result of the call to
> ip6_flowinfo() to .flowlabel. Assigning a constant makes the warning go away.

No complaints for the same initialization style at line 151?

Re: net: vrf: Handle ipv6 multicast and link-local addresses

[Geert Uytterhoeven]

On Wed, Aug 3, 2016 at 10:36 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
> On 8/3/16 2:27 PM, Geert Uytterhoeven wrote:
>> On Wed, Aug 3, 2016 at 10:11 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
>>> On 8/3/16 1:57 PM, Geert Uytterhoeven wrote:
>>>>> +static void vrf_ip6_input_dst(struct sk_buff *skb, struct net_device *vrf_dev,
>>>>>> +                             int ifindex)
>>>>>> +{
>>>>>> +       const struct ipv6hdr *iph = ipv6_hdr(skb);
>>>>>> +       struct flowi6 fl6 = {
>>>>>> +               .daddr          = iph->daddr,
>>>>>> +               .saddr          = iph->saddr,
>>>>>> +               .flowlabel      = ip6_flowinfo(iph),
>>>> The above assignment causes the following compiler warning with
>>>> m68k-linux-gnu-gcc-4.1:
>>>>
>>>>     drivers/net/vrf.c: In function ‘vrf_ip6_input_dst’:
>>>>     drivers/net/vrf.c:870: warning: initialized field with
>>>> side-effects overwritten
>>>>     drivers/net/vrf.c:870: warning: (near initialization for ‘fl6’)
>>>>
>>>> Unfortunately I have no idea what it means, nor do I see what's wrong
>>>> with the code.
>>>
>>> no idea. Fields are initialized once and left and right data types are the same.
>>>
>>> Can you remove one line at a time? Line 870 is ".flowi6_proto   = iph->nexthdr," but all of the flowi6 macros are unique references to unique fields in flowi_common. The flowlabel line you point out is a unique field as well.
>>
>> The only thing that seems to matter is assigning the result of the call to
>> ip6_flowinfo() to .flowlabel. Assigning a constant makes the warning go away.
>
> No complaints for the same initialization style at line 151?

No.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: net: vrf: Handle ipv6 multicast and link-local addresses

[Geert Uytterhoeven]

Hi David,

On Thu, Aug 4, 2016 at 7:01 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
> Does making the code the same between those 2 functions matter?

Yes, it does make the warning go away.

> diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
> index 1ce7420322ee..3951a2c98431 100644
> --- a/drivers/net/vrf.c
> +++ b/drivers/net/vrf.c
> @@ -862,15 +862,17 @@ static void vrf_ip6_input_dst(struct sk_buff *skb, struct net_device *vrf_dev,
>                               int ifindex)
>  {
>         const struct ipv6hdr *iph = ipv6_hdr(skb);
> +       struct net *net = dev_net(vrf_dev);
>         struct flowi6 fl6 = {
> +               .flowi6_oif     = 0,
> +               .flowi6_iif     = ifindex,
>                 .daddr          = iph->daddr,
>                 .saddr          = iph->saddr,
>                 .flowlabel      = ip6_flowinfo(iph),
>                 .flowi6_mark    = skb->mark,
>                 .flowi6_proto   = iph->nexthdr,
> -               .flowi6_iif     = ifindex,
> +               .flowi6_flags   = 0,
>         };
> -       struct net *net = dev_net(vrf_dev);
>         struct rt6_info *rt6;

The critical change seems to be moving the initialization of
.flowi6_iif: if that
is done before the initialization of .flowlabel, there's no compiler warning.

The generated asm output is identical, though, so I think this is some sort of
false positive or compiler bug. Hence let's ignore it.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: net: vrf: Handle ipv6 multicast and link-local addresses

[Arnaldo Carvalho de Melo]

Em Wed, Aug 03, 2016 at 10:27:44PM +0200, Geert Uytterhoeven escreveu:
> On Wed, Aug 3, 2016 at 10:11 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
> > On 8/3/16 1:57 PM, Geert Uytterhoeven wrote:
> >>> +static void vrf_ip6_input_dst(struct sk_buff *skb, struct net_device *vrf_dev,
> >>> > +                             int ifindex)
> >>> > +{
> >>> > +       const struct ipv6hdr *iph = ipv6_hdr(skb);
> >>> > +       struct flowi6 fl6 = {
> >>> > +               .daddr          = iph->daddr,
> >>> > +               .saddr          = iph->saddr,
> >>> > +               .flowlabel      = ip6_flowinfo(iph),
> >> The above assignment causes the following compiler warning with
> >> m68k-linux-gnu-gcc-4.1:
> >>
> >>     drivers/net/vrf.c: In function ‘vrf_ip6_input_dst’:
> >>     drivers/net/vrf.c:870: warning: initialized field with
> >> side-effects overwritten
> >>     drivers/net/vrf.c:870: warning: (near initialization for ‘fl6’)
> >>
> >> Unfortunately I have no idea what it means, nor do I see what's wrong
> >> with the code.
> >
> > no idea. Fields are initialized once and left and right data types are the same.
> >
> > Can you remove one line at a time? Line 870 is ".flowi6_proto   = iph->nexthdr," but all of the flowi6 macros are unique references to unique fields in flowi_common. The flowlabel line you point out is a unique field as well.
> 
> The only thing that seems to matter is assigning the result of the call to
> ip6_flowinfo() to .flowlabel. Assigning a constant makes the warning go away.
> 
> Yeah, the 870 line number is funny, as it doesn't point to the offending line.
> 
> > Can you run pahole on file that did compile? e.g.,
> >
> > pahole -C 'flowi6' net/ipv6/route.o
> >
> > and get the common struct too:
> >
> > pahole -C 'flowi_common' net/ipv6/route.o
> 
> No output. Perhaps pahole doesn't play well with cross-compiling?

Can you send me this net/ipv6/route.o file? It should work as long as
CONFIG_DEBUG_INFO was used to build this cross kernel.

For instance, here cross compiling perf tools for s390 from a ubuntu
16.04 container running on a fedora 24 host, I get:

[root@jouet 14.04.4]# file /var/lib/docker/devicemapper/mnt/b7ba22999d88feebc42de2ea17fe27e94febf228439fccf2b84c0c9e1ccd8af7/rootfs/tmp/build/perf/perf
/var/lib/docker/devicemapper/mnt/b7ba22999d88feebc42de2ea17fe27e94febf228439fccf2b84c0c9e1ccd8af7/rootfs/tmp/build/perf/perf: ELF 64-bit MSB shared object, IBM S/390, version 1 (SYSV), dynamically linked, interpreter /lib/ld64.so.1, for GNU/Linux 3.2.0, BuildID[sha1]=22b27009c61130ed5727bcaf9ad0813d414b8c5a, not stripped
[root@jouet 14.04.4]#

Thus a s/390 object, that I then process from a x86_64 pahole:

[root@jouet 14.04.4]# file /home/acme/git/pahole/build/pahole
/home/acme/git/pahole/build/pahole: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, BuildID[sha1]=c90855d4ffa815bf0dddb3463bc45aea4855d0e2, not stripped
[root@jouet 14.04.4]# 

Giving these results:

[root@jouet 14.04.4]# pahole -C sockaddr_in6 /var/lib/docker/devicemapper/mnt/b7ba22999d88feebc42de2ea17fe27e94febf228439fccf2b84c0c9e1ccd8af7/rootfs/tmp/build/perf/perf
struct sockaddr_in6 {
	sa_family_t                sin6_family;          /*     0     2 */
	in_port_t                  sin6_port;            /*     2     2 */
	uint32_t                   sin6_flowinfo;        /*     4     4 */
	struct in6_addr            sin6_addr;            /*     8    16 */
	uint32_t                   sin6_scope_id;        /*    24     4 */

	/* size: 28, cachelines: 1, members: 5 */
	/* last cacheline: 28 bytes */
};
[root@jouet 14.04.4]#

Expanding types:

[root@jouet 14.04.4]# pahole --expand_types -C sockaddr_in6 /var/lib/docker/devicemapper/mnt/b7ba22999d88feebc42de2ea17fe27e94febf228439fccf2b84c0c9e1ccd8af7/rootfs/tmp/build/perf/perf
struct sockaddr_in6 {
	/* typedef sa_family_t */ short unsigned int         sin6_family;                /*     0     2 */
	/* typedef in_port_t -> uint16_t */ short unsigned int         sin6_port;        /*     2     2 */
	/* typedef uint32_t */ unsigned int               sin6_flowinfo;                 /*     4     4 */
	struct in6_addr {
		union {
			/* typedef uint8_t */ unsigned char __u6_addr8[16];              /*          16 */
			/* typedef uint16_t */ short unsigned int __u6_addr16[8];        /*          16 */
			/* typedef uint32_t */ unsigned int __u6_addr32[4];              /*          16 */
		} __in6_u;                                                               /*     8    16 */
	} sin6_addr; /*     8    16 */
	/* typedef uint32_t */ unsigned int               sin6_scope_id;                 /*    24     4 */

	/* size: 28, cachelines: 1, members: 5 */
	/* last cacheline: 28 bytes */
};
[root@jouet 14.04.4]#

etc.

Guess I need to put together a m68k target for building the perf tools (and
test pahole & friends on such objects)... :-)

- Arnaldo