* [PATCH net v3 1/2] rtnetlink: allow to set iface down before enslaving it [not found] <20240104164300.3870209-1-nicolas.dichtel@6wind.com> @ 2024-01-04 16:42 ` Nicolas Dichtel 2024-01-05 11:59 ` Jiri Pirko 0 siblings, 1 reply; 5+ messages in thread From: Nicolas Dichtel @ 2024-01-04 16:42 UTC (permalink / raw) To: David S . Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet, Phil Sutter, David Ahern Cc: netdev, Nicolas Dichtel, stable The below commit adds support for: > ip link set dummy0 down > ip link set dummy0 master bond0 up but breaks the opposite: > ip link set dummy0 up > ip link set dummy0 master bond0 down Let's add a workaround to have both commands working. Cc: stable@vger.kernel.org Fixes: a4abfa627c38 ("net: rtnetlink: Enslave device before bringing it up") Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> Acked-by: Phil Sutter <phil@nwl.cc> Reviewed-by: David Ahern <dsahern@kernel.org> --- net/core/rtnetlink.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index e8431c6c8490..dd79693c2d91 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -2905,6 +2905,14 @@ static int do_setlink(const struct sk_buff *skb, call_netdevice_notifiers(NETDEV_CHANGEADDR, dev); } + /* Backward compat: enable to set interface down before enslaving it */ + if (!(ifm->ifi_flags & IFF_UP) && ifm->ifi_change & IFF_UP) { + err = dev_change_flags(dev, rtnl_dev_combine_flags(dev, ifm), + extack); + if (err < 0) + goto errout; + } + if (tb[IFLA_MASTER]) { err = do_set_master(dev, nla_get_u32(tb[IFLA_MASTER]), extack); if (err) -- 2.39.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net v3 1/2] rtnetlink: allow to set iface down before enslaving it 2024-01-04 16:42 ` [PATCH net v3 1/2] rtnetlink: allow to set iface down before enslaving it Nicolas Dichtel @ 2024-01-05 11:59 ` Jiri Pirko 2024-01-05 16:22 ` Nicolas Dichtel 2024-01-06 3:32 ` Phil Sutter 0 siblings, 2 replies; 5+ messages in thread From: Jiri Pirko @ 2024-01-05 11:59 UTC (permalink / raw) To: Nicolas Dichtel Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet, Phil Sutter, David Ahern, netdev, stable Thu, Jan 04, 2024 at 05:42:59PM CET, nicolas.dichtel@6wind.com wrote: >The below commit adds support for: >> ip link set dummy0 down >> ip link set dummy0 master bond0 up > >but breaks the opposite: >> ip link set dummy0 up >> ip link set dummy0 master bond0 down It is a bit weird to see these 2 and assume some ordering. The first one assumes: dummy0 master bond 0, dummy0 up The second one assumes: dummy0 down, dummy0 master bond 0 But why? What is the practival reason for a4abfa627c38 existence? I mean, bond/team bring up the device themselfs when needed. Phil? Wouldn't simple revert do better job here? > >Let's add a workaround to have both commands working. > >Cc: stable@vger.kernel.org >Fixes: a4abfa627c38 ("net: rtnetlink: Enslave device before bringing it up") >Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> >Acked-by: Phil Sutter <phil@nwl.cc> >Reviewed-by: David Ahern <dsahern@kernel.org> >--- > net/core/rtnetlink.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > >diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c >index e8431c6c8490..dd79693c2d91 100644 >--- a/net/core/rtnetlink.c >+++ b/net/core/rtnetlink.c >@@ -2905,6 +2905,14 @@ static int do_setlink(const struct sk_buff *skb, > call_netdevice_notifiers(NETDEV_CHANGEADDR, dev); > } > >+ /* Backward compat: enable to set interface down before enslaving it */ >+ if (!(ifm->ifi_flags & IFF_UP) && ifm->ifi_change & IFF_UP) { >+ err = dev_change_flags(dev, rtnl_dev_combine_flags(dev, ifm), >+ extack); >+ if (err < 0) >+ goto errout; >+ } >+ > if (tb[IFLA_MASTER]) { > err = do_set_master(dev, nla_get_u32(tb[IFLA_MASTER]), extack); > if (err) >-- >2.39.2 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v3 1/2] rtnetlink: allow to set iface down before enslaving it 2024-01-05 11:59 ` Jiri Pirko @ 2024-01-05 16:22 ` Nicolas Dichtel 2024-01-06 3:32 ` Phil Sutter 1 sibling, 0 replies; 5+ messages in thread From: Nicolas Dichtel @ 2024-01-05 16:22 UTC (permalink / raw) To: Jiri Pirko Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet, Phil Sutter, David Ahern, netdev, stable Le 05/01/2024 à 12:59, Jiri Pirko a écrit : > Thu, Jan 04, 2024 at 05:42:59PM CET, nicolas.dichtel@6wind.com wrote: >> The below commit adds support for: >>> ip link set dummy0 down >>> ip link set dummy0 master bond0 up >> >> but breaks the opposite: >>> ip link set dummy0 up >>> ip link set dummy0 master bond0 down > > It is a bit weird to see these 2 and assume some ordering. > The first one assumes: > dummy0 master bond 0, dummy0 up > The second one assumes: > dummy0 down, dummy0 master bond 0 > But why? > > What is the practival reason for a4abfa627c38 existence? I mean, > bond/team bring up the device themselfs when needed. Phil? > Wouldn't simple revert do better job here? Yeah, I assumed there was a good reason, but you're right. > > >> >> Let's add a workaround to have both commands working. >> >> Cc: stable@vger.kernel.org >> Fixes: a4abfa627c38 ("net: rtnetlink: Enslave device before bringing it up") >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> >> Acked-by: Phil Sutter <phil@nwl.cc> >> Reviewed-by: David Ahern <dsahern@kernel.org> >> --- >> net/core/rtnetlink.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c >> index e8431c6c8490..dd79693c2d91 100644 >> --- a/net/core/rtnetlink.c >> +++ b/net/core/rtnetlink.c >> @@ -2905,6 +2905,14 @@ static int do_setlink(const struct sk_buff *skb, >> call_netdevice_notifiers(NETDEV_CHANGEADDR, dev); >> } >> >> + /* Backward compat: enable to set interface down before enslaving it */ >> + if (!(ifm->ifi_flags & IFF_UP) && ifm->ifi_change & IFF_UP) { >> + err = dev_change_flags(dev, rtnl_dev_combine_flags(dev, ifm), >> + extack); >> + if (err < 0) >> + goto errout; >> + } >> + >> if (tb[IFLA_MASTER]) { >> err = do_set_master(dev, nla_get_u32(tb[IFLA_MASTER]), extack); >> if (err) >> -- >> 2.39.2 >> >> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v3 1/2] rtnetlink: allow to set iface down before enslaving it 2024-01-05 11:59 ` Jiri Pirko 2024-01-05 16:22 ` Nicolas Dichtel @ 2024-01-06 3:32 ` Phil Sutter 2024-01-06 11:08 ` Jiri Pirko 1 sibling, 1 reply; 5+ messages in thread From: Phil Sutter @ 2024-01-06 3:32 UTC (permalink / raw) To: Jiri Pirko Cc: Nicolas Dichtel, David S . Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet, David Ahern, netdev, stable Hi, On Fri, Jan 05, 2024 at 12:59:24PM +0100, Jiri Pirko wrote: > Thu, Jan 04, 2024 at 05:42:59PM CET, nicolas.dichtel@6wind.com wrote: > >The below commit adds support for: > >> ip link set dummy0 down > >> ip link set dummy0 master bond0 up > > > >but breaks the opposite: > >> ip link set dummy0 up > >> ip link set dummy0 master bond0 down > > It is a bit weird to see these 2 and assume some ordering. > The first one assumes: > dummy0 master bond 0, dummy0 up > The second one assumes: > dummy0 down, dummy0 master bond 0 > But why? > > What is the practival reason for a4abfa627c38 existence? I mean, > bond/team bring up the device themselfs when needed. Phil? > Wouldn't simple revert do better job here? Ah, I wasn't aware bond master manipulates slaves' links itself and thus treated all types' link master setting the same by setting the slave up afterwards. This is basically what a4abfa627c38 is good for: Enabling 'ip link set X master Y up' regardless of Y's link type. If setting a bond slave up manually is not recommended, the easiest solution is probbaly indeed to revert a4abfa627c38 and live with the quirk in bond driver. Cheers, Phil ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v3 1/2] rtnetlink: allow to set iface down before enslaving it 2024-01-06 3:32 ` Phil Sutter @ 2024-01-06 11:08 ` Jiri Pirko 0 siblings, 0 replies; 5+ messages in thread From: Jiri Pirko @ 2024-01-06 11:08 UTC (permalink / raw) To: Phil Sutter, Nicolas Dichtel, David S . Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet, David Ahern, netdev, stable Sat, Jan 06, 2024 at 04:32:12AM CET, phil@nwl.cc wrote: >Hi, > >On Fri, Jan 05, 2024 at 12:59:24PM +0100, Jiri Pirko wrote: >> Thu, Jan 04, 2024 at 05:42:59PM CET, nicolas.dichtel@6wind.com wrote: >> >The below commit adds support for: >> >> ip link set dummy0 down >> >> ip link set dummy0 master bond0 up >> > >> >but breaks the opposite: >> >> ip link set dummy0 up >> >> ip link set dummy0 master bond0 down >> >> It is a bit weird to see these 2 and assume some ordering. >> The first one assumes: >> dummy0 master bond 0, dummy0 up >> The second one assumes: >> dummy0 down, dummy0 master bond 0 >> But why? >> >> What is the practival reason for a4abfa627c38 existence? I mean, >> bond/team bring up the device themselfs when needed. Phil? >> Wouldn't simple revert do better job here? > >Ah, I wasn't aware bond master manipulates slaves' links itself and thus >treated all types' link master setting the same by setting the slave up >afterwards. This is basically what a4abfa627c38 is good for: Enabling >'ip link set X master Y up' regardless of Y's link type. > >If setting a bond slave up manually is not recommended, the easiest >solution is probbaly indeed to revert a4abfa627c38 and live with the >quirk in bond driver. Okay, lets revert then. > >Cheers, Phil ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-01-06 11:08 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20240104164300.3870209-1-nicolas.dichtel@6wind.com>
2024-01-04 16:42 ` [PATCH net v3 1/2] rtnetlink: allow to set iface down before enslaving it Nicolas Dichtel
2024-01-05 11:59 ` Jiri Pirko
2024-01-05 16:22 ` Nicolas Dichtel
2024-01-06 3:32 ` Phil Sutter
2024-01-06 11:08 ` Jiri Pirko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox