Re: [RFC v1 1/3] udp_tunnel: add config option to bind to a device

Re: [RFC v1 1/3] udp_tunnel: add config option to bind to a device

[Nicolas Dichtel]

Le 14/11/2018 à 10:31, Alexis Bauvin a écrit :
> UDP tunnel sockets are always opened unbound to a specific device. This
> patch allow the socket to be bound on a custom device, which
> incidentally makes UDP tunnels VRF-aware if binding to an l3mdev.
> 
> Signed-off-by: Alexis Bauvin <abauvin@scaleway.com>
> Reviewed-by: Amine Kherbouche <akherbouche@scaleway.com>
> Tested-by: Amine Kherbouche <akherbouche@scaleway.com>
What is the difference with the previous version?
Maybe a cover letter would help to track the history.


Regards,
Nicolas

Re: [RFC v1 1/3] udp_tunnel: add config option to bind to a device

[Alexis Bauvin]

Le 14 nov. 2018 à 17:07, Nicolas Dichtel <nicolas.dichtel@6wind.com> a écrit :
> Le 14/11/2018 à 10:31, Alexis Bauvin a écrit :
>> UDP tunnel sockets are always opened unbound to a specific device. This
>> patch allow the socket to be bound on a custom device, which
>> incidentally makes UDP tunnels VRF-aware if binding to an l3mdev.
>> 
>> Signed-off-by: Alexis Bauvin <abauvin@scaleway.com>
>> Reviewed-by: Amine Kherbouche <akherbouche@scaleway.com>
>> Tested-by: Amine Kherbouche <akherbouche@scaleway.com>
> What is the difference with the previous version?
> Maybe a cover letter would help to track the history.
Unless a mistake from my side, you should have received the cover letter in
the previous email. The previous version had a typo in the commit log of the
third patch of this patch set.

> Regards,
> Nicolas
Regards,
Alexis

Re: [RFC v1 2/3] vxlan: add support for underlay in non-default VRF

[David Ahern]

you are making this more specific than it needs to be ....

On 11/14/18 1:31 AM, Alexis Bauvin wrote:
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 27bd586b94b0..7477b5510a04 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -208,11 +208,23 @@ static inline struct vxlan_rdst *first_remote_rtnl(struct vxlan_fdb *fdb)
>  	return list_first_entry(&fdb->remotes, struct vxlan_rdst, list);
>  }
>  
> +static int vxlan_get_l3mdev(struct net *net, int ifindex)
> +{
> +	struct net_device *dev;
> +
> +	dev = __dev_get_by_index(net, ifindex);
> +	while (dev && !netif_is_l3_master(dev))
> +		dev = netdev_master_upper_dev_get(dev);
> +
> +	return dev ? dev->ifindex : 0;
> +}

l3mdev_master_ifindex_by_index should work instead of defining this for
vxlan.

But I do not believe you need this function.


