From: Pavel Emelyanov <hidden> Date: 2012-08-06 14:13:15
Hi!
This set tries to summarize the recent discussion of making ifindices friendly
to checkpoint-restore and consists of:
1. Prepare hash function to non-unique ifindices
2. Allow for specifying the desired ifindex on net link creation
3. Make ifindex generation per-net
4. Simplify loopback device ifindex access
Changes since v1:
* Fixed a stupid mistake with a pointer bits shift
* Turned the netdev_hash_mix routine into the generic ptr_hash_mix one
* Added a comment describing why loopback index is always 1 after the patch
Thanks,
Pavel
From: Pavel Emelyanov <hidden> Date: 2012-08-06 14:13:53
This one is used to make a salt out of a pointer to be mixed to some
hash function later. Idea and implementation are proposed by Eric Dumazet.
Signed-off-by: Pavel Emelyanov <redacted>
---
include/linux/hash.h | 10 ++++++++++
include/net/netns/hash.h | 9 ++-------
2 files changed, 12 insertions(+), 7 deletions(-)
From: Pavel Emelyanov <hidden> Date: 2012-08-06 14:14:13
Eric noticed, that when there will be devices with equal indices, some
hash functions that use them will become less effective as they could.
Fix this in advance by taking the net_device address into calculations
instead of the device index. Since the net_device is always aligned in
memory, shift the pointer to eliminate always zero bits (like we do it
in net_hash_mix).
This is true for arp and ndisc hash fns. The netlabel, can and llc ones
are also ifindex-based, but that three are init_net-only, thus will not
be affected.
Signed-off-by: Pavel Emelyanov <redacted>
---
include/net/arp.h | 3 ++-
include/net/ndisc.h | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
From: Pavel Emelyanov <hidden> Date: 2012-08-06 14:14:29
Currently the RTM_NEWLINK results in -EOPNOTSUPP if the ifinfomsg->ifi_index
is not zero. I propose to allow requesting ifindices on link creation. This
is required by the checkpoint-restore to correctly restore a net namespace
(i.e. -- a container).
Signed-off-by: Pavel Emelyanov <redacted>
---
net/core/dev.c | 7 ++++++-
net/core/rtnetlink.c | 12 +++++++-----
2 files changed, 13 insertions(+), 6 deletions(-)
From: Pavel Emelyanov <hidden> Date: 2012-08-06 14:14:44
The ifinfomsg is in there (thanks kaber@ for foreseeing this long time ago),
so take the given ifidex and register netdev with it.
Signed-off-by: Pavel Emelyanov <redacted>
---
drivers/net/veth.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
From: Pavel Emelyanov <hidden> Date: 2012-08-06 14:15:01
Strictly speaking this is only _really_ required for checkpoint-restore to
make loopback device always have the same index.
This change appears to be safe wrt "ifindex should be unique per-system"
concept, as all the ifindex usage is either already made per net namespace
of is explicitly limited with init_net only.
There are two cool side effects of this. The first one -- ifindices of
devices in container are always small, regardless of how many containers
we've started (and re-started) so far. The second one is -- we can speed
up the loopback ifidex access as shown in the next patch.
Signed-off-by: Pavel Emelyanov <redacted>
---
include/net/net_namespace.h | 1 +
net/core/dev.c | 4 ++--
2 files changed, 3 insertions(+), 2 deletions(-)
@@ -5221,12 +5221,12 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)*/staticintdev_new_index(structnet*net){-staticintifindex;+intifindex=net->ifindex;for(;;){if(++ifindex<=0)ifindex=1;if(!__dev_get_by_index(net,ifindex))-returnifindex;+returnnet->ifindex=ifindex;}}
From: Pavel Emelyanov <hidden> Date: 2012-08-06 14:15:18
As pointed out, there are places, that access net->loopback_dev->ifindex
and after ifindex generation is made per-net this value becomes constant
equals 1. So go ahead and introduce the LOOPBACK_IFINDEX constant and use
it where appropriate.
Signed-off-by: Pavel Emelyanov <redacted>
---
drivers/net/loopback.c | 1 +
include/net/net_namespace.h | 7 +++++++
net/decnet/dn_route.c | 6 +++---
net/ipv4/fib_frontend.c | 2 +-
net/ipv4/ipmr.c | 2 +-
net/ipv4/netfilter/ipt_rpfilter.c | 2 +-
net/ipv4/route.c | 6 +++---
net/ipv6/route.c | 2 +-
8 files changed, 18 insertions(+), 10 deletions(-)
@@ -197,6 +197,7 @@ static __net_init int loopback_net_init(struct net *net)if(err)gotoout_free_netdev;+BUG_ON(dev->ifindex!=LOOPBACK_IFINDEX);net->loopback_dev=dev;return0;
@@ -979,7 +979,7 @@ static int dn_route_output_slow(struct dst_entry **pprt, const struct flowidn *o"dn_route_output_slow: dst=%04x src=%04x mark=%d"" iif=%d oif=%d\n",le16_to_cpu(oldflp->daddr),le16_to_cpu(oldflp->saddr),-oldflp->flowidn_mark,init_net.loopback_dev->ifindex,+oldflp->flowidn_mark,LOOPBACK_IFINDEX,oldflp->flowidn_oif);/* If we have an output interface, verify its a DECnet device */
This doesn't make much sense to me.
If the whole 32-bits of the pointer is useful for entropy on 32-bit
why isn't the whole 64-bits useful on 64-bit?
I would, instead, expect something like:
ptr ^ (ptr >> 32)
for the 64-bit case.
Also, that L1_CACHE_SHIFT is something callers can decide to do.
Only they know the size of their structure, the alignment used to
allocate such objects, and thus what bits are "less relevant" and
therefore profitable to elide from the bottom of the value.
This doesn't make much sense to me.
If the whole 32-bits of the pointer is useful for entropy on 32-bit
why isn't the whole 64-bits useful on 64-bit?
I would, instead, expect something like:
ptr ^ (ptr >> 32)
for the 64-bit case.
Also, that L1_CACHE_SHIFT is something callers can decide to do.
Only they know the size of their structure, the alignment used to
allocate such objects, and thus what bits are "less relevant" and
therefore profitable to elide from the bottom of the value.
.
Maybe it would be better to change the way neigh_table->hash work more
significantly then? Currently it is used like
hash = tbl->hash(key, dev, tbl->rnd);
hash >>= (32 - tbl->hash_shift);
i.e. the caller asks for u32 hash value and then trims some lower bits.
It can be changed like
hash = tbl->hash(key, dev, tbl->rnd, tbl->hash_shift);
making the hash fn trim the bits itself. This will allow us to use the
existing (declared to be proven to be effective) hash_ptr() routine for
the net_device pointer hashing (it requires the number of bits to use).
E.g. the arp hash might look like
static u32 arp_hashfn(u32 key, struct net_device *dev, u32 hash_rnd,
unsigned int bits)
{
return hash_ptr(dev, bits) ^ hash_32(key * hash_rnd, bits);
}
and the ndisc one like
static u32 ndisc_hashfn(u32 *pkey, struct net_device *dev, u32 *hash_rnd,
unsigned int bits)
{
return hash_ptr(dev, bits) ^
hash_32(key[0] * hash_rnd[0], bits) ^
hash_32(key[1] * hash_rnd[1], bits) ^
hash_32(key[2] * hash_rnd[2], bits) ^
hash_32(key[3] * hash_rnd[3], bits);
}
What do you think?
Thanks,
Pavel
This doesn't make much sense to me.
If the whole 32-bits of the pointer is useful for entropy on 32-bit
why isn't the whole 64-bits useful on 64-bit?
I would, instead, expect something like:
ptr ^ (ptr >> 32)
for the 64-bit case.
Also, that L1_CACHE_SHIFT is something callers can decide to do.
Only they know the size of their structure, the alignment used to
allocate such objects, and thus what bits are "less relevant" and
therefore profitable to elide from the bottom of the value.
.
Maybe it would be better to change the way neigh_table->hash work more
significantly then? Currently it is used like
hash = tbl->hash(key, dev, tbl->rnd);
hash >>= (32 - tbl->hash_shift);
i.e. the caller asks for u32 hash value and then trims some lower bits.
It can be changed like
hash = tbl->hash(key, dev, tbl->rnd, tbl->hash_shift);
making the hash fn trim the bits itself. This will allow us to use the
existing (declared to be proven to be effective) hash_ptr() routine for
the net_device pointer hashing (it requires the number of bits to use).
E.g. the arp hash might look like
static u32 arp_hashfn(u32 key, struct net_device *dev, u32 hash_rnd,
unsigned int bits)
{
return hash_ptr(dev, bits) ^ hash_32(key * hash_rnd, bits);
}
and the ndisc one like
static u32 ndisc_hashfn(u32 *pkey, struct net_device *dev, u32 *hash_rnd,
unsigned int bits)
{
return hash_ptr(dev, bits) ^
hash_32(key[0] * hash_rnd[0], bits) ^
hash_32(key[1] * hash_rnd[1], bits) ^
hash_32(key[2] * hash_rnd[2], bits) ^
hash_32(key[3] * hash_rnd[3], bits);
}
What do you think?
I think we should avoid hash_ptr() because its quite expensive
David suggested to not use the L1_CACHE_SHIFT and instead do a plain :
static inline u32 ptr_hash_mix(const void *ptr)
{
unsigned long val = (unsigned long)ptr;
#if BITS_PER_LONG == 64
val ^= (val >> 32);
#endif
return (u32)val;
}
By the way we could name this hash32_ptr() instead of ptr_hash_mix()
From: Pavel Emelyanov <hidden> Date: 2012-08-07 09:55:35
I think we should avoid hash_ptr() because its quite expensive
David suggested to not use the L1_CACHE_SHIFT and instead do a plain :
static inline u32 ptr_hash_mix(const void *ptr)
{
unsigned long val = (unsigned long)ptr;
#if BITS_PER_LONG == 64
val ^= (val >> 32);
#endif
return (u32)val;
}
By the way we could name this hash32_ptr() instead of ptr_hash_mix()
OK. I was under impression, that hash_ptr was balanced from the fast/effective
perspective, but I can't argue with you in that area :) So, please, consider
the below patch instead of #1 and #2 (the rest ones remain unchanged).
Thanks,
Pavel
From: Pavel Emelyanov <redacted>
Subject: [PATCH 1/5] net: Dont use ifindices in hash fns
Eric noticed, that when there will be devices with equal indices, some
hash functions that use them will become less effective as they could.
Fix this in advance by mixing the net_device address into the hash value
instead of the device index.
This is true for arp and ndisc hash fns. The netlabel, can and llc ones
are also ifindex-based, but that three are init_net-only, thus will not
be affected.
Many thanks to David and Eric for the hash32_ptr implementation!
Signed-off-by: Pavel Emelyanov <redacted>
---
include/linux/hash.h | 10 ++++++++++
include/net/arp.h | 3 ++-
include/net/ndisc.h | 3 ++-
3 files changed, 14 insertions(+), 2 deletions(-)
From: Eric Dumazet <hidden> Date: 2012-08-07 10:30:11
On Tue, 2012-08-07 at 13:55 +0400, Pavel Emelyanov wrote:
OK. I was under impression, that hash_ptr was balanced from the fast/effective
perspective, but I can't argue with you in that area :) So, please, consider
the below patch instead of #1 and #2 (the rest ones remain unchanged).
Thanks,
Pavel
From: Pavel Emelyanov <redacted>
Subject: [PATCH 1/5] net: Dont use ifindices in hash fns
Eric noticed, that when there will be devices with equal indices, some
hash functions that use them will become less effective as they could.
Fix this in advance by mixing the net_device address into the hash value
instead of the device index.
This is true for arp and ndisc hash fns. The netlabel, can and llc ones
are also ifindex-based, but that three are init_net-only, thus will not
be affected.
Many thanks to David and Eric for the hash32_ptr implementation!
Signed-off-by: Pavel Emelyanov <redacted>
---
include/linux/hash.h | 10 ++++++++++
include/net/arp.h | 3 ++-
include/net/ndisc.h | 3 ++-
3 files changed, 14 insertions(+), 2 deletions(-)
From: Pavel Emelyanov <hidden> Date: 2012-08-07 11:01:43
Currently the RTM_NEWLINK results in -EOPNOTSUPP if the ifinfomsg->ifi_index
is not zero. I propose to allow requesting ifindices on link creation. This
is required by the checkpoint-restore to correctly restore a net namespace
(i.e. -- a container).
Signed-off-by: Pavel Emelyanov <redacted>
---
net/core/dev.c | 7 ++++++-
net/core/rtnetlink.c | 12 +++++++-----
2 files changed, 13 insertions(+), 6 deletions(-)
From: Pavel Emelyanov <hidden> Date: 2012-08-07 11:02:09
The ifinfomsg is in there (thanks kaber@ for foreseeing this long time ago),
so take the given ifidex and register netdev with it.
Signed-off-by: Pavel Emelyanov <redacted>
---
drivers/net/veth.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
From: Pavel Emelyanov <hidden> Date: 2012-08-07 11:02:33
Strictly speaking this is only _really_ required for checkpoint-restore to
make loopback device always have the same index.
This change appears to be safe wrt "ifindex should be unique per-system"
concept, as all the ifindex usage is either already made per net namespace
of is explicitly limited with init_net only.
There are two cool side effects of this. The first one -- ifindices of
devices in container are always small, regardless of how many containers
we've started (and re-started) so far. The second one is -- we can speed
up the loopback ifidex access as shown in the next patch.
Signed-off-by: Pavel Emelyanov <redacted>
---
include/net/net_namespace.h | 1 +
net/core/dev.c | 4 ++--
2 files changed, 3 insertions(+), 2 deletions(-)
@@ -5221,12 +5221,12 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)*/staticintdev_new_index(structnet*net){-staticintifindex;+intifindex=net->ifindex;for(;;){if(++ifindex<=0)ifindex=1;if(!__dev_get_by_index(net,ifindex))-returnifindex;+returnnet->ifindex=ifindex;}}
From: Pavel Emelyanov <hidden> Date: 2012-08-07 11:03:34
As pointed out, there are places, that access net->loopback_dev->ifindex
and after ifindex generation is made per-net this value becomes constant
equals 1. So go ahead and introduce the LOOPBACK_IFINDEX constant and use
it where appropriate.
Signed-off-by: Pavel Emelyanov <redacted>
---
drivers/net/loopback.c | 1 +
include/net/net_namespace.h | 7 +++++++
net/decnet/dn_route.c | 6 +++---
net/ipv4/fib_frontend.c | 2 +-
net/ipv4/ipmr.c | 2 +-
net/ipv4/netfilter/ipt_rpfilter.c | 2 +-
net/ipv4/route.c | 6 +++---
net/ipv6/route.c | 2 +-
8 files changed, 18 insertions(+), 10 deletions(-)
@@ -197,6 +197,7 @@ static __net_init int loopback_net_init(struct net *net)if(err)gotoout_free_netdev;+BUG_ON(dev->ifindex!=LOOPBACK_IFINDEX);net->loopback_dev=dev;return0;
@@ -979,7 +979,7 @@ static int dn_route_output_slow(struct dst_entry **pprt, const struct flowidn *o"dn_route_output_slow: dst=%04x src=%04x mark=%d"" iif=%d oif=%d\n",le16_to_cpu(oldflp->daddr),le16_to_cpu(oldflp->saddr),-oldflp->flowidn_mark,init_net.loopback_dev->ifindex,+oldflp->flowidn_mark,LOOPBACK_IFINDEX,oldflp->flowidn_oif);/* If we have an output interface, verify its a DECnet device */
From: Eric Dumazet <hidden> Date: 2012-08-07 12:11:11
On Tue, 2012-08-07 at 15:02 +0400, Pavel Emelyanov wrote:
quoted hunk
Strictly speaking this is only _really_ required for checkpoint-restore to
make loopback device always have the same index.
This change appears to be safe wrt "ifindex should be unique per-system"
concept, as all the ifindex usage is either already made per net namespace
of is explicitly limited with init_net only.
There are two cool side effects of this. The first one -- ifindices of
devices in container are always small, regardless of how many containers
we've started (and re-started) so far. The second one is -- we can speed
up the loopback ifidex access as shown in the next patch.
Signed-off-by: Pavel Emelyanov <redacted>
---
include/net/net_namespace.h | 1 +
net/core/dev.c | 4 ++--
2 files changed, 3 insertions(+), 2 deletions(-)
@@ -5221,12 +5221,12 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)*/staticintdev_new_index(structnet*net){-staticintifindex;+intifindex=net->ifindex;for(;;){if(++ifindex<=0)ifindex=1;if(!__dev_get_by_index(net,ifindex))-returnifindex;+returnnet->ifindex=ifindex;}}
From: Pavel Emelyanov <hidden> Date: 2012-08-07 12:38:07
quoted
@@ -62,6 +62,7 @@ struct net { struct sock *rtnl; /* rtnetlink socket */ struct sock *genl_sock;+ int ifindex;
could you place ifindex right after dev_base_seq : avoid two holes
and use the same cache line, dirtied in
list_netdevice()/unlist_netdevice()
Sure! Here it is:
From: Pavel Emelyanov <redacted>
Subject: [PATCH 4/5] net: Make ifindex generation per-net namespace
Strictly speaking this is only _really_ required for checkpoint-restore to
make loopback device always have the same index.
This change appears to be safe wrt "ifindex should be unique per-system"
concept, as all the ifindex usage is either already made per net namespace
of is explicitly limited with init_net only.
There are two cool side effects of this. The first one -- ifindices of
devices in container are always small, regardless of how many containers
we've started (and re-started) so far. The second one is -- we can speed
up the loopback ifidex access as shown in the next patch.
v2: Place ifindex right after dev_base_seq : avoid two holes and use the
same cache line, dirtied in list_netdevice()/unlist_netdevice()
Signed-off-by: Pavel Emelyanov <redacted>
---
include/net/net_namespace.h | 1 +
net/core/dev.c | 4 ++--
2 files changed, 3 insertions(+), 2 deletions(-)
@@ -5221,12 +5221,12 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)*/staticintdev_new_index(structnet*net){-staticintifindex;+intifindex=net->ifindex;for(;;){if(++ifindex<=0)ifindex=1;if(!__dev_get_by_index(net,ifindex))-returnifindex;+returnnet->ifindex=ifindex;}}
From: Eric Dumazet <hidden> Date: 2012-08-07 13:13:18
On Tue, 2012-08-07 at 16:37 +0400, Pavel Emelyanov wrote:
quoted
quoted
@@ -62,6 +62,7 @@ struct net { struct sock *rtnl; /* rtnetlink socket */ struct sock *genl_sock;+ int ifindex;
could you place ifindex right after dev_base_seq : avoid two holes
and use the same cache line, dirtied in
list_netdevice()/unlist_netdevice()
Sure! Here it is:
From: Pavel Emelyanov <redacted>
Subject: [PATCH 4/5] net: Make ifindex generation per-net namespace
Strictly speaking this is only _really_ required for checkpoint-restore to
make loopback device always have the same index.
This change appears to be safe wrt "ifindex should be unique per-system"
concept, as all the ifindex usage is either already made per net namespace
of is explicitly limited with init_net only.
There are two cool side effects of this. The first one -- ifindices of
devices in container are always small, regardless of how many containers
we've started (and re-started) so far. The second one is -- we can speed
up the loopback ifidex access as shown in the next patch.
v2: Place ifindex right after dev_base_seq : avoid two holes and use the
same cache line, dirtied in list_netdevice()/unlist_netdevice()
Signed-off-by: Pavel Emelyanov <redacted>
---
From: Eric Dumazet <hidden> Date: 2012-08-07 13:13:59
On Tue, 2012-08-07 at 15:02 +0400, Pavel Emelyanov wrote:
As pointed out, there are places, that access net->loopback_dev->ifindex
and after ifindex generation is made per-net this value becomes constant
equals 1. So go ahead and introduce the LOOPBACK_IFINDEX constant and use
it where appropriate.
Signed-off-by: Pavel Emelyanov <redacted>
---
drivers/net/loopback.c | 1 +
include/net/net_namespace.h | 7 +++++++
net/decnet/dn_route.c | 6 +++---
net/ipv4/fib_frontend.c | 2 +-
net/ipv4/ipmr.c | 2 +-
net/ipv4/netfilter/ipt_rpfilter.c | 2 +-
net/ipv4/route.c | 6 +++---
net/ipv6/route.c | 2 +-
8 files changed, 18 insertions(+), 10 deletions(-)
From: Eric Dumazet <hidden> Date: 2012-08-07 13:14:13
On Tue, 2012-08-07 at 15:02 +0400, Pavel Emelyanov wrote:
The ifinfomsg is in there (thanks kaber@ for foreseeing this long time ago),
so take the given ifidex and register netdev with it.
Signed-off-by: Pavel Emelyanov <redacted>
---
drivers/net/veth.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
From: Eric Dumazet <hidden> Date: 2012-08-07 13:14:30
On Tue, 2012-08-07 at 15:01 +0400, Pavel Emelyanov wrote:
Currently the RTM_NEWLINK results in -EOPNOTSUPP if the ifinfomsg->ifi_index
is not zero. I propose to allow requesting ifindices on link creation. This
is required by the checkpoint-restore to correctly restore a net namespace
(i.e. -- a container).
Signed-off-by: Pavel Emelyanov <redacted>
From: Ben Hutchings <hidden> Date: 2012-08-07 18:36:17
On Tue, 2012-08-07 at 15:02 +0400, Pavel Emelyanov wrote:
quoted hunk
The ifinfomsg is in there (thanks kaber@ for foreseeing this long time ago),
so take the given ifidex and register netdev with it.
Signed-off-by: Pavel Emelyanov <redacted>
---
drivers/net/veth.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
@@ -348,6 +348,9 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,if(tbp[IFLA_ADDRESS]==NULL)eth_hw_addr_random(peer);+if(ifmp)+peer->ifindex=ifmp->ifi_index;+err=register_netdevice(peer);put_net(net);net=NULL;
Is this safe, given that this code path previously ignored
ifmp->ifi_index? Userland could be passing in garbage and may now fail
occasionally because the value clashes with an existing interface.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
From: David Miller <davem@davemloft.net> Date: 2012-08-07 21:39:35
From: Pavel Emelyanov <redacted>
Date: Tue, 07 Aug 2012 13:11:41 +0400
Maybe it would be better to change the way neigh_table->hash work more
significantly then? Currently it is used like
hash = tbl->hash(key, dev, tbl->rnd);
hash >>= (32 - tbl->hash_shift);
i.e. the caller asks for u32 hash value and then trims some lower bits.
We do this because the hash function we use in the neigh
implementations causes the top bits to have the most entropy.
Please look at the commits that made the code this way, it's
very much intentional.
From: David Miller <davem@davemloft.net> Date: 2012-08-07 21:42:40
Where is patch 1/5?
You have the resend the entire series as a group, every single one,
not just then ones you think you need to. Because when you only sent
1/5 all by itself, I tossed it.
Never take shortcuts like this.
From: Pavel Emelyanov <hidden> Date: 2012-08-08 09:01:04
On 08/07/2012 10:36 PM, Ben Hutchings wrote:
On Tue, 2012-08-07 at 15:02 +0400, Pavel Emelyanov wrote:
quoted
The ifinfomsg is in there (thanks kaber@ for foreseeing this long time ago),
so take the given ifidex and register netdev with it.
Signed-off-by: Pavel Emelyanov <redacted>
---
drivers/net/veth.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
@@ -348,6 +348,9 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,if(tbp[IFLA_ADDRESS]==NULL)eth_hw_addr_random(peer);+if(ifmp)+peer->ifindex=ifmp->ifi_index;+err=register_netdevice(peer);put_net(net);net=NULL;
Is this safe, given that this code path previously ignored
ifmp->ifi_index? Userland could be passing in garbage and may now fail
occasionally because the value clashes with an existing interface.
You're right, I've missed that fact :( The good news is that we still can
use the ifmp->ifi_index for the peer index configuration. We just need to
assume that if the caller specified the ifindex for the veth master device,
then it's aware of this possibility and should explicitly configure (or set
to 0) the peer's ifindex as well. Like this:
if (ifmp && (dev->ifindex != 0))
peer->ifindex = ifmp->ifi_index;
Does this assumption work from you POV?
From: Ben Hutchings <hidden> Date: 2012-08-08 13:27:00
On Wed, 2012-08-08 at 13:00 +0400, Pavel Emelyanov wrote:
On 08/07/2012 10:36 PM, Ben Hutchings wrote:
quoted
On Tue, 2012-08-07 at 15:02 +0400, Pavel Emelyanov wrote:
quoted
The ifinfomsg is in there (thanks kaber@ for foreseeing this long time ago),
so take the given ifidex and register netdev with it.
Signed-off-by: Pavel Emelyanov <redacted>
---
drivers/net/veth.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
@@ -348,6 +348,9 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,if(tbp[IFLA_ADDRESS]==NULL)eth_hw_addr_random(peer);+if(ifmp)+peer->ifindex=ifmp->ifi_index;+err=register_netdevice(peer);put_net(net);net=NULL;
Is this safe, given that this code path previously ignored
ifmp->ifi_index? Userland could be passing in garbage and may now fail
occasionally because the value clashes with an existing interface.
You're right, I've missed that fact :( The good news is that we still can
use the ifmp->ifi_index for the peer index configuration. We just need to
assume that if the caller specified the ifindex for the veth master device,
then it's aware of this possibility and should explicitly configure (or set
to 0) the peer's ifindex as well. Like this:
if (ifmp && (dev->ifindex != 0))
peer->ifindex = ifmp->ifi_index;
Does this assumption work from you POV?
Yes, that looks like a neat way to do it. Maybe with an explanatory
comment?
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
From: Pavel Emelyanov <hidden> Date: 2012-08-08 13:39:08
quoted
You're right, I've missed that fact :( The good news is that we still can
use the ifmp->ifi_index for the peer index configuration. We just need to
assume that if the caller specified the ifindex for the veth master device,
then it's aware of this possibility and should explicitly configure (or set
to 0) the peer's ifindex as well. Like this:
if (ifmp && (dev->ifindex != 0))
peer->ifindex = ifmp->ifi_index;
Does this assumption work from you POV?
Yes, that looks like a neat way to do it. Maybe with an explanatory
comment?
Yes, sure. I will resend the set shortly.
Thanks for the feedback!