[PATCH] veth: Cleanly handle a missing peer_tb argument on creation.

[PATCH] veth: Cleanly handle a missing peer_tb argument on creation.

[Eric W. Biederman]

I was getting strange kernel crashes when attempting to
create veth devices when I did not specify a peer argument
to /bin/ip.

So this patch defaults peer_tb to all zeros and doesn't attempt to
reuse the netlink attributes for the primary link to create the
secondary link and now I can't reproduce the failures.

Given that some of the most interesting netlink attributes to specify
like a mac address or a network device name seem are generally
the wrong thing to do this seems like the right approach.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 drivers/net/veth.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 9e6a746..d49bd2c 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -313,7 +313,7 @@ static int veth_newlink(struct net_device *dev,
 	struct net_device *peer;
 	struct veth_priv *priv;
 	char ifname[IFNAMSIZ];
-	struct nlattr *peer_tb[IFLA_MAX + 1], **tbp;
+	struct nlattr *peer_tb[IFLA_MAX + 1];
 
 	/*
 	 * create and register peer first
@@ -322,6 +322,7 @@ static int veth_newlink(struct net_device *dev,
 	 * skip it since no info from it is useful yet
 	 */
 
+	memset(peer_tb, 0, sizeof(peer_tb));
 	if (data != NULL && data[VETH_INFO_PEER] != NULL) {
 		struct nlattr *nla_peer;
 
@@ -336,21 +337,18 @@ static int veth_newlink(struct net_device *dev,
 		err = veth_validate(peer_tb, NULL);
 		if (err < 0)
 			return err;
+	}
 
-		tbp = peer_tb;
-	} else
-		tbp = tb;
-
-	if (tbp[IFLA_IFNAME])
-		nla_strlcpy(ifname, tbp[IFLA_IFNAME], IFNAMSIZ);
+	if (peer_tb[IFLA_IFNAME])
+		nla_strlcpy(ifname, peer_tb[IFLA_IFNAME], IFNAMSIZ);
 	else
 		snprintf(ifname, IFNAMSIZ, DRV_NAME "%%d");
 
-	peer = rtnl_create_link(dev->nd_net, ifname, &veth_link_ops, tbp);
+	peer = rtnl_create_link(dev->nd_net, ifname, &veth_link_ops, peer_tb);
 	if (IS_ERR(peer))
 		return PTR_ERR(peer);
 
-	if (tbp[IFLA_ADDRESS] == NULL)
+	if (peer_tb[IFLA_ADDRESS] == NULL)
 		random_ether_addr(peer->dev_addr);
 
 	err = register_netdevice(peer);
-- 
1.5.3.rc6.17.g1911

Re: [PATCH] veth: Cleanly handle a missing peer_tb argument on creation.

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Wed, 12 Sep 2007 07:19:56 -0600

> 
> I was getting strange kernel crashes when attempting to
> create veth devices when I did not specify a peer argument
> to /bin/ip.
> 
> So this patch defaults peer_tb to all zeros and doesn't attempt to
> reuse the netlink attributes for the primary link to create the
> secondary link and now I can't reproduce the failures.
> 
> Given that some of the most interesting netlink attributes to specify
> like a mac address or a network device name seem are generally
> the wrong thing to do this seems like the right approach.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>

This looks mostly fine, can someone else who knows
veth a bit review this as well?

Re: [PATCH] veth: Cleanly handle a missing peer_tb argument on creation.

[Pavel Emelyanov]

Eric W. Biederman wrote:
> I was getting strange kernel crashes when attempting to
> create veth devices when I did not specify a peer argument
> to /bin/ip.
> 
> So this patch defaults peer_tb to all zeros and doesn't attempt to
> reuse the netlink attributes for the primary link to create the
> secondary link and now I can't reproduce the failures.
> 
> Given that some of the most interesting netlink attributes to specify
> like a mac address or a network device name seem are generally
> the wrong thing to do this seems like the right approach.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> ---
>  drivers/net/veth.c |   16 +++++++---------
>  1 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 9e6a746..d49bd2c 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -313,7 +313,7 @@ static int veth_newlink(struct net_device *dev,
>  	struct net_device *peer;
>  	struct veth_priv *priv;
>  	char ifname[IFNAMSIZ];
> -	struct nlattr *peer_tb[IFLA_MAX + 1], **tbp;
> +	struct nlattr *peer_tb[IFLA_MAX + 1];
>  
>  	/*
>  	 * create and register peer first
> @@ -322,6 +322,7 @@ static int veth_newlink(struct net_device *dev,
>  	 * skip it since no info from it is useful yet
>  	 */
>  
> +	memset(peer_tb, 0, sizeof(peer_tb));
>  	if (data != NULL && data[VETH_INFO_PEER] != NULL) {
>  		struct nlattr *nla_peer;
>  
> @@ -336,21 +337,18 @@ static int veth_newlink(struct net_device *dev,
>  		err = veth_validate(peer_tb, NULL);
>  		if (err < 0)
>  			return err;
> +	}
>  
> -		tbp = peer_tb;
> -	} else
> -		tbp = tb;

The intention of this part was to get the same parameters for
peer as for the first device if no "peer" argument was specified
for ip utility. Does it still work?

> -
> -	if (tbp[IFLA_IFNAME])
> -		nla_strlcpy(ifname, tbp[IFLA_IFNAME], IFNAMSIZ);
> +	if (peer_tb[IFLA_IFNAME])
> +		nla_strlcpy(ifname, peer_tb[IFLA_IFNAME], IFNAMSIZ);
>  	else
>  		snprintf(ifname, IFNAMSIZ, DRV_NAME "%%d");
>  
> -	peer = rtnl_create_link(dev->nd_net, ifname, &veth_link_ops, tbp);
> +	peer = rtnl_create_link(dev->nd_net, ifname, &veth_link_ops, peer_tb);
>  	if (IS_ERR(peer))
>  		return PTR_ERR(peer);
>  
> -	if (tbp[IFLA_ADDRESS] == NULL)
> +	if (peer_tb[IFLA_ADDRESS] == NULL)
>  		random_ether_addr(peer->dev_addr);
>  
>  	err = register_netdevice(peer);

Re: [PATCH] veth: Cleanly handle a missing peer_tb argument on creation.

[Eric W. Biederman]

Pavel Emelyanov <xemul@openvz.org> writes:

>> +	}
>>  
>> -		tbp = peer_tb;
>> -	} else
>> -		tbp = tb;
>
> The intention of this part was to get the same parameters for
> peer as for the first device if no "peer" argument was specified
> for ip utility. Does it still work?

