[net-next PATCH] net: rtnetlink: Enslave device before bringing it up

[net-next PATCH] net: rtnetlink: Enslave device before bringing it up

[Phil Sutter]

Unlike with bridges, one can't add an interface to a bond and set it up
at the same time:

| # ip link set dummy0 down
| # ip link set dummy0 master bond0 up
| Error: Device can not be enslaved while up.

Of all drivers with ndo_add_slave callback, bond and team decline if
IFF_UP flag is set, vrf cycles the interface (i.e., sets it down and
immediately up again) and the others just don't care.

Support the common notion of setting the interface up after enslaving it
by sorting the operations accordingly.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/core/rtnetlink.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index adcc045952c2f..d81acace02ce5 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2488,13 +2488,6 @@ static int do_setlink(const struct sk_buff *skb,
 		call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
 	}
 
-	if (ifm->ifi_flags || ifm->ifi_change) {
-		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)
@@ -2502,6 +2495,13 @@ static int do_setlink(const struct sk_buff *skb,
 		status |= DO_SETLINK_MODIFIED;
 	}
 
+	if (ifm->ifi_flags || ifm->ifi_change) {
+		err = dev_change_flags(dev, rtnl_dev_combine_flags(dev, ifm),
+				       extack);
+		if (err < 0)
+			goto errout;
+	}
+
 	if (tb[IFLA_CARRIER]) {
 		err = dev_change_carrier(dev, nla_get_u8(tb[IFLA_CARRIER]));
 		if (err)
-- 
2.21.0

Re: [net-next PATCH] net: rtnetlink: Enslave device before bringing it up

[David Ahern]

On 5/29/19 7:51 AM, Phil Sutter wrote:
> Unlike with bridges, one can't add an interface to a bond and set it up
> at the same time:
> 
> | # ip link set dummy0 down
> | # ip link set dummy0 master bond0 up
> | Error: Device can not be enslaved while up.
> 
> Of all drivers with ndo_add_slave callback, bond and team decline if
> IFF_UP flag is set, vrf cycles the interface (i.e., sets it down and
> immediately up again) and the others just don't care.
> 
> Support the common notion of setting the interface up after enslaving it
> by sorting the operations accordingly.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  net/core/rtnetlink.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 

I agree with the intent - enslave before up.

Not sure how likely this is, but it does break the case:
ip li set dummy0 master bond0 down

Re: [net-next PATCH] net: rtnetlink: Enslave device before bringing it up

[Phil Sutter]

On Wed, May 29, 2019 at 09:41:07AM -0600, David Ahern wrote:
> On 5/29/19 7:51 AM, Phil Sutter wrote:
> > Unlike with bridges, one can't add an interface to a bond and set it up
> > at the same time:
> > 
> > | # ip link set dummy0 down
> > | # ip link set dummy0 master bond0 up
> > | Error: Device can not be enslaved while up.
> > 
> > Of all drivers with ndo_add_slave callback, bond and team decline if
> > IFF_UP flag is set, vrf cycles the interface (i.e., sets it down and
> > immediately up again) and the others just don't care.
> > 
> > Support the common notion of setting the interface up after enslaving it
> > by sorting the operations accordingly.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  net/core/rtnetlink.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> 
> I agree with the intent - enslave before up.
> 
> Not sure how likely this is, but it does break the case:
> ip li set dummy0 master bond0 down

That's right. I could allow for that by ordering the enslave and state
change dynamically, but doubt it's worth the effort. Instead of the
above I'd rather expect 'ip l s d0 nomaster down' to be a more common
use-case (which is not affected by my patch).

Thanks, Phil

Re: [net-next PATCH] net: rtnetlink: Enslave device before bringing it up

[David Miller]

From: Phil Sutter <phil@nwl.cc>
Date: Wed, 29 May 2019 15:51:20 +0200

> Unlike with bridges, one can't add an interface to a bond and set it up
> at the same time:
> 
> | # ip link set dummy0 down
> | # ip link set dummy0 master bond0 up
> | Error: Device can not be enslaved while up.
> 
> Of all drivers with ndo_add_slave callback, bond and team decline if
> IFF_UP flag is set, vrf cycles the interface (i.e., sets it down and
> immediately up again) and the others just don't care.
> 
> Support the common notion of setting the interface up after enslaving it
> by sorting the operations accordingly.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

What about other flags like IFF_PROMISCUITY?

Re: [net-next PATCH] net: rtnetlink: Enslave device before bringing it up

[Phil Sutter]

Hi David,

On Fri, May 31, 2019 at 02:26:15PM -0700, David Miller wrote:
> From: Phil Sutter <phil@nwl.cc>
> Date: Wed, 29 May 2019 15:51:20 +0200
> 
> > Unlike with bridges, one can't add an interface to a bond and set it up
> > at the same time:
> > 
> > | # ip link set dummy0 down
> > | # ip link set dummy0 master bond0 up
> > | Error: Device can not be enslaved while up.
> > 
> > Of all drivers with ndo_add_slave callback, bond and team decline if
> > IFF_UP flag is set, vrf cycles the interface (i.e., sets it down and
> > immediately up again) and the others just don't care.
> > 
> > Support the common notion of setting the interface up after enslaving it
> > by sorting the operations accordingly.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> 
> What about other flags like IFF_PROMISCUITY?

Crap, that's the crux: Upon enslaving, team driver propagates
IFF_PROMISC and IFF_ALLMULTI flags from master to slave. With my change,
these propagations roll back by accident. So please disregard this
patch, I'll have to find a different solution.

Thanks, Phil