> +
>  /* Find VXLAN socket based on network namespace, address family and UDP port
>   * and enabled unshareable flags.
>   */
>  static struct vxlan_sock *vxlan_find_sock(struct net *net, sa_family_t family,
> -					  __be16 port, u32 flags)
> +					  __be16 port, u32 flags,
> +					  int l3mdev_ifindex)
>  {
>  	struct vxlan_sock *vs;
>  
> @@ -221,7 +233,8 @@ static struct vxlan_sock *vxlan_find_sock(struct net *net, sa_family_t family,
>  	hlist_for_each_entry_rcu(vs, vs_head(net, port), hlist) {
>  		if (inet_sk(vs->sock->sk)->inet_sport == port &&
>  		    vxlan_get_sk_family(vs) == family &&
> -		    vs->flags == flags)
> +		    vs->flags == flags &&
> +		    vs->sock->sk->sk_bound_dev_if == l3mdev_ifindex)

Why not allow the vxlan socket to bind to any ifindex? In that case this
socket lookup follows what we do for tcp, udp and raw sockets, and  you
don't need to call out vrf / l3mdev directly (ie.,
s/l3mdev_ifindex/ifindex/g) - it comes for free.

Re: [RFC v1 2/3] vxlan: add support for underlay in non-default VRF

[Alexis Bauvin]

Le 14 nov. 2018 à 20:58, David Ahern <dsa@cumulusnetworks.com> a écrit :
> 
> you are making this more specific than it needs to be ....
> 
> On 11/14/18 1:31 AM, Alexis Bauvin wrote:
>> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
>> index 27bd586b94b0..7477b5510a04 100644
>> --- a/drivers/net/vxlan.c
>> +++ b/drivers/net/vxlan.c
>> @@ -208,11 +208,23 @@ static inline struct vxlan_rdst *first_remote_rtnl(struct vxlan_fdb *fdb)
>> 	return list_first_entry(&fdb->remotes, struct vxlan_rdst, list);
>> }
>> 
>> +static int vxlan_get_l3mdev(struct net *net, int ifindex)
>> +{
>> +	struct net_device *dev;
>> +
>> +	dev = __dev_get_by_index(net, ifindex);
>> +	while (dev && !netif_is_l3_master(dev))
>> +		dev = netdev_master_upper_dev_get(dev);
>> +
>> +	return dev ? dev->ifindex : 0;
>> +}
> 
> l3mdev_master_ifindex_by_index should work instead of defining this for
> vxlan.
> 
> But I do not believe you need this function.

l3mdev_master_ifindex_by_index does not recursively climbs up the master chain.
This means that if the l3mdev is not a direct master of the device, it will not
be found.

E.G. Calling l3mdev_master_ifindex_by_index with the index of eth0 will
return 0:

+------+     +-----+     +----------+
|      |     |     |     |          |
| eth0 +-----+ br0 +-----+ vrf-blue |
|      |     |     |     |          |
+------+     +-----+     +----------+

This is because the underlying l3mdev_master_dev_rcu function fetches the master
(br0 in this case), checks whether it is an l3mdev (which it is not), and
returns its index if so.

So if using l3mdev_master_dev_rcu, using eth0 as a lower device will still bind
to no specific device, thus in the default VRF.

Maybe I should have patched l3mdev_master_dev_rcu to do a recursive resolution
(as vxlan_get_l3mdev does), but I don’t know the impact of such a change.

>> +
>> /* Find VXLAN socket based on network namespace, address family and UDP port
>>  * and enabled unshareable flags.
>>  */
>> static struct vxlan_sock *vxlan_find_sock(struct net *net, sa_family_t family,
>> -					  __be16 port, u32 flags)
>> +					  __be16 port, u32 flags,
>> +					  int l3mdev_ifindex)
>> {
>> 	struct vxlan_sock *vs;
>> 
>> @@ -221,7 +233,8 @@ static struct vxlan_sock *vxlan_find_sock(struct net *net, sa_family_t family,
>> 	hlist_for_each_entry_rcu(vs, vs_head(net, port), hlist) {
>> 		if (inet_sk(vs->sock->sk)->inet_sport == port &&
>> 		    vxlan_get_sk_family(vs) == family &&
>> -		    vs->flags == flags)
>> +		    vs->flags == flags &&
>> +		    vs->sock->sk->sk_bound_dev_if == l3mdev_ifindex)
> 
> Why not allow the vxlan socket to bind to any ifindex? In that case this
> socket lookup follows what we do for tcp, udp and raw sockets, and  you
> don't need to call out vrf / l3mdev directly (ie.,
> s/l3mdev_ifindex/ifindex/g) - it comes for free.

Reword will be done in next version!

Re: [RFC v1 2/3] vxlan: add support for underlay in non-default VRF

[David Ahern]

On 11/15/18 2:05 AM, Alexis Bauvin wrote:
> Le 14 nov. 2018 à 20:58, David Ahern <dsa@cumulusnetworks.com> a écrit :
>>
>> you are making this more specific than it needs to be ....
>>
>> On 11/14/18 1:31 AM, Alexis Bauvin wrote:
>>> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
>>> index 27bd586b94b0..7477b5510a04 100644
>>> --- a/drivers/net/vxlan.c
>>> +++ b/drivers/net/vxlan.c
>>> @@ -208,11 +208,23 @@ static inline struct vxlan_rdst *first_remote_rtnl(struct vxlan_fdb *fdb)
>>> 	return list_first_entry(&fdb->remotes, struct vxlan_rdst, list);
>>> }
>>>
>>> +static int vxlan_get_l3mdev(struct net *net, int ifindex)
>>> +{
>>> +	struct net_device *dev;
>>> +
>>> +	dev = __dev_get_by_index(net, ifindex);
>>> +	while (dev && !netif_is_l3_master(dev))
>>> +		dev = netdev_master_upper_dev_get(dev);
>>> +
>>> +	return dev ? dev->ifindex : 0;
>>> +}
>>
>> l3mdev_master_ifindex_by_index should work instead of defining this for
>> vxlan.
>>
>> But I do not believe you need this function.
> 
> l3mdev_master_ifindex_by_index does not recursively climbs up the master chain.
> This means that if the l3mdev is not a direct master of the device, it will not
> be found.
> 
> E.G. Calling l3mdev_master_ifindex_by_index with the index of eth0 will
> return 0:
> 
> +------+     +-----+     +----------+
> |      |     |     |     |          |
> | eth0 +-----+ br0 +-----+ vrf-blue |
> |      |     |     |     |          |
> +------+     +-----+     +----------+
> 