I know it is problematic because we try to assign the same name
to both network devices, if we assign a name to the primary
network device.  That can't work.

Beyond that I had some really weird crashes while testing this
piece of code, especially when I did not specify a peer parameter.

So it was just easier to avoid the problem with this patch then
to completely root cause it.

I think the easiest semantic is to not have any peer parameters
if they were not specified, then to try and to figure out which
subset of parameters to copy.  If I hadn't been getting weird
kernel crashes I would not have cared.

Eric

Re: [PATCH] veth: Cleanly handle a missing peer_tb argument on creation.

[Pavel Emelyanov]

Eric W. Biederman wrote:
> Pavel Emelyanov <xemul@openvz.org> writes:
> 
>>> +	}
>>>  
>>> -		tbp = peer_tb;
>>> -	} else
>>> -		tbp = tb;
>> The intention of this part was to get the same parameters for
>> peer as for the first device if no "peer" argument was specified
>> for ip utility. Does it still work?
> 
> I know it is problematic because we try to assign the same name
> to both network devices, if we assign a name to the primary
> network device.  That can't work.

This can - as you can see I reallocate the name lower.

> Beyond that I had some really weird crashes while testing this
> piece of code, especially when I did not specify a peer parameter.

Can you please give me the exact command that caused an oops.
I try simple ip link add type veth and everything is just fine.

