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

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 <redacted>
---
 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

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 <redacted>

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

Eric W. Biederman wrote:

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);

Pavel Emelyanov [off-list ref] writes:

quoted

+	}
 
-		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

Eric W. Biederman wrote:

Pavel Emelyanov [off-list ref] writes:

quoted

quoted

+	}
 
-		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

Pavel Emelyanov [off-list ref] writes:

Eric W. Biederman wrote:

quoted

Pavel Emelyanov [off-list ref] writes:

quoted

quoted

+	}
 
-		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.

quoted

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.

quoted

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

Eric W. Biederman wrote:

Pavel Emelyanov [off-list ref] writes:

quoted

Eric W. Biederman wrote:

quoted

Pavel Emelyanov [off-list ref] writes:

quoted

quoted

+	}
 
-		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.

quoted

quoted

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.

quoted

quoted

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

Eric W. Biederman wrote:

Pavel Emelyanov [off-list ref] writes:

quoted

Eric W. Biederman wrote:

quoted

Pavel Emelyanov [off-list ref] writes:

quoted

quoted

+	}
 
-		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.

quoted

quoted

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.

quoted

quoted

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