| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 25 Aug 2010 15:03:00 +0200
From: Daniel Lezcano <daniel.lezcano@...e.fr>
To: David Lamparter <equinox@...c24.net>
CC: "Eric W. Biederman" <ebiederm@...ssion.com>,
netdev@...r.kernel.org, Patrick McHardy <kaber@...sh.net>
Subject: Re: [PATCH RFC] netns: introduce NETREG_NETNS_MOVING reg_state
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@...c24.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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists