[PATCH RFC] netns: keep vlan slaves on master netns move

[PATCH RFC] netns: keep vlan slaves on master netns move

[David Lamparter]

Hi everyone,

attached patch makes it possible to keep macvlan / 802.1q slave devices
on moving their master to a different namespace. as the opposite works
without problems (moving the slaves into a different namespace), this
shouldn't cause problems either.

i've tested this with 802.1q on real ethernet devs and bridges and,
well, it works without crashing, but that obviously doesn't mean it is
correct :) - therefore input very welcome.

RFC,

David

P.S.: who do i Cc: on netns related stuff?

--
previously, if a vlan master device was moved from one network namespace
to another, all 802.1q and macvlan slaves were deleted.

we can use dev->reg_state to figure out whether dev_change_net_namespace
is happening, since that won't set dev->reg_state NETREG_UNREGISTERING.
so, this changes 8021q and macvlan to ignore NETDEV_UNREGISTER when
reg_state is not NETREG_UNREGISTERING.

Signed-off-by: David Lamparter <equinox@diac24.net>
---
 drivers/net/macvlan.c |    4 ++++
 net/8021q/vlan.c      |    4 ++++
 net/core/dev.c        |    4 ++++
 3 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index f15fe2c..f43f8e4 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -754,6 +754,10 @@ static int macvlan_device_event(struct notifier_block *unused,
 		}
 		break;
 	case NETDEV_UNREGISTER:
+		/* twiddle thumbs on netns device moves */
+		if (dev->reg_state != NETREG_UNREGISTERING)
+			break;
+
 		list_for_each_entry_safe(vlan, next, &port->vlans, list)
 			vlan->dev->rtnl_link_ops->dellink(vlan->dev, NULL);
 		break;
diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 3c1c8c1..cdc37e8 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -516,6 +516,10 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 		break;
 
 	case NETDEV_UNREGISTER:
+		/* twiddle thumbs on netns device moves */
+		if (dev->reg_state != NETREG_UNREGISTERING)
+			break;
+
 		/* Delete all VLANs for this dev. */
 		grp->killall = 1;
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 1f466e8..b1589bc 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5618,6 +5618,10 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 
 	/* Notify protocols, that we are about to destroy
 	   this device. They should clean all the things.
+
+	   Note that dev->reg_state stays at NETREG_REGISTERED.
+	   This is wanted because this way 8021q and macvlan know
+	   the device is just moving and can keep their slaves up.
 	*/
 	call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
 	call_netdevice_notifiers(NETDEV_UNREGISTER_BATCH, dev);
-- 
1.7.0.4

Re: [PATCH RFC] netns: keep vlan slaves on master netns move

[Daniel Lezcano]

On 08/24/2010 01:50 PM, David Lamparter wrote:
> Hi everyone,
>
> attached patch makes it possible to keep macvlan / 802.1q slave devices
> on moving their master to a different namespace. as the opposite works
> without problems (moving the slaves into a different namespace), this
> shouldn't cause problems either.
>
> i've tested this with 802.1q on real ethernet devs and bridges and,
> well, it works without crashing, but that obviously doesn't mean it is
> correct :) - therefore input very welcome.
>
> RFC,
>
> David
>
> P.S.: who do i Cc: on netns related stuff?

Definitively the netns is the brain child of:
Eric W. Biederman <ebiederm@xmission.com>

As I contributed and I am interested by following the changes, that will 
be nice if you can Cc me too.

Daniel Lezcano <daniel.lezcano@free.fr>

> --
> previously, if a vlan master device was moved from one network namespace
> to another, all 802.1q and macvlan slaves were deleted.
>
> we can use dev->reg_state to figure out whether dev_change_net_namespace
> is happening, since that won't set dev->reg_state NETREG_UNREGISTERING.
> so, this changes 8021q and macvlan to ignore NETDEV_UNREGISTER when
> reg_state is not NETREG_UNREGISTERING.
>
> Signed-off-by: David Lamparter<equinox@diac24.net>
> ---
>   drivers/net/macvlan.c |    4 ++++
>   net/8021q/vlan.c      |    4 ++++
>   net/core/dev.c        |    4 ++++
>   3 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index f15fe2c..f43f8e4 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -754,6 +754,10 @@ static int macvlan_device_event(struct notifier_block *unused,
>   		}
>   		break;
>   	case NETDEV_UNREGISTER:
> +		/* twiddle thumbs on netns device moves */
> +		if (dev->reg_state != NETREG_UNREGISTERING)
> +			break;
> +

Hmm, I don't feel comfortable with this change. It is like we are trying 
to catch a transient state, not clearly defined (you need a comment) and 
that may lead to some confusion later.

