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

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

[Jarod Wilson]

Bonding slave and team port devices should not have link-local addresses
automatically added to them, as it can interfere with openvswitch being
able to properly add tc ingress.

Reported-by: Moshe Levi <moshele@mellanox.com>
CC: Marcelo Ricardo Leitner <mleitner@redhat.com>
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 net/ipv6/addrconf.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 46d614b611db..aed891695084 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -3296,6 +3296,10 @@ static void addrconf_addr_gen(struct inet6_dev *idev, bool prefix_route)
 	if (netif_is_l3_master(idev->dev))
 		return;
 
+	/* no link local addresses on bond slave or team port devices */
+	if (netif_is_lag_port(idev->dev))
+		return;
+
 	ipv6_addr_set(&addr, htonl(0xFE800000), 0, 0, 0);
 
 	switch (idev->cnf.addr_gen_mode) {
-- 
2.20.1

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

[Eric Dumazet]

On 3/18/20 7:06 AM, Jarod Wilson wrote:
> Bonding slave and team port devices should not have link-local addresses
> automatically added to them, as it can interfere with openvswitch being
> able to properly add tc ingress.
> 
> Reported-by: Moshe Levi <moshele@mellanox.com>
> CC: Marcelo Ricardo Leitner <mleitner@redhat.com>
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>


This does not look a net candidate to me, unless the bug has been added recently ?

The absence of Fixes: tag is a red flag for a net submission.

By adding a Fixes: tag, you are doing us a favor, please.

Thanks.

> ---
>  net/ipv6/addrconf.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 46d614b611db..aed891695084 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -3296,6 +3296,10 @@ static void addrconf_addr_gen(struct inet6_dev *idev, bool prefix_route)
>  	if (netif_is_l3_master(idev->dev))
>  		return;
>  
> +	/* no link local addresses on bond slave or team port devices */
> +	if (netif_is_lag_port(idev->dev))
> +		return;
> +
>  	ipv6_addr_set(&addr, htonl(0xFE800000), 0, 0, 0);
>  
>  	switch (idev->cnf.addr_gen_mode) {
> 

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

[Jarod Wilson]

On Wed, Mar 18, 2020 at 2:02 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> On 3/18/20 7:06 AM, Jarod Wilson wrote:
> > Bonding slave and team port devices should not have link-local addresses
> > automatically added to them, as it can interfere with openvswitch being
> > able to properly add tc ingress.
> >
> > Reported-by: Moshe Levi <moshele@mellanox.com>
> > CC: Marcelo Ricardo Leitner <mleitner@redhat.com>
> > CC: netdev@vger.kernel.org
> > Signed-off-by: Jarod Wilson <jarod@redhat.com>
>
>
> This does not look a net candidate to me, unless the bug has been added recently ?
>
> The absence of Fixes: tag is a red flag for a net submission.
>
> By adding a Fixes: tag, you are doing us a favor, please.

Yeah, wasn't entirely sure on this one. It fixes a problem, but it's
not exactly a new one. A quick look at git history suggests this might
actually be something that technically pre-dates the move to git in
2005, but only really became a problem with some additional far more
recent infrastructure (tc and friends). I can resubmit it as net-next
if that's preferred.

-- 
Jarod Wilson
jarod@redhat.com

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

[Jay Vosburgh]

Jarod Wilson <jarod@redhat.com> wrote:

>On Wed, Mar 18, 2020 at 2:02 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>> On 3/18/20 7:06 AM, Jarod Wilson wrote:
>> > Bonding slave and team port devices should not have link-local addresses
>> > automatically added to them, as it can interfere with openvswitch being
>> > able to properly add tc ingress.
>> >
>> > Reported-by: Moshe Levi <moshele@mellanox.com>
>> > CC: Marcelo Ricardo Leitner <mleitner@redhat.com>
>> > CC: netdev@vger.kernel.org
>> > Signed-off-by: Jarod Wilson <jarod@redhat.com>
>>
>>
>> This does not look a net candidate to me, unless the bug has been added recently ?
>>
>> The absence of Fixes: tag is a red flag for a net submission.
>>
>> By adding a Fixes: tag, you are doing us a favor, please.
>
>Yeah, wasn't entirely sure on this one. It fixes a problem, but it's
>not exactly a new one. A quick look at git history suggests this might
>actually be something that technically pre-dates the move to git in
>2005, but only really became a problem with some additional far more
>recent infrastructure (tc and friends). I can resubmit it as net-next
>if that's preferred.

	Commit

c2edacf80e15 bonding / ipv6: no addrconf for slaves separately from master

	should (in theory) already prevent ipv6 link-local addrconf, at
least for bonding slaves, and dates from 2007.  If something has changed
to break the logic in this commit, then (a) you might need to do some
research to find a candidate for your Fixes tag, and (b) I'd suggest
also investigating whether or not the change added by c2edacf80e15 to
addrconf_notify() no longer serves any purpose, and should be removed if
that is the case.

	Note also that the hyperv netvsc driver, in netvsc_vf_join(),
sets IFF_SLAVE in order to trigger the addrconf prevention logic from
c2edacf80e15; I'm not sure if your patch would affect its expectations
(if c2edacf80e15 were removed).

	-J

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

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

[Jarod Wilson]

On Wed, Mar 18, 2020 at 4:42 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>
> Jarod Wilson <jarod@redhat.com> wrote:
>
> >On Wed, Mar 18, 2020 at 2:02 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >>
> >> On 3/18/20 7:06 AM, Jarod Wilson wrote:
> >> > Bonding slave and team port devices should not have link-local addresses
> >> > automatically added to them, as it can interfere with openvswitch being
> >> > able to properly add tc ingress.
> >> >
> >> > Reported-by: Moshe Levi <moshele@mellanox.com>
> >> > CC: Marcelo Ricardo Leitner <mleitner@redhat.com>
> >> > CC: netdev@vger.kernel.org
> >> > Signed-off-by: Jarod Wilson <jarod@redhat.com>
> >>
> >>
> >> This does not look a net candidate to me, unless the bug has been added recently ?
> >>
> >> The absence of Fixes: tag is a red flag for a net submission.
> >>
> >> By adding a Fixes: tag, you are doing us a favor, please.
> >
> >Yeah, wasn't entirely sure on this one. It fixes a problem, but it's
> >not exactly a new one. A quick look at git history suggests this might
> >actually be something that technically pre-dates the move to git in
> >2005, but only really became a problem with some additional far more
> >recent infrastructure (tc and friends). I can resubmit it as net-next
> >if that's preferred.
>
>         Commit
>
> c2edacf80e15 bonding / ipv6: no addrconf for slaves separately from master
>
>         should (in theory) already prevent ipv6 link-local addrconf, at
> least for bonding slaves, and dates from 2007.  If something has changed
> to break the logic in this commit, then (a) you might need to do some
> research to find a candidate for your Fixes tag, and (b) I'd suggest
> also investigating whether or not the change added by c2edacf80e15 to
> addrconf_notify() no longer serves any purpose, and should be removed if
> that is the case.
>
>         Note also that the hyperv netvsc driver, in netvsc_vf_join(),
> sets IFF_SLAVE in order to trigger the addrconf prevention logic from
> c2edacf80e15; I'm not sure if your patch would affect its expectations
> (if c2edacf80e15 were removed).

Interesting. We'll keep digging over here, but that's definitely not
working for this particular use case with OVS for whatever reason.

-- 
Jarod Wilson
jarod@redhat.com

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

[Eric Dumazet]

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 :)

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

[Jarod Wilson]

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.

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.

-- 
Jarod Wilson
jarod@redhat.com

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

[Jay Vosburgh]

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.

	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@canonical.com

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

[Jarod Wilson]

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

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

[Stephen Hemminger]

On Thu, 19 Mar 2020 15:29:51 -0400
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.
> 
> 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.
> 

Yes the addrgen mode patches caused bad things to happen with hyper-v
sub devices.  Addrconf code is very tricky to get right.
If you look back there have been a large number of changes where
a patch looks good, gets reviewed, merged, and then breaks something
and has to be reverted.

Probably the original patch should just be reverted rather than
trying to add more here.

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

[Jarod Wilson]

On Thu, Mar 19, 2020 at 6:41 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Thu, 19 Mar 2020 15:29:51 -0400
> 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.
> >
> > 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.
> >
>
> Yes the addrgen mode patches caused bad things to happen with hyper-v
> sub devices.  Addrconf code is very tricky to get right.
> If you look back there have been a large number of changes where
> a patch looks good, gets reviewed, merged, and then breaks something
> and has to be reverted.
>
> Probably the original patch should just be reverted rather than
> trying to add more here.

I'm not prepared to do a full revert here myself, I don't know the
code well enough, or what the ramifications might be. For v2, I was
just going to propose a check-and-bail for devices with IFF_SLAVE set
in addrconf_addr_gen(), to hopefully catch all the same devices the
existing check from c2edacf80e15 caught, should they take this code
pathway that skips that check.

-- 
Jarod Wilson
jarod@redhat.com

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

[Jarod Wilson]

Bonding slave and team port devices should not have link-local addresses
automatically added to them, as it can interfere with openvswitch being
able to properly add tc ingress.

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 added by commit c2edacf80e15, and
results in an address getting added, while w/the proposed patch added,
no address gets added. This simply adds the same gating check to another
code path, and thus should prevent the same devices from erroneously
obtaining an ipv6 link-local address.

Fixes: d35a00b8e33d ("net/ipv6: allow sysctl to change link-local address generation mode")
Reported-by: Moshe Levi <moshele@mellanox.com>
CC: Stephen Hemminger <stephen@networkplumber.org>
CC: Marcelo Ricardo Leitner <mleitner@redhat.com>
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 net/ipv6/addrconf.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 46d614b611db..2a8175de8578 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -3296,6 +3296,10 @@ static void addrconf_addr_gen(struct inet6_dev *idev, bool prefix_route)
 	if (netif_is_l3_master(idev->dev))
 		return;
 
+	/* no link local addresses on devices flagged as slaves */
+	if (idev->dev->flags & IFF_SLAVE)
+		return;
+
 	ipv6_addr_set(&addr, htonl(0xFE800000), 0, 0, 0);
 
 	switch (idev->cnf.addr_gen_mode) {
-- 
2.20.1

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

[David Miller]

From: Jarod Wilson <jarod@redhat.com>
Date: Mon, 30 Mar 2020 11:22:19 -0400

> Bonding slave and team port devices should not have link-local addresses
> automatically added to them, as it can interfere with openvswitch being
> able to properly add tc ingress.
> 
> Basic reproducer, courtesy of Marcelo:
 ...
> (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 added by commit c2edacf80e15, and
> results in an address getting added, while w/the proposed patch added,
> no address gets added. This simply adds the same gating check to another
> code path, and thus should prevent the same devices from erroneously
> obtaining an ipv6 link-local address.
> 
> Fixes: d35a00b8e33d ("net/ipv6: allow sysctl to change link-local address generation mode")
> Reported-by: Moshe Levi <moshele@mellanox.com>
> CC: Stephen Hemminger <stephen@networkplumber.org>
> CC: Marcelo Ricardo Leitner <mleitner@redhat.com>
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>

Applied and queued up for -stable, thanks.