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@xxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, 19 Mar 2020 15:29:51 -0400
> Jarod Wilson <jarod@xxxxxxxxxx> wrote:
>
> > On Thu, Mar 19, 2020 at 1:06 PM Eric Dumazet <eric.dumazet@xxxxxxxxx> 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@xxxxxxxxxx