* [PATCH net] ipv6: don't auto-add link-local address to lag ports @ 2020-03-18 14:06 Jarod Wilson 2020-03-18 18:02 ` Eric Dumazet 0 siblings, 1 reply; 13+ messages in thread From: Jarod Wilson @ 2020-03-18 14:06 UTC (permalink / raw) To: linux-kernel; +Cc: Jarod Wilson, Moshe Levi, Marcelo Ricardo Leitner, netdev 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH net] ipv6: don't auto-add link-local address to lag ports 2020-03-18 14:06 [PATCH net] ipv6: don't auto-add link-local address to lag ports Jarod Wilson @ 2020-03-18 18:02 ` Eric Dumazet 2020-03-18 18:32 ` Jarod Wilson 0 siblings, 1 reply; 13+ messages in thread From: Eric Dumazet @ 2020-03-18 18:02 UTC (permalink / raw) To: Jarod Wilson, linux-kernel; +Cc: Moshe Levi, Marcelo Ricardo Leitner, netdev 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) { > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] ipv6: don't auto-add link-local address to lag ports 2020-03-18 18:02 ` Eric Dumazet @ 2020-03-18 18:32 ` Jarod Wilson 2020-03-18 20:41 ` Jay Vosburgh 0 siblings, 1 reply; 13+ messages in thread From: Jarod Wilson @ 2020-03-18 18:32 UTC (permalink / raw) To: Eric Dumazet; +Cc: LKML, Moshe Levi, Marcelo Ricardo Leitner, Netdev 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] ipv6: don't auto-add link-local address to lag ports 2020-03-18 18:32 ` Jarod Wilson @ 2020-03-18 20:41 ` Jay Vosburgh 2020-03-19 16:42 ` Jarod Wilson 0 siblings, 1 reply; 13+ messages in thread From: Jay Vosburgh @ 2020-03-18 20:41 UTC (permalink / raw) To: Jarod Wilson Cc: Eric Dumazet, LKML, Moshe Levi, Marcelo Ricardo Leitner, Netdev 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] ipv6: don't auto-add link-local address to lag ports 2020-03-18 20:41 ` Jay Vosburgh @ 2020-03-19 16:42 ` Jarod Wilson 2020-03-19 17:05 ` Eric Dumazet 0 siblings, 1 reply; 13+ messages in thread From: Jarod Wilson @ 2020-03-19 16:42 UTC (permalink / raw) To: Jay Vosburgh Cc: Eric Dumazet, LKML, Moshe Levi, Marcelo Ricardo Leitner, Netdev 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] ipv6: don't auto-add link-local address to lag ports 2020-03-19 16:42 ` Jarod Wilson @ 2020-03-19 17:05 ` Eric Dumazet 2020-03-19 19:29 ` Jarod Wilson 0 siblings, 1 reply; 13+ messages in thread From: Eric Dumazet @ 2020-03-19 17:05 UTC (permalink / raw) To: Jarod Wilson, Jay Vosburgh Cc: LKML, Moshe Levi, Marcelo Ricardo Leitner, Netdev 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 :) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] ipv6: don't auto-add link-local address to lag ports 2020-03-19 17:05 ` Eric Dumazet @ 2020-03-19 19:29 ` Jarod Wilson 2020-03-19 19:52 ` Jay Vosburgh 2020-03-19 22:41 ` Stephen Hemminger 0 siblings, 2 replies; 13+ messages in thread From: Jarod Wilson @ 2020-03-19 19:29 UTC (permalink / raw) To: Eric Dumazet Cc: Jay Vosburgh, LKML, Moshe Levi, Marcelo Ricardo Leitner, Netdev 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] ipv6: don't auto-add link-local address to lag ports 2020-03-19 19:29 ` Jarod Wilson @ 2020-03-19 19:52 ` Jay Vosburgh 2020-03-19 21:53 ` Jarod Wilson 2020-03-19 22:41 ` Stephen Hemminger 1 sibling, 1 reply; 13+ messages in thread From: Jay Vosburgh @ 2020-03-19 19:52 UTC (permalink / raw) To: Jarod Wilson Cc: Eric Dumazet, LKML, Moshe Levi, Marcelo Ricardo Leitner, Netdev, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] ipv6: don't auto-add link-local address to lag ports 2020-03-19 19:52 ` Jay Vosburgh @ 2020-03-19 21:53 ` Jarod Wilson 0 siblings, 0 replies; 13+ messages in thread From: Jarod Wilson @ 2020-03-19 21:53 UTC (permalink / raw) To: Jay Vosburgh Cc: Eric Dumazet, LKML, Moshe Levi, Marcelo Ricardo Leitner, Netdev, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] ipv6: don't auto-add link-local address to lag ports 2020-03-19 19:29 ` Jarod Wilson 2020-03-19 19:52 ` Jay Vosburgh @ 2020-03-19 22:41 ` Stephen Hemminger 2020-03-23 17:25 ` Jarod Wilson 1 sibling, 1 reply; 13+ messages in thread From: Stephen Hemminger @ 2020-03-19 22:41 UTC (permalink / raw) To: Jarod Wilson Cc: Eric Dumazet, Jay Vosburgh, LKML, Moshe Levi, Marcelo Ricardo Leitner, Netdev 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net] ipv6: don't auto-add link-local address to lag ports 2020-03-19 22:41 ` Stephen Hemminger @ 2020-03-23 17:25 ` Jarod Wilson 2020-03-30 15:22 ` [PATCH net v2] " Jarod Wilson 0 siblings, 1 reply; 13+ messages in thread From: Jarod Wilson @ 2020-03-23 17:25 UTC (permalink / raw) To: Stephen Hemminger Cc: Eric Dumazet, Jay Vosburgh, LKML, Moshe Levi, Marcelo Ricardo Leitner, Netdev 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net v2] ipv6: don't auto-add link-local address to lag ports 2020-03-23 17:25 ` Jarod Wilson @ 2020-03-30 15:22 ` Jarod Wilson 2020-04-01 18:14 ` David Miller 0 siblings, 1 reply; 13+ messages in thread From: Jarod Wilson @ 2020-03-30 15:22 UTC (permalink / raw) To: linux-kernel Cc: Jarod Wilson, Moshe Levi, Stephen Hemminger, Marcelo Ricardo Leitner, netdev 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH net v2] ipv6: don't auto-add link-local address to lag ports 2020-03-30 15:22 ` [PATCH net v2] " Jarod Wilson @ 2020-04-01 18:14 ` David Miller 0 siblings, 0 replies; 13+ messages in thread From: David Miller @ 2020-04-01 18:14 UTC (permalink / raw) To: jarod; +Cc: linux-kernel, moshele, stephen, mleitner, netdev 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-04-01 18:14 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-18 14:06 [PATCH net] ipv6: don't auto-add link-local address to lag ports Jarod Wilson 2020-03-18 18:02 ` Eric Dumazet 2020-03-18 18:32 ` Jarod Wilson 2020-03-18 20:41 ` Jay Vosburgh 2020-03-19 16:42 ` Jarod Wilson 2020-03-19 17:05 ` Eric Dumazet 2020-03-19 19:29 ` Jarod Wilson 2020-03-19 19:52 ` Jay Vosburgh 2020-03-19 21:53 ` Jarod Wilson 2020-03-19 22:41 ` Stephen Hemminger 2020-03-23 17:25 ` Jarod Wilson 2020-03-30 15:22 ` [PATCH net v2] " Jarod Wilson 2020-04-01 18:14 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.