Re: [PATCH net 7/7] net: ip6_gre: Fix ip6erspan hlen calculation

2018-05-17 Thread Petr Machata
Fixes: 5a963eb61b7c ("ip6_gre: Add ERSPAN native tunnel support")


Re: [PATCH net 7/7] net: ip6_gre: Fix ip6erspan hlen calculation

2018-05-17 Thread William Tu
On Thu, May 17, 2018 at 7:36 AM, Petr Machata  wrote:
> Even though ip6erspan_tap_init() sets up hlen and tun_hlen according to
> what ERSPAN needs, it goes ahead to call ip6gre_tnl_link_config() which
> overwrites these settings with GRE-specific ones.
>
> Similarly for changelink callbacks, which are handled by
> ip6gre_changelink() calls ip6gre_tnl_change() calls
> ip6gre_tnl_link_config() as well.
>
> The difference ends up being 12 vs. 20 bytes, and this is generally not
> a problem, because a 12-byte request likely ends up allocating more and
> the extra 8 bytes are thus available. However correct it is not.
>
> So replace the newlink and changelink callbacks with an ERSPAN-specific
> ones, reusing the newly-introduced _common() functions.
>
> Signed-off-by: Petr Machata 
> ---

Acked-by: William Tu 

Thanks, using ERSPAN-specific newlink and changelink is also on
my todo list.  I'm hitting another issue related to the shared collect_md_tun
between erspan and gre, I will make my patch based on this series.


>  net/ipv6/ip6_gre.c | 74 
> +++---
>  1 file changed, 65 insertions(+), 9 deletions(-)
>
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index c17e38b..36b1669 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -81,6 +81,7 @@ static int ip6gre_tunnel_init(struct net_device *dev);
>  static void ip6gre_tunnel_setup(struct net_device *dev);
>  static void ip6gre_tunnel_link(struct ip6gre_net *ign, struct ip6_tnl *t);
>  static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu);
> +static void ip6erspan_tnl_link_config(struct ip6_tnl *t, int set_mtu);
>
>  /* Tunnel hash table */
>
> @@ -1751,6 +1752,19 @@ static const struct net_device_ops 
> ip6gre_tap_netdev_ops = {
> .ndo_get_iflink = ip6_tnl_get_iflink,
>  };
>
> +static int ip6erspan_calc_hlen(struct ip6_tnl *tunnel)
> +{
> +   int t_hlen;
> +
> +   tunnel->tun_hlen = 8;
> +   tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen +
> +  erspan_hdr_len(tunnel->parms.erspan_ver);
> +
> +   t_hlen = tunnel->hlen + sizeof(struct ipv6hdr);
> +   tunnel->dev->hard_header_len = LL_MAX_HEADER + t_hlen;
> +   return t_hlen;
> +}
> +
>  static int ip6erspan_tap_init(struct net_device *dev)
>  {
> struct ip6_tnl *tunnel;
> @@ -1774,12 +1788,7 @@ static int ip6erspan_tap_init(struct net_device *dev)
> return ret;
> }
>
> -   tunnel->tun_hlen = 8;
> -   tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen +
> -  erspan_hdr_len(tunnel->parms.erspan_ver);
> -   t_hlen = tunnel->hlen + sizeof(struct ipv6hdr);
> -
> -   dev->hard_header_len = LL_MAX_HEADER + t_hlen;
> +   t_hlen = ip6erspan_calc_hlen(tunnel);
> dev->mtu = ETH_DATA_LEN - t_hlen;
> if (dev->type == ARPHRD_ETHER)
> dev->mtu -= ETH_HLEN;
> @@ -1787,7 +1796,7 @@ static int ip6erspan_tap_init(struct net_device *dev)
> dev->mtu -= 8;
>
> dev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
> -   ip6gre_tnl_link_config(tunnel, 1);
> +   ip6erspan_tnl_link_config(tunnel, 1);
>
> return 0;
>  }
> @@ -2118,6 +2127,53 @@ static void ip6erspan_tap_setup(struct net_device *dev)
> netif_keep_dst(dev);
>  }
>
> +static int ip6erspan_newlink(struct net *src_net, struct net_device *dev,
> +struct nlattr *tb[], struct nlattr *data[],
> +struct netlink_ext_ack *extack)
> +{
> +   int err = ip6gre_newlink_common(src_net, dev, tb, data, extack);
> +   struct ip6_tnl *nt = netdev_priv(dev);
> +   struct net *net = dev_net(dev);
> +
> +   if (!err) {
> +   ip6erspan_tnl_link_config(nt, !tb[IFLA_MTU]);
> +   ip6gre_tunnel_link(net_generic(net, ip6gre_net_id), nt);
> +   }
> +   return err;
> +}
> +
> +static void ip6erspan_tnl_link_config(struct ip6_tnl *t, int set_mtu)
> +{
> +   ip6gre_tnl_link_config_common(t);
> +   ip6gre_tnl_link_config_route(t, set_mtu, ip6erspan_calc_hlen(t));
> +}
> +
> +static int ip6erspan_tnl_change(struct ip6_tnl *t,
> +   const struct __ip6_tnl_parm *p, int set_mtu)
> +{
> +   ip6gre_tnl_copy_tnl_parm(t, p);
> +   ip6erspan_tnl_link_config(t, set_mtu);
> +   return 0;
> +}
> +
> +static int ip6erspan_changelink(struct net_device *dev, struct nlattr *tb[],
> +   struct nlattr *data[],
> +   struct netlink_ext_ack *extack)
> +{
> +   struct ip6gre_net *ign = net_generic(dev_net(dev), ip6gre_net_id);
> +   struct __ip6_tnl_parm p;
> +   struct ip6_tnl *t;
> +
> +   t = ip6gre_changelink_common(dev, tb, data, &p, extack);
> +   if (IS_ERR(t))
> +   return PTR_ERR(t);
> +
> +   ip6gre_tunnel_unlink(ign, t);
> +   ip6erspan_tnl_change(t, &p, !tb[IFLA_MTU]);
> +   ip6gre_tunnel_