Re: [PATCH net-next v1 2/3] gtp: Support creating flow-based gtp net_device

[Joe Stringer]

On 12 July 2017 at 17:44, Jiannan Ouyang <ouya...@fb.com> wrote:
> Add the gtp_create_flow_based_dev() interface to create flow-based gtp
> net_device, which sets gtp->collect_md. Under flow-based mode, UDP sockets are
> created and maintained in kernel.
>
> Signed-off-by: Jiannan Ouyang <ouya...@fb.com>
> ---

<snip>

> +static int gtp_configure(struct net *net, struct net_device *dev,
> +                        const struct ip_tunnel_info *info)
> +{
> +       struct gtp_net *gn = net_generic(net, gtp_net_id);
> +       struct gtp_dev *gtp = netdev_priv(dev);
> +       int err;
> +
> +       gtp->net = net;
> +       gtp->dev = dev;
> +
> +       if (gtp_find_flow_based_dev(net))
> +               return -EBUSY;
> +
> +       dev->netdev_ops         = &gtp_netdev_ops;
> +       dev->priv_destructor    = free_netdev;
> +
> +       dev->hard_header_len = 0;
> +       dev->addr_len = 0;
> +
> +       /* Zero header length. */
> +       dev->type = ARPHRD_NONE;
> +       dev->flags = IFF_POINTOPOINT | IFF_NOARP | IFF_MULTICAST;
> +
> +       dev->priv_flags |= IFF_NO_QUEUE;
> +       dev->features   |= NETIF_F_LLTX;
> +       netif_keep_dst(dev);
> +
> +       /* Assume largest header, ie. GTPv0. */
> +       dev->needed_headroom    = LL_MAX_HEADER +
> +               sizeof(struct iphdr) +
> +               sizeof(struct udphdr) +
> +               sizeof(struct gtp0_header);
> +
> +       dst_cache_reset(&gtp->info.dst_cache);
> +       gtp->info = *info;
> +       gtp->collect_md = true;
> +
> +       err = gtp_hashtable_new(gtp, GTP_PDP_HASHSIZE); // JO: when to free??
> +       if (err < 0) {
> +               pr_err("Error gtp_hashtable_new");
> +               return err;
> +       }

Firstly, if this succeeds then the subsequent register_netdevice()
call below could still fail, in which case the error handling there
free this hashtable. Don't forget that even if this whole function
succeeds, the caller may still fail to set up the device and in those
error cases everything needs to be freed/cleaned up as well.

Then in terms of the overall lifecycle, I guess it's a matter of 1) If
there is a hashtable allocated then it must be freed on device close;
2) If the device can be reconfigured, and there is already one of
these allocated then it would need to be freed upon reconfiguration.

Cheers,
Joe