> So it was just easier to avoid the problem with this patch then
> to completely root cause it.

Let me handle this problem. AFAIR this was one of wishes from 
Patrick that we make two equal devices in case peer is not given, 
not just the default peer.

> I think the easiest semantic is to not have any peer parameters
> if they were not specified, then to try and to figure out which
> subset of parameters to copy.  If I hadn't been getting weird
> kernel crashes I would not have cared.
> 
> Eric
> 

Thanks,
Pavel

Re: [PATCH] veth: Cleanly handle a missing peer_tb argument on creation.

[Eric W. Biederman]

Pavel Emelyanov <xemul@openvz.org> writes:

> Eric W. Biederman wrote:
>> Pavel Emelyanov <xemul@openvz.org> writes:
>> 
>>>> +	}
>>>>  
>>>> -		tbp = peer_tb;
>>>> -	} else
>>>> -		tbp = tb;
>>> The intention of this part was to get the same parameters for
>>> peer as for the first device if no "peer" argument was specified
>>> for ip utility. Does it still work?
>> 
>> I know it is problematic because we try to assign the same name
>> to both network devices, if we assign a name to the primary
>> network device.  That can't work.
>
> This can - as you can see I reallocate the name lower.

Hmm. I just see:
	if (tbp[IFLA_IFNAME])
		nla_strlcpy(ifname, tbp[IFLA_IFNAME], IFNAMSIZ);

Then lower I see:
	if (tb[IFLA_IFNAME])
		nla_strlcpy(dev->name, tb[IFLA_IFNAME], IFNAMSIZ);

If (tb == tbp) then dev->name == ifname
Unless I'm completely misreading that code.

>> Beyond that I had some really weird crashes while testing this
>> piece of code, especially when I did not specify a peer parameter.
>
> Can you please give me the exact command that caused an oops.
> I try simple ip link add type veth and everything is just fine.

It might have been 64bit specific. 

What I have in my history is:
./ip/ip link add veth23 type veth

I forget exactly how it failed but as I recall it wasn't as
nice as an oops.  My memory may be a bit foggy though.

If I haven't provided a bit enough clue I guess I can go back
and remove the patch and try to reproduce the failure again.

>> So it was just easier to avoid the problem with this patch then
>> to completely root cause it.
>
> Let me handle this problem. AFAIR this was one of wishes from 
> Patrick that we make two equal devices in case peer is not given, 
> not just the default peer.

Ok.  I have if we can track down the weird cases I have no problem
if we handle this.  I think it still might be simpler if just
copy tb onto peer_tb instead of using tbp.

Eric

Re: [PATCH] veth: Cleanly handle a missing peer_tb argument on creation.

[Pavel Emelyanov]

Eric W. Biederman wrote:
> Pavel Emelyanov <xemul@openvz.org> writes:
> 
>> Eric W. Biederman wrote:
>>> Pavel Emelyanov <xemul@openvz.org> writes:
>>>
>>>>> +	}
>>>>>  
>>>>> -		tbp = peer_tb;
>>>>> -	} else
>>>>> -		tbp = tb;
>>>> The intention of this part was to get the same parameters for
>>>> peer as for the first device if no "peer" argument was specified
>>>> for ip utility. Does it still work?
>>> I know it is problematic because we try to assign the same name
>>> to both network devices, if we assign a name to the primary
>>> network device.  That can't work.
>> This can - as you can see I reallocate the name lower.
> 
> Hmm. I just see:
> 	if (tbp[IFLA_IFNAME])
> 		nla_strlcpy(ifname, tbp[IFLA_IFNAME], IFNAMSIZ);
> 
> Then lower I see:
> 	if (tb[IFLA_IFNAME])
> 		nla_strlcpy(dev->name, tb[IFLA_IFNAME], IFNAMSIZ);
> 
> If (tb == tbp) then dev->name == ifname
> Unless I'm completely misreading that code.