eth0 is not the L3/router interface in this picture; br0 is. There
should not be a need for invoking l3mdev_master_ifindex_by_index on eth0.

What device stacking are you expecting to handle with vxlan devices?
vxlan on eth0 with vxlan devices in a VRF? vxlan devices into a bridge
with the bridge (or SVI) enslaved to a VRF?


> This is because the underlying l3mdev_master_dev_rcu function fetches the master
> (br0 in this case), checks whether it is an l3mdev (which it is not), and
> returns its index if so.
> 
> So if using l3mdev_master_dev_rcu, using eth0 as a lower device will still bind
> to no specific device, thus in the default VRF.
> 
> Maybe I should have patched l3mdev_master_dev_rcu to do a recursive resolution
> (as vxlan_get_l3mdev does), but I don’t know the impact of such a change.

no, that is definitely the wrong the approach.

Re: [RFC v1 2/3] vxlan: add support for underlay in non-default VRF

[Alexis Bauvin]

Le 16 nov. 2018 à 08:37, David Ahern <dsa@cumulusnetworks.com> a écrit :
> On 11/15/18 2:05 AM, Alexis Bauvin wrote:
>> Le 14 nov. 2018 à 20:58, David Ahern <dsa@cumulusnetworks.com> a écrit :
>>> 
>>> you are making this more specific than it needs to be ....
>>> 
>>> On 11/14/18 1:31 AM, Alexis Bauvin wrote:
>>>> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
>>>> index 27bd586b94b0..7477b5510a04 100644
>>>> --- a/drivers/net/vxlan.c
>>>> +++ b/drivers/net/vxlan.c
>>>> @@ -208,11 +208,23 @@ static inline struct vxlan_rdst *first_remote_rtnl(struct vxlan_fdb *fdb)
>>>> 	return list_first_entry(&fdb->remotes, struct vxlan_rdst, list);
>>>> }
>>>> 
>>>> +static int vxlan_get_l3mdev(struct net *net, int ifindex)
>>>> +{
>>>> +	struct net_device *dev;
>>>> +
>>>> +	dev = __dev_get_by_index(net, ifindex);
>>>> +	while (dev && !netif_is_l3_master(dev))
>>>> +		dev = netdev_master_upper_dev_get(dev);
>>>> +
>>>> +	return dev ? dev->ifindex : 0;
>>>> +}
>>> 
>>> l3mdev_master_ifindex_by_index should work instead of defining this for
>>> vxlan.
>>> 
>>> But I do not believe you need this function.
>> 
>> l3mdev_master_ifindex_by_index does not recursively climbs up the master chain.
>> This means that if the l3mdev is not a direct master of the device, it will not
>> be found.
>> 
>> E.G. Calling l3mdev_master_ifindex_by_index with the index of eth0 will
>> return 0:
>> 
>> +------+     +-----+     +----------+
>> |      |     |     |     |          |
>> | eth0 +-----+ br0 +-----+ vrf-blue |
>> |      |     |     |     |          |
>> +------+     +-----+     +----------+
>> 
> 
> eth0 is not the L3/router interface in this picture; br0 is. There
> should not be a need for invoking l3mdev_master_ifindex_by_index on eth0.
> 
> What device stacking are you expecting to handle with vxlan devices?
> vxlan on eth0 with vxlan devices in a VRF? vxlan devices into a bridge
> with the bridge (or SVI) enslaved to a VRF?

