Re: [PATCH net-next v3 4/7] vxlan: improve vxlan route lookup checks.

[Pravin Shelar]

On Thu, Nov 17, 2016 at 7:59 AM, Jiri Benc  wrote:
> On Thu, 17 Nov 2016 10:17:01 +, David Laight wrote:
>> Worse than arbitrary, it adds 4 bytes of pad on 64bit systems.
>
> It does not, this is not a struct.
>
right.

After looking at the assembly code, it is clear that GCC and most of
modern compiler can reorder function variables for efficient storage.

Re: [PATCH net-next v3 4/7] vxlan: improve vxlan route lookup checks.

[Pravin Shelar]

On Thu, Nov 17, 2016 at 2:17 AM, David Laight  wrote:
> From: Of Jiri Benc
>> Sent: 15 November 2016 14:40
>> On Sun, 13 Nov 2016 20:43:55 -0800, Pravin B Shelar wrote:
>> > @@ -1929,8 +1951,8 @@ static void vxlan_xmit_one(struct sk_buff *skb, 
>> > struct net_device *dev,
>> > union vxlan_addr *src;
>> > struct vxlan_metadata _md;
>> > struct vxlan_metadata *md = &_md;
>> > -   struct dst_entry *ndst = NULL;
>> > __be16 src_port = 0, dst_port;
>> > +   struct dst_entry *ndst = NULL;
>> > __be32 vni, label;
>> > __be16 df = 0;
>> > __u8 tos, ttl;
>>
>> This looks kind of arbitrary. You might want to remove this hunk or
>> merge it to patch 3.
>
> Worse than arbitrary, it adds 4 bytes of pad on 64bit systems.
>
OK. I will send out a patch.
But this is not real issue in vxlan module today ;)

Re: [PATCH net-next v3 4/7] vxlan: improve vxlan route lookup checks.

[David Miller]

From: Jiri Benc 
Date: Thu, 17 Nov 2016 16:59:49 +0100

> On Thu, 17 Nov 2016 10:17:01 +, David Laight wrote:
>> Worse than arbitrary, it adds 4 bytes of pad on 64bit systems.
> 
> It does not, this is not a struct.

He is talking about on the function stack.

Re: [PATCH net-next v3 4/7] vxlan: improve vxlan route lookup checks.

[Jiri Benc]

On Thu, 17 Nov 2016 10:17:01 +, David Laight wrote:
> Worse than arbitrary, it adds 4 bytes of pad on 64bit systems.

It does not, this is not a struct.

 Jiri

RE: [PATCH net-next v3 4/7] vxlan: improve vxlan route lookup checks.

[David Laight]

