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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 13 Jul 2017 11:01:01 -0700
From:   Joe Stringer <joe@....org>
To:     Jiannan Ouyang <ouyangj@...com>
Cc:     osmocom-net-gprs@...ts.osmocom.org,
        netdev <netdev@...r.kernel.org>, ovs dev <dev@...nvswitch.org>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Harald Welte <laforge@...monks.org>,
        Pravin B Shelar <pshelar@...ira.com>,
        Wieger IJntema <wieger.ijntema.tno@...il.com>,
        "Yang, Yi Y" <yi.y.yang@...el.com>,
        Amar Padmanabhan <amarpadmanabhan@...com>
Subject: Re: [PATCH net-next v1 2/3] gtp: Support creating flow-based gtp net_device

On 12 July 2017 at 17:44, Jiannan Ouyang <ouyangj@...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 <ouyangj@...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