Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200318140605.45273-1-jarod@redhat.com> <8a88d1c8-c6b1-ad85-7971-e6ae8c6fa0e4@gmail.com>
 <CAKfmpSc0yea5-OfE1rnVdErDTeOza=owbL00QQEaH-M-A6Za7g@mail.gmail.com>
 <25629.1584564113@famine> <CAKfmpScbzEZAEw=zOEwguQJvr6L2fQiGmAY60SqSBQ_g-+B4tw@mail.gmail.com>
 <3dbabf42-90e6-4c82-0b84-d1b1a9e8fadf@gmail.com> <CAKfmpScXTnnz6wQK3OZcqw4aM1PaLnBRfQL769JgyR7tgM-u5A@mail.gmail.com>
 <7028.1584647543@famine>
In-Reply-To: <7028.1584647543@famine>
From:   Jarod Wilson <jarod@redhat.com>
Date:   Thu, 19 Mar 2020 17:53:03 -0400
Message-ID: <CAKfmpSfw0xvmOKYA+VDdTP4GM=uxWsrYQ7ywHufa+KDrLEvf_Q@mail.gmail.com>
Subject: Re: [PATCH net] ipv6: don't auto-add link-local address to lag ports
To:     Jay Vosburgh <jay.vosburgh@canonical.com>
Cc:     Eric Dumazet <eric.dumazet@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Moshe Levi <moshele@mellanox.com>,
        Marcelo Ricardo Leitner <mleitner@redhat.com>,
        Netdev <netdev@vger.kernel.org>,
        "K. Y. Srinivasan" <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Stephen Hemminger <sthemmin@microsoft.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Mar 19, 2020 at 3:52 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>
> Jarod Wilson <jarod@redhat.com> wrote:
>
> >On Thu, Mar 19, 2020 at 1:06 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >>
> >> On 3/19/20 9:42 AM, Jarod Wilson wrote:
> >>
> >> > Interesting. We'll keep digging over here, but that's definitely not
> >> > working for this particular use case with OVS for whatever reason.
> >>
> >> I did a quick test and confirmed that my bonding slaves do not have link-local addresses,
> >> without anything done to prevent them to appear.
> >>
> >> You might add a selftest, if you ever find what is the trigger :)
> >
> >Okay, have a basic reproducer, courtesy of Marcelo:
> >
> ># ip link add name bond0 type bond
> ># ip link set dev ens2f0np0 master bond0
> ># ip link set dev ens2f1np2 master bond0
> ># ip link set dev bond0 up
> ># ip a s
> >1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN
> >group default qlen 1000
> >    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
> >    inet 127.0.0.1/8 scope host lo
> >       valid_lft forever preferred_lft forever
> >    inet6 ::1/128 scope host
> >       valid_lft forever preferred_lft forever
> >2: ens2f0np0: <BROADCAST,MULTICAST,SLAVE,UP,LOWER_UP> mtu 1500 qdisc
> >mq master bond0 state UP group default qlen 1000
> >    link/ether 00:0f:53:2f:ea:40 brd ff:ff:ff:ff:ff:ff
> >5: ens2f1np2: <NO-CARRIER,BROADCAST,MULTICAST,SLAVE,UP> mtu 1500 qdisc
> >mq master bond0 state DOWN group default qlen 1000
> >    link/ether 00:0f:53:2f:ea:40 brd ff:ff:ff:ff:ff:ff
> >11: bond0: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 qdisc
> >noqueue state UP group default qlen 1000
> >    link/ether 00:0f:53:2f:ea:40 brd ff:ff:ff:ff:ff:ff
> >    inet6 fe80::20f:53ff:fe2f:ea40/64 scope link
> >       valid_lft forever preferred_lft forever
> >
> >(above trimmed to relevant entries, obviously)
> >
> ># sysctl net.ipv6.conf.ens2f0np0.addr_gen_mode=0
> >net.ipv6.conf.ens2f0np0.addr_gen_mode = 0
> ># sysctl net.ipv6.conf.ens2f1np2.addr_gen_mode=0
> >net.ipv6.conf.ens2f1np2.addr_gen_mode = 0
> >
> ># ip a l ens2f0np0
> >2: ens2f0np0: <BROADCAST,MULTICAST,SLAVE,UP,LOWER_UP> mtu 1500 qdisc
> >mq master bond0 state UP group default qlen 1000
> >    link/ether 00:0f:53:2f:ea:40 brd ff:ff:ff:ff:ff:ff
> >    inet6 fe80::20f:53ff:fe2f:ea40/64 scope link tentative
> >       valid_lft forever preferred_lft forever
> ># ip a l ens2f1np2
> >5: ens2f1np2: <NO-CARRIER,BROADCAST,MULTICAST,SLAVE,UP> mtu 1500 qdisc
> >mq master bond0 state DOWN group default qlen 1000
> >    link/ether 00:0f:53:2f:ea:40 brd ff:ff:ff:ff:ff:ff
> >    inet6 fe80::20f:53ff:fe2f:ea40/64 scope link tentative
> >       valid_lft forever preferred_lft forever
> >
> >Looks like addrconf_sysctl_addr_gen_mode() bypasses the original "is
> >this a slave interface?" check, and results in an address getting
> >added, while w/the proposed patch added, no address gets added.
>
>         I wonder if this also breaks for the netvsc usage of IFF_SLAVE
> to suppress ipv6 addrconf?  Adding the hyperv maintainers to Cc.
>
>         In any event, it looks like addrconf_sysctl_addr_gen_mode()
> calls addrconf_dev_config() directly, which bypasses the IFF_SLAVE check
> in addrconf_notify() that would gate other callers.

Yeah, that's what I was thinking as well.

>         From my reading, your patch appears to cover a superset of cases
> as compared to the existing IFF_SLAVE test from c2edacf80e15.

I wasn't aware of additional devices that would want to prevent these
addresses, could certainly alter the patch to reject anything with
IFF_SLAVE too for consistency.

> >Looking back through git history again, I see a bunch of 'Fixes:
> >d35a00b8e33d ("net/ipv6: allow sysctl to change link-local address
> >generation mode")' patches, and I guess that's where this issue was
> >also introduced.
>
>         Can the problem be induced via ip link set ... addrgenmode ?
> That functionality predates the sysctl interface, looks like it was
> introduced with

Doesn't look like it, no. No change after either trying addrgenmode
none or random.

-- 
Jarod Wilson
jarod@redhat.com