From: Of Jiri Benc
> Sent: 15 November 2016 14:40
> On Sun, 13 Nov 2016 20:43:55 -0800, Pravin B Shelar wrote:
> > @@ -1929,8 +1951,8 @@ static void vxlan_xmit_one(struct sk_buff *skb, 
> > struct net_device *dev,
> > union vxlan_addr *src;
> > struct vxlan_metadata _md;
> > struct vxlan_metadata *md = &_md;
> > -   struct dst_entry *ndst = NULL;
> > __be16 src_port = 0, dst_port;
> > +   struct dst_entry *ndst = NULL;
> > __be32 vni, label;
> > __be16 df = 0;
> > __u8 tos, ttl;
> 
> This looks kind of arbitrary. You might want to remove this hunk or
> merge it to patch 3.

Worse than arbitrary, it adds 4 bytes of pad on 64bit systems.

David

Re: [PATCH net-next v3 4/7] vxlan: improve vxlan route lookup checks.

[Jiri Benc]

On Sun, 13 Nov 2016 20:43:55 -0800, Pravin B Shelar wrote:
> @@ -1929,8 +1951,8 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct 
> net_device *dev,
>   union vxlan_addr *src;
>   struct vxlan_metadata _md;
>   struct vxlan_metadata *md = &_md;
> - struct dst_entry *ndst = NULL;
>   __be16 src_port = 0, dst_port;
> + struct dst_entry *ndst = NULL;
>   __be32 vni, label;
>   __be16 df = 0;
>   __u8 tos, ttl;

This looks kind of arbitrary. You might want to remove this hunk or
merge it to patch 3.

Other than that,
Acked-by: Jiri Benc 

[PATCH net-next v3 4/7] vxlan: improve vxlan route lookup checks.

[Pravin B Shelar]

Move route sanity check to respective vxlan[4/6]_get_route functions.
This allows us to perform all sanity checks before caching the dst so
that we can avoid these checks on subsequent packets.
This give move accurate metadata information for packet from
fill_metadata_dst().

Signed-off-by: Pravin B Shelar 
---
 drivers/net/vxlan.c | 77 ++---
 1 file changed, 38 insertions(+), 39 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 8bb58f6..aabb918 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1789,7 +1789,8 @@ static int vxlan_build_skb(struct sk_buff *skb, struct 
dst_entry *dst,
return 0;
 }
 
-static struct rtable *vxlan_get_route(struct vxlan_dev *vxlan,
+static struct rtable *vxlan_get_route(struct vxlan_dev *vxlan, struct 
net_device *dev,
+ struct vxlan_sock *sock4,
  struct sk_buff *skb, int oif, u8 tos,
  __be32 daddr, __be32 *saddr,
  struct dst_cache *dst_cache,
@@ -1799,6 +1800,9 @@ static struct rtable *vxlan_get_route(struct vxlan_dev 
*vxlan,
struct rtable *rt = NULL;
struct flowi4 fl4;
 
+   if (!sock4)
+   return ERR_PTR(-EIO);
+
if (tos && !info)
use_cache = false;
if (use_cache) {
@@ -1816,16 +1820,26 @@ static struct rtable *vxlan_get_route(struct vxlan_dev 
*vxlan,
fl4.saddr = *saddr;
 
rt = ip_route_output_key(vxlan->net, );
-   if (!IS_ERR(rt)) {
+   if (likely(!IS_ERR(rt))) {
+   if (rt->dst.dev == dev) {
+   netdev_dbg(dev, "circular route to %pI4\n", );
+   ip_rt_put(rt);
+   return ERR_PTR(-ELOOP);
+   }
+
*saddr = fl4.saddr;
if (use_cache)
dst_cache_set_ip4(dst_cache, >dst, fl4.saddr);
+   } else {
+   netdev_dbg(dev, "no route to %pI4\n", );
+   return ERR_PTR(-ENETUNREACH);
}
return rt;
 }
 
 #if IS_ENABLED(CONFIG_IPV6)
 static struct dst_entry *vxlan6_get_route(struct vxlan_dev *vxlan,
+ struct net_device *dev,
  struct vxlan_sock *sock6,
  struct sk_buff *skb, int oif, u8 tos,
  __be32 label,
@@ -1861,8 +1875,16 @@ static struct dst_entry *vxlan6_get_route(struct 
vxlan_dev *vxlan,
err = ipv6_stub->ipv6_dst_lookup(vxlan->net,
 sock6->sock->sk,
 , );
-   if (err < 0)
-   return ERR_PTR(err);
+   if (unlikely(err < 0)) {
+   netdev_dbg(dev, "no route to %pI6\n", daddr);
+   return ERR_PTR(-ENETUNREACH);
+   }
+
+   if (unlikely(ndst->dev == dev)) {
+   netdev_dbg(dev, "circular route to %pI6\n", daddr);
+   dst_release(ndst);
+   return ERR_PTR(-ELOOP);
+   }
 
*saddr = fl6.saddr;
if (use_cache)
@@ -1929,8 +1951,8 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct 
net_device *dev,
union vxlan_addr *src;
struct vxlan_metadata _md;
struct vxlan_metadata *md = &_md;
-   struct dst_entry *ndst = NULL;
__be16 src_port = 0, dst_port;
+   struct dst_entry *ndst = NULL;
__be32 vni, label;
__be16 df = 0;
__u8 tos, ttl;
@@ -2007,29 +2029,14 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct 
net_device *dev,
struct vxlan_sock *sock4 = rcu_dereference(vxlan->vn4_sock);
struct rtable *rt;
 
-   if (!sock4)
-   goto drop;
-   sk = sock4->sock->sk;
-
-   rt = vxlan_get_route(vxlan, skb,
+   rt = vxlan_get_route(vxlan, dev, sock4, skb,
 rdst ? rdst->remote_ifindex : 0, tos,
 dst->sin.sin_addr.s_addr,
 >sin.sin_addr.s_addr,
 dst_cache, info);
-   if (IS_ERR(rt)) {
-   netdev_dbg(dev, "no route to %pI4\n",
-  >sin.sin_addr.s_addr);
-   dev->stats.tx_carrier_errors++;
-   goto tx_error;
-   }
-
-   if (rt->dst.dev == dev) {
-   netdev_dbg(dev, "circular route to %pI4\n",
-  >sin.sin_addr.s_addr);
-   dev->stats.collisions++;
-   ip_rt_put(rt);
+   if (IS_ERR(rt))
goto tx_error;
-   }
+   sk = sock4->sock->sk;
 
/*