veth: Cleanly handle a missing peer_tb argument on creation.
Subsystems (2)
PATCH 1/1 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.
--- drivers/net/veth.c | 16 +++++++--------- 1 files changed, 7 insertions(+), 9 deletions(-)
drivers/net/veth.c +7 -9
@@ -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);
Signature · 1 line
1.5.3.rc6.17.g1911
David Miller
maintainerFrom: [email protected] (Eric W. Biederman)
Date: Wed, 12 Sep 2007 07:19:56 -0600
Quoted 9 lines
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_tbto 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.
Quoted 5 lines
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 <[email protected]>
This looks mostly fine, can someone else who knows veth a bit review this as well?
Quoted 39 lines · 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_tbto 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 <[email protected]>--- drivers/net/veth.c | 16 +++++++--------- 1 files changed, 7 insertions(+), 9 deletions(-)drivers/net/veth.c +2
-1@@ -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;
drivers/net/veth.c static int veth_newlink(struct net_device *dev, · line 342
Quoted 8 lines
drivers/net/veth.c +1
-3@@ -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?
Quoted 18 lines
drivers/net/veth.c +4
-5- -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);
Quoted 9 lines · Pavel Emelyanov
drivers/net/veth.c static int veth_newlink(struct net_device *dev, · line 342
Quoted 5 lines
drivers/net/veth.c +1
-3+ } - 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
Quoted 11 lines · Eric W. Biederman
Quoted 8 lines · Pavel Emelyanov
drivers/net/veth.c static int veth_newlink(struct net_device *dev, · line 342
Quoted 5 lines
drivers/net/veth.c +1
-3+ } - 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?
Quoted 3 lines
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.
Quoted 2 lines
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.
Quoted 2 lines
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.
Quoted 7 lines
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
Quoted 12 lines · Pavel Emelyanov
Quoted 11 lines · Eric W. Biederman
Quoted 8 lines · Pavel Emelyanov
drivers/net/veth.c static int veth_newlink(struct net_device *dev, · line 342
Quoted 5 lines
drivers/net/veth.c +1
-3+ } - 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?
Quoted 5 lines
Quoted 3 lines
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 5 lines
Quoted 2 lines
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 6 lines
Quoted 2 lines
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
Quoted 22 lines · Eric W. Biederman
Quoted 11 lines · Pavel Emelyanov
Quoted 10 lines · Eric W. Biederman
Quoted 8 lines · Pavel Emelyanov
drivers/net/veth.c static int veth_newlink(struct net_device *dev, · line 342
Quoted 5 lines
drivers/net/veth.c +1
-3+ } - 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?
Quoted 4 lines
Quoted 3 lines
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);
Quoted 6 lines
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 6 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 8 lines
What I have in my history is:
./ip/ip link add veth23 type vethI 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 9 lines
Quoted 5 lines
Quoted 2 lines
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_tbinstead of using tbp.
Well, maybe, but what to copy some region aside if we can use it as is.
Quoted 2 lines
Eric
Quoted 36 lines · Eric W. Biederman
Quoted 11 lines · Pavel Emelyanov
Quoted 10 lines · Eric W. Biederman
Quoted 8 lines · Pavel Emelyanov
drivers/net/veth.c static int veth_newlink(struct net_device *dev, · line 342
Quoted 5 lines
drivers/net/veth.c +1
-3+ } - 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?
Quoted 4 lines
Quoted 3 lines
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 4 lines
Quoted 2 lines
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.
Quoted 8 lines
What I have in my history is:
./ip/ip link add veth23 type vethI 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 12 lines
Quoted 5 lines
Quoted 2 lines
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_tbinstead of using tbp.Eric