linux-kernel - Re: [PATCH net] ipv6: don't auto-add link-local address to lag ports

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:   Thu, 19 Mar 2020 12:52:23 -0700
From:   Jay Vosburgh <jay.vosburgh@...onical.com>
To:     Jarod Wilson <jarod@...hat.com>
cc:     Eric Dumazet <eric.dumazet@...il.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Moshe Levi <moshele@...lanox.com>,
        Marcelo Ricardo Leitner <mleitner@...hat.com>,
        Netdev <netdev@...r.kernel.org>,
        "K. Y. Srinivasan" <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>
Subject: Re: [PATCH net] ipv6: don't auto-add link-local address to lag ports

Jarod Wilson <jarod@...hat.com> wrote:

>On Thu, Mar 19, 2020 at 1:06 PM Eric Dumazet <eric.dumazet@...il.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.

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

>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

bc91b0f07ada ipv6: addrconf: implement address generation modes

	-J

---
	-Jay Vosburgh, jay.vosburgh@...onical.com