The case I am trying to cover here is the user creating a VXLAN device with eth0
as its lower device (ip link add vxlan0 type vxlan ... dev eth0), thus ignoring
the fact that it should be br0 (the actual L3 interface). In this case, the only
information available from the module's point of view is eth0. I may be wrong,
but eth0 is indirectly "part" of vrf-blue (even if it is only L2), as packets
flowing in from it would land in vrf-blue if L3.

As for the device stacking, I am only interested in the VXLAN underlay: the
VXLAN device itself could be in a specific VRF or not, it should not influence
its underlay. 

+----------+                         +---------+
|          |                         |         |
| vrf-blue |                         | vrf-red |
|          |                         |         |
+----+-----+                         +----+----+
     |                                    |
     |                                    |
+----+-----+                         +----+----+
|          |                         |         |
| br-blue  |                         | br-red  |
|          |                         |         |
+----+-----+                         +---+-+---+
     |                                   | |
     |                             +-----+ +-----+
     |                             |             |
+----+-----+                +------+----+   +----+----+
|          |  lower device  |           |   |         |
|   eth0   | <- - - - - - - | vxlan-red |   | tap-red | (... more taps)
|          |                |           |   |         |
+----------+                +-----------+   +---------+


While I don't see any use case for having a bridged uplink when using VXLAN,
someone may and would find a different behavior depending on the lower device.
In the above example, vxlan-red's lower device should be br-blue, but a user
would expect the underlay VRF (vrf-blue) to still be taken into account if eth0
was used as the lower device.

A different approach would be to check if the lower device is a bridge. If not,
fetch a potential master bridge. Then, with this L3/router interface, we fetch
the l3mdev with l3mdev_master_ifindex_by_index (if any).

> 
>> This is because the underlying l3mdev_master_dev_rcu function fetches the master
>> (br0 in this case), checks whether it is an l3mdev (which it is not), and
>> returns its index if so.
>> 
>> So if using l3mdev_master_dev_rcu, using eth0 as a lower device will still bind
>> to no specific device, thus in the default VRF.
>> 
>> Maybe I should have patched l3mdev_master_dev_rcu to do a recursive resolution
>> (as vxlan_get_l3mdev does), but I don’t know the impact of such a change.
> 
> no, that is definitely the wrong the approach.

Ok! What is the best approach in your opinion?

Re: [RFC v1 2/3] vxlan: add support for underlay in non-default VRF

[David Ahern]

On 11/16/18 2:41 AM, Alexis Bauvin wrote:
> The case I am trying to cover here is the user creating a VXLAN device with eth0
> as its lower device (ip link add vxlan0 type vxlan ... dev eth0), thus ignoring
> the fact that it should be br0 (the actual L3 interface). In this case, the only
> information available from the module's point of view is eth0. I may be wrong,
> but eth0 is indirectly "part" of vrf-blue (even if it is only L2), as packets
> flowing in from it would land in vrf-blue if L3.

for routing lookups, yes.