There must be a
        if (strchr(dev->name, '%')) {
                err = dev_alloc_name(dev, dev->name);
                if (err < 0)
                        goto err_alloc_name;
        }
code just before registering the first device.

>>> Beyond that I had some really weird crashes while testing this
>>> piece of code, especially when I did not specify a peer parameter.
>> Can you please give me the exact command that caused an oops.
>> I try simple ip link add type veth and everything is just fine.
> 
> It might have been 64bit specific. 

Maybe. I will try on x86_64.

> What I have in my history is:
> ./ip/ip link add veth23 type veth
> 
> I forget exactly how it failed but as I recall it wasn't as
> nice as an oops.  My memory may be a bit foggy though.
> 
> If I haven't provided a bit enough clue I guess I can go back
> and remove the patch and try to reproduce the failure again.

That would be nice. Thanks.

>>> So it was just easier to avoid the problem with this patch then
>>> to completely root cause it.
>> Let me handle this problem. AFAIR this was one of wishes from 
>> Patrick that we make two equal devices in case peer is not given, 
>> not just the default peer.
> 
> Ok.  I have if we can track down the weird cases I have no problem
> if we handle this.  I think it still might be simpler if just
> copy tb onto peer_tb instead of using tbp.

Well, maybe, but what to copy some region aside if we can use it
as is.

> Eric
> 

Re: [PATCH] veth: Cleanly handle a missing peer_tb argument on creation.

[Pavel Emelyanov]

Eric W. Biederman wrote:
> Pavel Emelyanov <xemul@openvz.org> writes:
> 
>> Eric W. Biederman wrote:
>>> Pavel Emelyanov <xemul@openvz.org> writes:
>>>
>>>>> +	}
>>>>>  
>>>>> -		tbp = peer_tb;
>>>>> -	} else
>>>>> -		tbp = tb;
>>>> The intention of this part was to get the same parameters for
>>>> peer as for the first device if no "peer" argument was specified
>>>> for ip utility. Does it still work?
>>> I know it is problematic because we try to assign the same name
>>> to both network devices, if we assign a name to the primary
>>> network device.  That can't work.
>> This can - as you can see I reallocate the name lower.
> 
> Hmm. I just see:
> 	if (tbp[IFLA_IFNAME])
> 		nla_strlcpy(ifname, tbp[IFLA_IFNAME], IFNAMSIZ);
> 
> Then lower I see:
> 	if (tb[IFLA_IFNAME])
> 		nla_strlcpy(dev->name, tb[IFLA_IFNAME], IFNAMSIZ);
> 
> If (tb == tbp) then dev->name == ifname
> Unless I'm completely misreading that code.
> 
>>> Beyond that I had some really weird crashes while testing this
>>> piece of code, especially when I did not specify a peer parameter.
>> Can you please give me the exact command that caused an oops.
>> I try simple ip link add type veth and everything is just fine.
> 
> It might have been 64bit specific. 
> 
> What I have in my history is:
> ./ip/ip link add veth23 type veth
> 
> I forget exactly how it failed but as I recall it wasn't as
> nice as an oops.  My memory may be a bit foggy though.
> 
> If I haven't provided a bit enough clue I guess I can go back
> and remove the patch and try to reproduce the failure again.

Neither ip link add type veth nor your one fail on my x86_64 box.

However, maybe you didn't like that your command didn't produce
any devices. I can explain this. You order two *equal* devices with
the same name veth23. This has to fail. However if you request 
devices with generic name veth%d or with different names everything
is good.

So could you please give more clues on what's bad with veth driver.

>>> So it was just easier to avoid the problem with this patch then
>>> to completely root cause it.
>> Let me handle this problem. AFAIR this was one of wishes from 
>> Patrick that we make two equal devices in case peer is not given, 
>> not just the default peer.
> 
> Ok.  I have if we can track down the weird cases I have no problem
> if we handle this.  I think it still might be simpler if just
> copy tb onto peer_tb instead of using tbp.
> 
> Eric
>