Re: [PATCH] veth: Showing peer of veth type dev in ip link (kernel side)

[Nicolas Dichtel <nicolas.dichtel@6wind.com>]

Le 10/10/2013 02:17, Eric W. Biederman a écrit :
> Stephen Hemminger <stephen@networkplumber.org> writes:
>
>> On Tue, 8 Oct 2013 14:13:37 -0700
>> Stephen Hemminger <stephen@networkplumber.org> wrote:
>>
>>> On Tue, 08 Oct 2013 15:23:49 -0400 (EDT)
>>> David Miller <davem@davemloft.net> wrote:
>>>
>>>> From: Masatake YAMATO <yamato@redhat.com>
>>>> Date: Fri,  4 Oct 2013 11:34:21 +0900
>>>>
>>>>> ip link has ability to show extra information of net work device if
>>>>> kernel provides sunh information. With this patch veth driver can
>>>>> provide its peer ifindex information to ip command via netlink
>>>>> interface.
>>>>>
>>>>> Signed-off-by: Masatake YAMATO <yamato@redhat.com>
>>>>
>>>> Applied to net-next, thank you.
>>>> --
>>>> 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
>>>
>>> Please revert this. It is incorrect.
>>> The info returned by any netlink message should be equal to the message
>>> for setting.
>>>
>>> I think the correct patch would be something like this (compile tested only).
>>>
>>> --- a/drivers/net/veth.c	2013-10-06 14:48:23.806461177 -0700
>>> +++ b/drivers/net/veth.c	2013-10-08 14:11:42.434074690 -0700
>>> @@ -434,6 +434,35 @@ static const struct nla_policy veth_poli
>>>   	[VETH_INFO_PEER]	= { .len = sizeof(struct ifinfomsg) },
>>>   };
>>>
>>> +static size_t veth_get_size(const struct net_device *dev)
>>> +{
>>> +	return nla_total_size(sizeof(struct ifinfomsg)) + /* VETH_INFO_PEER */
>>> +		0;
>>> +}
>>> +
>>> +static int veth_fill_info(struct sk_buff *skb, const struct net_device *dev)
>>> +{
>>> +	struct veth_priv *priv = netdev_priv(dev);
>>> +	struct net_device *peer = rtnl_dereference(priv->peer);
>>> +
>>> +	if (peer) {
>>> +		struct ifinfomsg ifi = {
>>> +			.ifi_family = AF_UNSPEC,
>>> +			.ifi_type = peer->type,
>>> +			.ifi_index = peer->ifindex,
>>> +			.ifi_flags = dev_get_flags(peer),
>>> +		};
>>> +
>>> +		if (nla_put(skb, VETH_INFO_PEER, sizeof(ifi), &ifi))
>>> +			goto nla_put_failure;
>>> +	}
>>> +
>>> +	return 0;
>>> +
>>> +nla_put_failure:
>>> +	return -EMSGSIZE;
>>> +}
>>> +
>>>   static struct rtnl_link_ops veth_link_ops = {
>>>   	.kind		= DRV_NAME,
>>>   	.priv_size	= sizeof(struct veth_priv),
>>> @@ -443,6 +472,8 @@ static struct rtnl_link_ops veth_link_op
>>>   	.dellink	= veth_dellink,
>>>   	.policy		= veth_policy,
>>>   	.maxtype	= VETH_INFO_MAX,
>>> +	.get_size	= veth_get_size,
>>> +	.fill_info	= veth_fill_info,
>>>   };
>>>
>>>   /*
>>>
>>>
>>
>> This patch is ok as RFC starting point but the full implementation needs to
>> add on IFLA_NAME and other attributes such that the full peer can be reconstructed.
>>
>> Ideally, the output of 'ip link' command can be in a format that can be used
>> to recreate the same veth pair.
>>
>> One issue is that veth has the ability to make a peer in a different namespace
>> and the network namespace code does not appear to have the ability to be invertable.
>> I.e it is not possible to construct IFLA_NET_NS_PID or IFLA_NET_NS_FD attributes
>> from an existing network device namespace.
>
> Right.
>
> IFLA_NET_NS_PID is not invertible as there may be no processes running
> in a pid namespace.
>
> IFLA_NET_NS_FD is in principle invertible.  We just need to add a file
> descriptor to the callers fd table.  I don't see IFLA_NET_NS_FD being
> invertible for broadcast messages, but for unicast it looks like a bit
> of a pain but there are no fundamental problems.
I'm not sure to understand why it is invertible only for unicast message.
Or are you saying that it is invertible only for the netns where the caller 
stands (and then not for the veth peer)?

>
> I don't know if we care enough yet to write the code for the
> IFLA_NET_NS_FD attribute but it is doable.
I care ;-)
Has somebody already started to write a patch?