> 
> As for the device stacking, I am only interested in the VXLAN underlay: the
> VXLAN device itself could be in a specific VRF or not, it should not influence
> its underlay. 
> 
> +----------+                         +---------+
> |          |                         |         |
> | vrf-blue |                         | vrf-red |
> |          |                         |         |
> +----+-----+                         +----+----+
>      |                                    |
>      |                                    |
> +----+-----+                         +----+----+
> |          |                         |         |
> | br-blue  |                         | br-red  |
> |          |                         |         |
> +----+-----+                         +---+-+---+
>      |                                   | |
>      |                             +-----+ +-----+
>      |                             |             |
> +----+-----+                +------+----+   +----+----+
> |          |  lower device  |           |   |         |
> |   eth0   | <- - - - - - - | vxlan-red |   | tap-red | (... more taps)
> |          |                |           |   |         |
> +----------+                +-----------+   +---------+
> 
> 
> While I don't see any use case for having a bridged uplink when using VXLAN,
> someone may and would find a different behavior depending on the lower device.
> In the above example, vxlan-red's lower device should be br-blue, but a user
> would expect the underlay VRF (vrf-blue) to still be taken into account if eth0
> was used as the lower device.
> 
> A different approach would be to check if the lower device is a bridge. If not,
> fetch a potential master bridge. Then, with this L3/router interface, we fetch
> the l3mdev with l3mdev_master_ifindex_by_index (if any).

ok. got it. Add the above diagram to the commit message to document the
use case.

> 
>>
>>> This is because the underlying l3mdev_master_dev_rcu function fetches the master
>>> (br0 in this case), checks whether it is an l3mdev (which it is not), and
>>> returns its index if so.
>>>
>>> So if using l3mdev_master_dev_rcu, using eth0 as a lower device will still bind
>>> to no specific device, thus in the default VRF.
>>>
>>> Maybe I should have patched l3mdev_master_dev_rcu to do a recursive resolution
>>> (as vxlan_get_l3mdev does), but I don’t know the impact of such a change.
>>
>> no, that is definitely the wrong the approach.
> 
> Ok! What is the best approach in your opinion?
> 

Add the new function to l3mdev.c. The name should be consistent with the
others -- so something like l3mdev_master_upper_by_index (l3mdev for the
namespace, you are passing an index and wanting the master device but in
this case want to walk upper devices).

Also, annotate with expected locking.

Re: [RFC v1 3/3] vxlan: handle underlay VRF changes

[David Ahern]

On 11/14/18 1:31 AM, Alexis Bauvin wrote:
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 7477b5510a04..188c0cdb8838 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -208,6 +208,18 @@ static inline struct vxlan_rdst *first_remote_rtnl(struct vxlan_fdb *fdb)
>  	return list_first_entry(&fdb->remotes, struct vxlan_rdst, list);
>  }
>  
> +static int vxlan_is_in_l3mdev_chain(struct net_device *chain,
> +				    struct net_device *dev)
> +{
> +	if (!chain)
> +		return 0;
> +
> +	if (chain->ifindex == dev->ifindex)
> +		return 1;
> +	return vxlan_is_in_l3mdev_chain(netdev_master_upper_dev_get(chain),
> +					dev);

l3mdev_master_dev_rcu

Re: [RFC v1 3/3] vxlan: handle underlay VRF changes

[Alexis Bauvin]

Le 14 nov. 2018 à 21:04, David Ahern <dsa@cumulusnetworks.com> a écrit :
> 
> On 11/14/18 1:31 AM, Alexis Bauvin wrote:
>> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
>> index 7477b5510a04..188c0cdb8838 100644
>> --- a/drivers/net/vxlan.c
>> +++ b/drivers/net/vxlan.c
>> @@ -208,6 +208,18 @@ static inline struct vxlan_rdst *first_remote_rtnl(struct vxlan_fdb *fdb)
>> 	return list_first_entry(&fdb->remotes, struct vxlan_rdst, list);
>> }
>> 
>> +static int vxlan_is_in_l3mdev_chain(struct net_device *chain,
>> +				    struct net_device *dev)
>> +{
>> +	if (!chain)
>> +		return 0;
>> +
>> +	if (chain->ifindex == dev->ifindex)
>> +		return 1;
>> +	return vxlan_is_in_l3mdev_chain(netdev_master_upper_dev_get(chain),
>> +					dev);
> 
> l3mdev_master_dev_rcu

Same thing here, please see reply to previous patch.