| Message ID | 5020F5AB.3090101@parallels.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 <xemul@parallels.com> > --- > drivers/net/veth.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) Acked-by: Eric Dumazet <edumazet@google.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <xemul@parallels.com> > --- > drivers/net/veth.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/veth.c b/drivers/net/veth.c > index 5852361..496c026 100644 > --- a/drivers/net/veth.c > +++ b/drivers/net/veth.c > @@ -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.
On 08/07/2012 10:36 PM, Ben Hutchings wrote: > 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 <xemul@parallels.com> >> --- >> drivers/net/veth.c | 3 +++ >> 1 files changed, 3 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/net/veth.c b/drivers/net/veth.c >> index 5852361..496c026 100644 >> --- a/drivers/net/veth.c >> +++ b/drivers/net/veth.c >> @@ -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? > Ben. > Thanks, Pavel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2012-08-08 at 13:00 +0400, Pavel Emelyanov wrote: > On 08/07/2012 10:36 PM, Ben Hutchings wrote: > > 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 <xemul@parallels.com> > >> --- > >> drivers/net/veth.c | 3 +++ > >> 1 files changed, 3 insertions(+), 0 deletions(-) > >> > >> diff --git a/drivers/net/veth.c b/drivers/net/veth.c > >> index 5852361..496c026 100644 > >> --- a/drivers/net/veth.c > >> +++ b/drivers/net/veth.c > >> @@ -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.
>> 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! > Ben. > Thanks, Pavel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/veth.c b/drivers/net/veth.c index 5852361..496c026 100644 --- a/drivers/net/veth.c +++ b/drivers/net/veth.c @@ -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;
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 <xemul@parallels.com> --- drivers/net/veth.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)