IMHO, it should be convenient to add a NETREG_NETNS_MOVING and set this 
state in the dev_change_net_namespace.

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5c6f519..0214b77 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -924,6 +924,7 @@ struct net_device {

         /* register/unregister state machine */
         enum { NETREG_UNINITIALIZED=0,
+              NETREG_NETNS_MOVING,     /* changing netns */
                NETREG_REGISTERED,       /* completed register_netdevice */
                NETREG_UNREGISTERING,    /* called unregister_netdevice */
                NETREG_UNREGISTERED,     /* completed unregister todo */
diff --git a/net/core/dev.c b/net/core/dev.c
index e233933..9154123 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5752,6 +5752,8 @@ int dev_change_net_namespace(struct net_device 
*dev, struct net *net, const char
         err = -ENODEV;
         unlist_netdevice(dev);

+       dev->reg_state = NETREG_NETNS_MOVING;
+
         synchronize_net();

         /* Shutdown queueing discipline. */
@@ -5786,6 +5788,8 @@ int dev_change_net_namespace(struct net_device 
*dev, struct net *net, const char
         err = netdev_register_kobject(dev);
         WARN_ON(err);

+       dev->reg_state = NETREG_REGISTERED;
+
         /* Add the device back in the hashes */
         list_netdevice(dev);


Then you can check in macvlan_device_event

	...

	if (dev->reg_state != NETREG_NETNS_MOVING)
		break;

	...


Does it make sense ?

   -- Daniel

Re: [PATCH RFC] netns: keep vlan slaves on master netns move

[Eric W. Biederman]

Daniel Lezcano <daniel.lezcano@free.fr> writes:

> On 08/24/2010 01:50 PM, David Lamparter wrote:
>> Hi everyone,
>>
>> attached patch makes it possible to keep macvlan / 802.1q slave devices
>> on moving their master to a different namespace. as the opposite works
>> without problems (moving the slaves into a different namespace), this
>> shouldn't cause problems either.
>>
>> i've tested this with 802.1q on real ethernet devs and bridges and,
>> well, it works without crashing, but that obviously doesn't mean it is
>> correct :) - therefore input very welcome.
>>
>> RFC,
>>
>> David
>>
>> P.S.: who do i Cc: on netns related stuff?
>
> Definitively the netns is the brain child of:
> Eric W. Biederman <ebiederm@xmission.com>
>
> As I contributed and I am interested by following the changes, that will be nice
> if you can Cc me too.
>
> Daniel Lezcano <daniel.lezcano@free.fr>
>
>> --
>> previously, if a vlan master device was moved from one network namespace
>> to another, all 802.1q and macvlan slaves were deleted.
>>
>> we can use dev->reg_state to figure out whether dev_change_net_namespace
>> is happening, since that won't set dev->reg_state NETREG_UNREGISTERING.
>> so, this changes 8021q and macvlan to ignore NETDEV_UNREGISTER when
>> reg_state is not NETREG_UNREGISTERING.

Agreed.  The new semantics are the desired ones.
>>
>> Signed-off-by: David Lamparter<equinox@diac24.net>
>> ---
>>   drivers/net/macvlan.c |    4 ++++
>>   net/8021q/vlan.c      |    4 ++++
>>   net/core/dev.c        |    4 ++++
>>   3 files changed, 12 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
>> index f15fe2c..f43f8e4 100644
>> --- a/drivers/net/macvlan.c
>> +++ b/drivers/net/macvlan.c
>> @@ -754,6 +754,10 @@ static int macvlan_device_event(struct notifier_block *unused,
>>   		}
>>   		break;
>>   	case NETDEV_UNREGISTER:
>> +		/* twiddle thumbs on netns device moves */
>> +		if (dev->reg_state != NETREG_UNREGISTERING)
>> +			break;
>> +
>
> Hmm, I don't feel comfortable with this change. It is like we are trying to
> catch a transient state, not clearly defined (you need a comment) and that may
> lead to some confusion later.
>
> IMHO, it should be convenient to add a NETREG_NETNS_MOVING and set this state in
> the dev_change_net_namespace.

Interesting.  I thought you were proposing a new notification but then
upon looking down I see you are simply proposing a new device state.
As a clear and minimal set of changes to the current design that seems
like a good way to go.

As a long term direction I suspect we might want to create an ethernet
device abstraction that can transmit and receive packets and move the
vlan, macvlan abstractions into it.  Having them exist as side cars
on the real network device gets us into a number of odd situations.

I was thinking we might want to move the dellink to somewhere else, but
since the list we want to traverse is in the vlan or macvlan specific
data it doesn't look like we can.  It is a real shame those extra
dellink calls cannot participate in batching.  Especially as that is one
of those places where we can expect real gains from batching.

Hmm.. It looks like the vlan devices mostly participate in batching,
although not in the same batch as their parent device, and it is simply
an implementation bug that the macvlan devices don't participate in
batch.

There has got to be a better more general way to handle this and prevent
the unnecessary code duplication and divergence between vlan and macvlan
devices.  Unfortunately I am not seeing it at the moment.

Eric

> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 5c6f519..0214b77 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -924,6 +924,7 @@ struct net_device {
>
>         /* register/unregister state machine */
>         enum { NETREG_UNINITIALIZED=0,
> +              NETREG_NETNS_MOVING,     /* changing netns */
>                NETREG_REGISTERED,       /* completed register_netdevice */
>                NETREG_UNREGISTERING,    /* called unregister_netdevice */
>                NETREG_UNREGISTERED,     /* completed unregister todo */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index e233933..9154123 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5752,6 +5752,8 @@ int dev_change_net_namespace(struct net_device *dev,
> struct net *net, const char
>         err = -ENODEV;
>         unlist_netdevice(dev);
>
> +       dev->reg_state = NETREG_NETNS_MOVING;
> +
>         synchronize_net();
>
>         /* Shutdown queueing discipline. */
> @@ -5786,6 +5788,8 @@ int dev_change_net_namespace(struct net_device *dev,
> struct net *net, const char
>         err = netdev_register_kobject(dev);
>         WARN_ON(err);
>
> +       dev->reg_state = NETREG_REGISTERED;
> +
>         /* Add the device back in the hashes */
>         list_netdevice(dev);
>
>
> Then you can check in macvlan_device_event
>
> 	...
>
> 	if (dev->reg_state != NETREG_NETNS_MOVING)
> 		break;
>
> 	...
>
>
> Does it make sense ?
>
>   -- Daniel
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] netns: introduce NETREG_NETNS_MOVING reg_state

[David Lamparter]

previously, if a vlan master device was moved from one network namespace
to another, all 802.1q and macvlan slaves were deleted.

that deletion is handled through the NETDEV_UNREGISTER notifier, which,
as such, works well and is probably the right thing to do as a moving
device really is disappearing for all higher-up users.

now, to allow 8021q and macvlan to keep their slaves, we can tell them
through dev->reg_state that this is a logical unregister and not a
physical one. reg_state == NETREG_NETNS_MOVING does exactly this.

Signed-off-by: David Lamparter <equinox@diac24.net>
---

Please note that i'm quite unsure about the correctness of this
patch due to me not having much experience in this kind of
modification...

In particular, the following might get broken by the new reg_state:
drivers/net/ixgbe/ixgbe_main.c (hot-unplug)
drivers/net/wimax/i2400m/driver.c (device reset)

The other users of reg_state should be fine AFAICT.

On Tue, Aug 24, 2010 at 12:14:31PM -0700, Eric W. Biederman wrote:
> >> --
> >> previously, if a vlan master device was moved from one network namespace
> >> to another, all 802.1q and macvlan slaves were deleted.
> >>
> >> we can use dev->reg_state to figure out whether dev_change_net_namespace
> >> is happening, since that won't set dev->reg_state NETREG_UNREGISTERING.
> >> so, this changes 8021q and macvlan to ignore NETDEV_UNREGISTER when
> >> reg_state is not NETREG_UNREGISTERING.
> 
> Agreed.  The new semantics are the desired ones.
>
> > Hmm, I don't feel comfortable with this change. It is like we are trying to
> > catch a transient state, not clearly defined (you need a comment) and that may
> > lead to some confusion later.
> >
> > IMHO, it should be convenient to add a NETREG_NETNS_MOVING and set this state in
> > the dev_change_net_namespace.
> 
> Interesting.  I thought you were proposing a new notification but then
> upon looking down I see you are simply proposing a new device state.
> As a clear and minimal set of changes to the current design that seems
> like a good way to go.

During creating the patch from my original mail, I had first added a
NETDEV_NETNS_MOVING notification, but that meant to either change every
notification user to treat NETNS_MOVING the same as UNREGISTERING (ugh)
or to send both NETNS_MOVING and UNREGISTER and use some flag to ignore
the second (ugly...)

What could be done, alternatively, is split the notification:
 * NETDEV_UNREGISTER - now changed to mean "logical unregister"
 * NETDEV_UNREGISTER_PHYSICAL - _not_ sent by netns move, means
    "the device is going for good"

> >         /* register/unregister state machine */
> >         enum { NETREG_UNINITIALIZED=0,
> > +              NETREG_NETNS_MOVING,     /* changing netns */
> >                NETREG_REGISTERED,       /* completed register_netdevice */
> >                NETREG_UNREGISTERING,    /* called unregister_netdevice */
> >                NETREG_UNREGISTERED,     /* completed unregister todo */

Patch:

 drivers/net/macvlan.c     |    4 ++++
 include/linux/netdevice.h |    1 +
 net/8021q/vlan.c          |    4 ++++
 net/core/dev.c            |    4 ++++
 4 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 0ef0eb0..a21602e 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -788,6 +788,10 @@ static int macvlan_device_event(struct notifier_block *unused,
 		}
 		break;
 	case NETDEV_UNREGISTER:
+		/* twiddle thumbs on netns device moves */
+		if (dev->reg_state == NETREG_NETNS_MOVING)
+			break;
+
 		list_for_each_entry_safe(vlan, next, &port->vlans, list)
 			vlan->dev->rtnl_link_ops->dellink(vlan->dev, NULL);
 		break;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 59962db..df0e1a5 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1016,6 +1016,7 @@ struct net_device {
 
 	/* register/unregister state machine */
 	enum { NETREG_UNINITIALIZED=0,
+	       NETREG_NETNS_MOVING,	/* in dev_change_net_namespace */
 	       NETREG_REGISTERED,	/* completed register_netdevice */
 	       NETREG_UNREGISTERING,	/* called unregister_netdevice */
 	       NETREG_UNREGISTERED,	/* completed unregister todo */
diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index a2ad152..f059110 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -525,6 +525,10 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 		break;
 
 	case NETDEV_UNREGISTER:
+		/* twiddle thumbs on netns device moves */
+		if (dev->reg_state == NETREG_NETNS_MOVING)
+			break;
+
 		/* Delete all VLANs for this dev. */
 		grp->killall = 1;
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 859e30f..0d6b8ba 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5664,6 +5664,8 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 	err = -ENODEV;
 	unlist_netdevice(dev);
 
+	dev->reg_state = NETREG_NETNS_MOVING;
+
 	synchronize_net();
 
 	/* Shutdown queueing discipline. */
@@ -5696,6 +5698,8 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 	err = device_rename(&dev->dev, dev->name);
 	WARN_ON(err);
 
+	dev->reg_state = NETREG_REGISTERED;
+
 	/* Add the device back in the hashes */
 	list_netdevice(dev);
 
-- 
1.7.0.4

Re: [PATCH RFC] netns: introduce NETREG_NETNS_MOVING reg_state

[Daniel Lezcano]

On 08/25/2010 01:52 PM, David Lamparter wrote:
> previously, if a vlan master device was moved from one network namespace
> to another, all 802.1q and macvlan slaves were deleted.
>
> that deletion is handled through the NETDEV_UNREGISTER notifier, which,
> as such, works well and is probably the right thing to do as a moving
> device really is disappearing for all higher-up users.
>
> now, to allow 8021q and macvlan to keep their slaves, we can tell them
> through dev->reg_state that this is a logical unregister and not a
> physical one. reg_state == NETREG_NETNS_MOVING does exactly this.
>
> Signed-off-by: David Lamparter<equinox@diac24.net>
> ---
>
> Please note that i'm quite unsure about the correctness of this
> patch due to me not having much experience in this kind of
> modification...
>
> In particular, the following might get broken by the new reg_state:
> drivers/net/ixgbe/ixgbe_main.c (hot-unplug)
> drivers/net/wimax/i2400m/driver.c (device reset)
>    

IMHO, that should be ok. NETREG_NETNS_MOVING is a transient state where 
the network device is not registered and not in the netdev list.

I think you should take care of this kind of comparison:

net/core/net-sysfs.c

static inline int dev_isalive(const struct net_device *dev)
{
         return dev->reg_state <= NETREG_REGISTERED;
}

Not sure having NETREG_NETNS_MOVING < NETREG_REGISTERED is right.


> The other users of reg_state should be fine AFAICT.
>
> On Tue, Aug 24, 2010 at 12:14:31PM -0700, Eric W. Biederman wrote:
>    
>>>> --
>>>> previously, if a vlan master device was moved from one network namespace
>>>> to another, all 802.1q and macvlan slaves were deleted.
>>>>
>>>> we can use dev->reg_state to figure out whether dev_change_net_namespace
>>>> is happening, since that won't set dev->reg_state NETREG_UNREGISTERING.
>>>> so, this changes 8021q and macvlan to ignore NETDEV_UNREGISTER when
>>>> reg_state is not NETREG_UNREGISTERING.
>>>>          
>> Agreed.  The new semantics are the desired ones.
>>
>>      
>>> Hmm, I don't feel comfortable with this change. It is like we are trying to
>>> catch a transient state, not clearly defined (you need a comment) and that may
>>> lead to some confusion later.
>>>
>>> IMHO, it should be convenient to add a NETREG_NETNS_MOVING and set this state in
>>> the dev_change_net_namespace.
>>>        
>> Interesting.  I thought you were proposing a new notification but then
>> upon looking down I see you are simply proposing a new device state.
>> As a clear and minimal set of changes to the current design that seems
>> like a good way to go.
>>      
> During creating the patch from my original mail, I had first added a
> NETDEV_NETNS_MOVING notification, but that meant to either change every
> notification user to treat NETNS_MOVING the same as UNREGISTERING (ugh)
> or to send both NETNS_MOVING and UNREGISTER and use some flag to ignore
> the second (ugly...)
>
> What could be done, alternatively, is split the notification:
>   * NETDEV_UNREGISTER - now changed to mean "logical unregister"
>   * NETDEV_UNREGISTER_PHYSICAL - _not_ sent by netns move, means
>      "the device is going for good"
>    

Maybe I miss something but that will have a big impact to all the 
network stacks no ?

>>>          /* register/unregister state machine */
>>>          enum { NETREG_UNINITIALIZED=0,
>>> +              NETREG_NETNS_MOVING,     /* changing netns */
>>>                 NETREG_REGISTERED,       /* completed register_netdevice */
>>>                 NETREG_UNREGISTERING,    /* called unregister_netdevice */
>>>                 NETREG_UNREGISTERED,     /* completed unregister todo */
>>>        
> Patch:
>
>   drivers/net/macvlan.c     |    4 ++++
>   include/linux/netdevice.h |    1 +
>   net/8021q/vlan.c          |    4 ++++
>   net/core/dev.c            |    4 ++++
>   4 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 0ef0eb0..a21602e 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -788,6 +788,10 @@ static int macvlan_device_event(struct notifier_block *unused,
>   		}
>   		break;
>   	case NETDEV_UNREGISTER:
> +		/* twiddle thumbs on netns device moves */
> +		if (dev->reg_state == NETREG_NETNS_MOVING)
> +			break;
> +
>   		list_for_each_entry_safe(vlan, next,&port->vlans, list)
>   			vlan->dev->rtnl_link_ops->dellink(vlan->dev, NULL);
>   		break;
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 59962db..df0e1a5 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1016,6 +1016,7 @@ struct net_device {
>
>   	/* register/unregister state machine */
>   	enum { NETREG_UNINITIALIZED=0,
> +	       NETREG_NETNS_MOVING,	/* in dev_change_net_namespace */
>   	       NETREG_REGISTERED,	/* completed register_netdevice */
>   	       NETREG_UNREGISTERING,	/* called unregister_netdevice */
>   	       NETREG_UNREGISTERED,	/* completed unregister todo */
> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
> index a2ad152..f059110 100644
> --- a/net/8021q/vlan.c
> +++ b/net/8021q/vlan.c
> @@ -525,6 +525,10 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
>   		break;
>
>   	case NETDEV_UNREGISTER:
> +		/* twiddle thumbs on netns device moves */
> +		if (dev->reg_state == NETREG_NETNS_MOVING)
> +			break;
> +
>   		/* Delete all VLANs for this dev. */
>   		grp->killall = 1;
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 859e30f..0d6b8ba 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5664,6 +5664,8 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
>   	err = -ENODEV;
>   	unlist_netdevice(dev);
>
> +	dev->reg_state = NETREG_NETNS_MOVING;
> +
>   	synchronize_net();
>
>   	/* Shutdown queueing discipline. */
> @@ -5696,6 +5698,8 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
>   	err = device_rename(&dev->dev, dev->name);
>   	WARN_ON(err);
>
> +	dev->reg_state = NETREG_REGISTERED;
> +
>   	/* Add the device back in the hashes */
>   	list_netdevice(dev);
>
>    

Looks good for me.

Thanks
   -- Daniel

Re: [PATCH RFC] netns: introduce NETREG_NETNS_MOVING reg_state

[David Lamparter]

On Wed, Aug 25, 2010 at 03:03:00PM +0200, Daniel Lezcano wrote:
> > Please note that i'm quite unsure about the correctness of this
> > patch due to me not having much experience in this kind of
> > modification...
> >
> > In particular, the following might get broken by the new reg_state:
> > drivers/net/ixgbe/ixgbe_main.c (hot-unplug)
> > drivers/net/wimax/i2400m/driver.c (device reset)
> >    
> 
> IMHO, that should be ok. NETREG_NETNS_MOVING is a transient state where 
> the network device is not registered and not in the netdev list.
> 
> I think you should take care of this kind of comparison:
> 
> net/core/net-sysfs.c
> 
> static inline int dev_isalive(const struct net_device *dev)
> {
>          return dev->reg_state <= NETREG_REGISTERED;
> }
> 
> Not sure having NETREG_NETNS_MOVING < NETREG_REGISTERED is right.

I can't say I fully understand the code and all of its implications, but
I think all of the attributes sysfs provides access to can theoretically
be safely accessed while the device is moving.

> > What could be done, alternatively, is split the notification:
> >   * NETDEV_UNREGISTER - now changed to mean "logical unregister"
> >   * NETDEV_UNREGISTER_PHYSICAL - _not_ sent by netns move, means
> >      "the device is going for good"
> >    
> 
> Maybe I miss something but that will have a big impact to all the 
> network stacks no ?

No; since NETDEV_UNREGISTER is still sent in all places where it is
currently sent, none of the stacks would require changing. However,
users of the actual, physical device (independent of its network
namespace) would need changing to NETDEV_UNREGISTER_PHYSICAL, which is a
new notification that is only sent if the device really disappears.

TBH, I'm not quite sure which is the better solution here;
NETDEV_UNREGISTER_PHYSICAL or NETREG_NETNS_MOVING...
... or the initial one?


-David

Re: [PATCH RFC] netns: introduce NETREG_NETNS_MOVING reg_state

[Eric W. Biederman]

David Lamparter <equinox@diac24.net> writes:

> On Wed, Aug 25, 2010 at 03:03:00PM +0200, Daniel Lezcano wrote:
>> > Please note that i'm quite unsure about the correctness of this
>> > patch due to me not having much experience in this kind of
>> > modification...
>> >
>> > In particular, the following might get broken by the new reg_state:
>> > drivers/net/ixgbe/ixgbe_main.c (hot-unplug)
>> > drivers/net/wimax/i2400m/driver.c (device reset)
>> >    
>> 
>> IMHO, that should be ok. NETREG_NETNS_MOVING is a transient state where 
>> the network device is not registered and not in the netdev list.
>> 
>> I think you should take care of this kind of comparison:
>> 
>> net/core/net-sysfs.c
>> 
>> static inline int dev_isalive(const struct net_device *dev)
>> {
>>          return dev->reg_state <= NETREG_REGISTERED;
>> }
>> 
>> Not sure having NETREG_NETNS_MOVING < NETREG_REGISTERED is right.
>
> I can't say I fully understand the code and all of its implications, but
> I think all of the attributes sysfs provides access to can theoretically
> be safely accessed while the device is moving.
>
>> > What could be done, alternatively, is split the notification:
>> >   * NETDEV_UNREGISTER - now changed to mean "logical unregister"
>> >   * NETDEV_UNREGISTER_PHYSICAL - _not_ sent by netns move, means
>> >      "the device is going for good"
>> >    
>> 
>> Maybe I miss something but that will have a big impact to all the 
>> network stacks no ?
>
> No; since NETDEV_UNREGISTER is still sent in all places where it is
> currently sent, none of the stacks would require changing. However,
> users of the actual, physical device (independent of its network
> namespace) would need changing to NETDEV_UNREGISTER_PHYSICAL, which is a
> new notification that is only sent if the device really disappears.
>
> TBH, I'm not quite sure which is the better solution here;
> NETDEV_UNREGISTER_PHYSICAL or NETREG_NETNS_MOVING...
> ... or the initial one?

Before we get too far down any of these paths, what is it that caused
you to notice that moving the physical device broke the connection to
the vlan and macvlan devices?

I just want to understand this case a little better before we walk down
any of these paths.

Eric

Re: [PATCH RFC] netns: introduce NETREG_NETNS_MOVING reg_state

[David Lamparter]

On Wed, Aug 25, 2010 at 10:39:01AM -0700, Eric W. Biederman wrote:
> David Lamparter <equinox@diac24.net> writes:
> > No; since NETDEV_UNREGISTER is still sent in all places where it is
> > currently sent, none of the stacks would require changing. However,
> > users of the actual, physical device (independent of its network
> > namespace) would need changing to NETDEV_UNREGISTER_PHYSICAL, which is a
> > new notification that is only sent if the device really disappears.
> >
> > TBH, I'm not quite sure which is the better solution here;
> > NETDEV_UNREGISTER_PHYSICAL or NETREG_NETNS_MOVING...
> > ... or the initial one?
> 
> Before we get too far down any of these paths, what is it that caused
> you to notice that moving the physical device broke the connection to
> the vlan and macvlan devices?
> 
> I just want to understand this case a little better before we walk down
> any of these paths.

I'm doing system configuration software that allows you to manage
interfaces on a virtual routing platform. I can work around the
vlan-disappearings, but they kind of disrupt operation, especially as
they can affect namespaces not even related to what the user is changing
in the config.

If nothing else, I'd try to push my original patch. It is pretty
nonintrusive and should be OK with maybe a few more lines of comments.


David

Re: [PATCH RFC] netns: keep vlan slaves on master netns move

[David Lamparter]

previously, if a vlan master device was moved from one network namespace
to another, all 802.1q and macvlan slaves were deleted.

we can use dev->reg_state to figure out whether dev_change_net_namespace
is happening, since that won't set dev->reg_state NETREG_UNREGISTERING.
so, this changes 8021q and macvlan to ignore NETDEV_UNREGISTER when
reg_state is not NETREG_UNREGISTERING.

Signed-off-by: David Lamparter <equinox@diac24.net>
--

On Tue, Aug 24, 2010 at 01:50:56PM +0200, David Lamparter wrote:
> attached patch makes it possible to keep macvlan / 802.1q slave devices
> on moving their master to a different namespace. as the opposite works
> without problems (moving the slaves into a different namespace), this
> shouldn't cause problems either.

I would like to resurrect this. After the small excursion into
NETREG_NETNS_MOVING i've reevaluated my personal opinion and think the
behaviour is the desired one and this patch is the cleanest way to
implement it. While it depends on somewhat "fine print" on the reg_state
state machine, the probability of major screw-ups in this region is
rather small and it also quite likely makes a nice large boom when it is
unduly tampered with.


-David


 drivers/net/macvlan.c |    4 ++++
 net/8021q/vlan.c      |    4 ++++
 net/core/dev.c        |    4 ++++
 3 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index f15fe2c..f43f8e4 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -754,6 +754,10 @@ static int macvlan_device_event(struct notifier_block *unused,
 		}
 		break;
 	case NETDEV_UNREGISTER:
+		/* twiddle thumbs on netns device moves */
+		if (dev->reg_state != NETREG_UNREGISTERING)
+			break;
+
 		list_for_each_entry_safe(vlan, next, &port->vlans, list)
 			vlan->dev->rtnl_link_ops->dellink(vlan->dev, NULL);
 		break;
diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 3c1c8c1..cdc37e8 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -516,6 +516,10 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 		break;
 
 	case NETDEV_UNREGISTER:
+		/* twiddle thumbs on netns device moves */
+		if (dev->reg_state != NETREG_UNREGISTERING)
+			break;
+
 		/* Delete all VLANs for this dev. */
 		grp->killall = 1;
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 1f466e8..b1589bc 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5618,6 +5618,10 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 
 	/* Notify protocols, that we are about to destroy
 	   this device. They should clean all the things.
+
+	   Note that dev->reg_state stays at NETREG_REGISTERED.
+	   This is wanted because this way 8021q and macvlan know
+	   the device is just moving and can keep their slaves up.
 	*/
 	call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
 	call_netdevice_notifiers(NETDEV_UNREGISTER_BATCH, dev);
-- 
1.7.0.4

Re: [PATCH RFC] netns: keep vlan slaves on master netns move

[Eric W. Biederman]

Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>

My inclination is that the best way to handle this is to would be to
push the device deletion for vlan and macvlan devices into the device
core, where we could get the advantage of batched deletions.  Right
now vlan and macvlan devices are the only devices I know of that cause
other devices to be removed during unregister, so removing that
specialness seems reasonable.

However not being able to move the primary vlan to a different network
namespace is usability bug with no real alternatives.  There are
not any other patches on the table right now, and your patch below
seems obviously correct.  Let's get this merged before we forget about
this, bug fix.

David Lamparter <equinox@diac24.net> writes:

> previously, if a vlan master device was moved from one network namespace
> to another, all 802.1q and macvlan slaves were deleted.
>
> we can use dev->reg_state to figure out whether dev_change_net_namespace
> is happening, since that won't set dev->reg_state NETREG_UNREGISTERING.
> so, this changes 8021q and macvlan to ignore NETDEV_UNREGISTER when
> reg_state is not NETREG_UNREGISTERING.
>
> Signed-off-by: David Lamparter <equinox@diac24.net>
> --
>
> On Tue, Aug 24, 2010 at 01:50:56PM +0200, David Lamparter wrote:
>> attached patch makes it possible to keep macvlan / 802.1q slave devices
>> on moving their master to a different namespace. as the opposite works
>> without problems (moving the slaves into a different namespace), this
>> shouldn't cause problems either.
>
> I would like to resurrect this. After the small excursion into
> NETREG_NETNS_MOVING i've reevaluated my personal opinion and think the
> behaviour is the desired one and this patch is the cleanest way to
> implement it. While it depends on somewhat "fine print" on the reg_state
> state machine, the probability of major screw-ups in this region is
> rather small and it also quite likely makes a nice large boom when it is
> unduly tampered with.
>
>
> -David
>
>
>  drivers/net/macvlan.c |    4 ++++
>  net/8021q/vlan.c      |    4 ++++
>  net/core/dev.c        |    4 ++++
>  3 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index f15fe2c..f43f8e4 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -754,6 +754,10 @@ static int macvlan_device_event(struct notifier_block *unused,
>  		}
>  		break;
>  	case NETDEV_UNREGISTER:
> +		/* twiddle thumbs on netns device moves */
> +		if (dev->reg_state != NETREG_UNREGISTERING)
> +			break;
> +
>  		list_for_each_entry_safe(vlan, next, &port->vlans, list)
>  			vlan->dev->rtnl_link_ops->dellink(vlan->dev, NULL);
>  		break;
> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
> index 3c1c8c1..cdc37e8 100644
> --- a/net/8021q/vlan.c
> +++ b/net/8021q/vlan.c
> @@ -516,6 +516,10 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
>  		break;
>  
>  	case NETDEV_UNREGISTER:
> +		/* twiddle thumbs on netns device moves */
> +		if (dev->reg_state != NETREG_UNREGISTERING)
> +			break;
> +
>  		/* Delete all VLANs for this dev. */
>  		grp->killall = 1;
>  
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1f466e8..b1589bc 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5618,6 +5618,10 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
>  
>  	/* Notify protocols, that we are about to destroy
>  	   this device. They should clean all the things.
> +
> +	   Note that dev->reg_state stays at NETREG_REGISTERED.
> +	   This is wanted because this way 8021q and macvlan know
> +	   the device is just moving and can keep their slaves up.
>  	*/
>  	call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
>  	call_netdevice_notifiers(NETDEV_UNREGISTER_BATCH, dev);

Re: [PATCH RFC] netns: keep vlan slaves on master netns move

[Daniel Lezcano]

On 09/13/2010 01:43 PM, David Lamparter wrote:
> previously, if a vlan master device was moved from one network namespace
> to another, all 802.1q and macvlan slaves were deleted.
>
> we can use dev->reg_state to figure out whether dev_change_net_namespace
> is happening, since that won't set dev->reg_state NETREG_UNREGISTERING.
> so, this changes 8021q and macvlan to ignore NETDEV_UNREGISTER when
> reg_state is not NETREG_UNREGISTERING.
>
> Signed-off-by: David Lamparter<equinox@diac24.net>
>    
Acked-by: Daniel Lezcano <daniel.lezcano@free.fr>

Re: [PATCH RFC] netns: keep vlan slaves on master netns move

[Patrick McHardy]

Am 15.09.2010 04:10, schrieb Eric W. Biederman:
> 
> Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> My inclination is that the best way to handle this is to would be to
> push the device deletion for vlan and macvlan devices into the device
> core, where we could get the advantage of batched deletions.

We've added batched deletion to both about a year ago, what exactly
is the problem?

>  Right
> now vlan and macvlan devices are the only devices I know of that cause
> other devices to be removed during unregister, so removing that
> specialness seems reasonable.

Actually all devices can cause this when used as a lower device by
vlan or macvlan. Both vlan and macvlan are useless without a lower
device, so I don't see why we shouldn't remove them when the lower
device is unregistered.

> However not being able to move the primary vlan to a different network
> namespace is usability bug with no real alternatives.  There are
> not any other patches on the table right now, and your patch below
> seems obviously correct.  Let's get this merged before we forget about
> this, bug fix.

Looks reasonable to me.

Acked-by: Patrick McHardy <kaber@trash.net>

Re: [PATCH] netns: keep vlan slaves on master netns move

[David Lamparter]

previously, if a vlan master device was moved from one network namespace
to another, all 802.1q and macvlan slaves were deleted.

we can use dev->reg_state to figure out whether dev_change_net_namespace
is happening, since that won't set dev->reg_state NETREG_UNREGISTERING.
so, this changes 8021q and macvlan to ignore NETDEV_UNREGISTER when
reg_state is not NETREG_UNREGISTERING.

Signed-off-by: David Lamparter <equinox@diac24.net>
Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
Acked-by: Daniel Lezcano <daniel.lezcano@free.fr>
Acked-by: Patrick McHardy <kaber@trash.net>
---
Hi David,

Q)ueue, F)eedback, A)pply?

-David

On Tue, Sep 14, 2010 at 07:10:01PM -0700, Eric W. Biederman wrote:
> Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> My inclination is that the best way to handle this is to would be to
> push the device deletion for vlan and macvlan devices into the device
> core, where we could get the advantage of batched deletions.  Right
> now vlan and macvlan devices are the only devices I know of that cause
> other devices to be removed during unregister, so removing that
> specialness seems reasonable.
> 
> However not being able to move the primary vlan to a different network
> namespace is usability bug with no real alternatives.  There are
> not any other patches on the table right now, and your patch below
> seems obviously correct.  Let's get this merged before we forget about
> this, bug fix.
> 
> David Lamparter <equinox@diac24.net> writes:
> > On Tue, Aug 24, 2010 at 01:50:56PM +0200, David Lamparter wrote:
> >> attached patch makes it possible to keep macvlan / 802.1q slave devices
> >> on moving their master to a different namespace. as the opposite works
> >> without problems (moving the slaves into a different namespace), this
> >> shouldn't cause problems either.
> >
> > I would like to resurrect this. After the small excursion into
> > NETREG_NETNS_MOVING i've reevaluated my personal opinion and think the
> > behaviour is the desired one and this patch is the cleanest way to
> > implement it. While it depends on somewhat "fine print" on the reg_state
> > state machine, the probability of major screw-ups in this region is
> > rather small and it also quite likely makes a nice large boom when it is
> > unduly tampered with.

 drivers/net/macvlan.c |    4 ++++
 net/8021q/vlan.c      |    4 ++++
 net/core/dev.c        |    4 ++++
 3 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index f15fe2c..f43f8e4 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -754,6 +754,10 @@ static int macvlan_device_event(struct notifier_block *unused,
 		}
 		break;
 	case NETDEV_UNREGISTER:
+		/* twiddle thumbs on netns device moves */
+		if (dev->reg_state != NETREG_UNREGISTERING)
+			break;
+
 		list_for_each_entry_safe(vlan, next, &port->vlans, list)
 			vlan->dev->rtnl_link_ops->dellink(vlan->dev, NULL);
 		break;
diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 3c1c8c1..cdc37e8 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -516,6 +516,10 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 		break;
 
 	case NETDEV_UNREGISTER:
+		/* twiddle thumbs on netns device moves */
+		if (dev->reg_state != NETREG_UNREGISTERING)
+			break;
+
 		/* Delete all VLANs for this dev. */
 		grp->killall = 1;
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 1f466e8..b1589bc 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5618,6 +5618,10 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 
 	/* Notify protocols, that we are about to destroy
 	   this device. They should clean all the things.
+
+	   Note that dev->reg_state stays at NETREG_REGISTERED.
+	   This is wanted because this way 8021q and macvlan know
+	   the device is just moving and can keep their slaves up.
 	*/
 	call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
 	call_netdevice_notifiers(NETDEV_UNREGISTER_BATCH, dev);

Re: [PATCH] netns: keep vlan slaves on master netns move

[David Miller]

From: David Lamparter <equinox@diac24.net>
Date: Fri, 17 Sep 2010 15:22:19 +0200

> previously, if a vlan master device was moved from one network namespace
> to another, all 802.1q and macvlan slaves were deleted.
> 
> we can use dev->reg_state to figure out whether dev_change_net_namespace
> is happening, since that won't set dev->reg_state NETREG_UNREGISTERING.
> so, this changes 8021q and macvlan to ignore NETDEV_UNREGISTER when
> reg_state is not NETREG_UNREGISTERING.
> 
> Signed-off-by: David Lamparter <equinox@diac24.net>
> Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
> Acked-by: Daniel Lezcano <daniel.lezcano@free.fr>
> Acked-by: Patrick McHardy <kaber@trash.net>
> ---
> Hi David,
> 
> Q)ueue, F)eedback, A)pply?

I'll apply this when I get a chance, thanks.

Re: [PATCH RFC] netns: keep vlan slaves on master netns move

[Eric W. Biederman]

Patrick McHardy <kaber@trash.net> writes:

> Am 15.09.2010 04:10, schrieb Eric W. Biederman:
>> 
>> Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> 
>> My inclination is that the best way to handle this is to would be to
>> push the device deletion for vlan and macvlan devices into the device
>> core, where we could get the advantage of batched deletions.
>
> We've added batched deletion to both about a year ago, what exactly
> is the problem?

My problem is that while the both have dellink implemented we
don't really batch their deletes as best we can.

For macvlan when the underlying device is unregistered we delete
each macvlan device one by one.

For vlans we do batch the deletes, but not in the same batch as
the underlying device.

>>  Right
>> now vlan and macvlan devices are the only devices I know of that cause
>> other devices to be removed during unregister, so removing that
>> specialness seems reasonable.
>
> Actually all devices can cause this when used as a lower device by
> vlan or macvlan. Both vlan and macvlan are useless without a lower
> device, so I don't see why we shouldn't remove them when the lower
> device is unregistered.

I definitely think we should remove the devices when the lower device
is destroyed/removed.  However unregistered is a slightly different
concept, and arguably macvlan and vlan devices are much more intimately
connected to their underlying device than that.

What I meant is we should implement the deletion differently.  By doing
something like having a list of all derived devices for a device, that
we could walk and call dellink on from rollback_registered_many.  In
theory that could destroy a whole pile of macvlan and vlan devices with
various different stackings one on the other all in the same batch.

Eric