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]

Message-ID: <FFC0D5B5-37F4-4DD0-B294-FE3074EA9017@fb.com>
Date:   Fri, 14 Jul 2017 01:01:34 +0000
From:   Jiannan Ouyang <ouyangj@...com>
To:     Harald Welte <laforge@...monks.org>
CC:     "osmocom-net-gprs@...ts.osmocom.org" 
        <osmocom-net-gprs@...ts.osmocom.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "dev@...nvswitch.org" <dev@...nvswitch.org>,
        "pablo@...filter.org" <pablo@...filter.org>,
        "pshelar@...ira.com" <pshelar@...ira.com>,
        "wieger.ijntema.tno@...il.com" <wieger.ijntema.tno@...il.com>,
        "yi.y.yang@...el.com" <yi.y.yang@...el.com>,
        "joe@....org" <joe@....org>,
        Amar Padmanabhan <amarpadmanabhan@...com>
Subject: Re: [PATCH net-next v1 2/3] gtp: Support creating flow-based gtp
 net_device

Hi Harald,

> On 7/13/17, 12:35 AM, "Harald Welte" <laforge@...monks.org> wrote:
> 
> > +static int gtp_dev_open(struct net_device *dev)
> > +{
> > +	struct gtp_dev *gtp = netdev_priv(dev);
> > +	struct net *net = gtp->net;
> > +	struct socket *sock1u;
> > +	struct socket *sock0;
> > +	struct udp_tunnel_sock_cfg tunnel_cfg;
> > +	struct udp_port_cfg udp_conf;
> > +	int err;
> > +
> > +	memset(&udp_conf, 0, sizeof(udp_conf));
> > +
> > +	udp_conf.family = AF_INET;
> > +	udp_conf.local_ip.s_addr = htonl(INADDR_ANY);
> > +	udp_conf.local_udp_port = htons(GTP1U_PORT);
> > +
> > +	err = udp_sock_create(gtp->net, &udp_conf, &sock1u);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	udp_conf.local_udp_port = htons(GTP0_PORT);
> > +	err = udp_sock_create(gtp->net, &udp_conf, &sock0);
> > +	if (err < 0)
> > +		return err;
> 
> you're unconditionally binding to both GTP0 and GTP1 UDP ports.  This is
> done selectively based on netlink attributes in the existing "normal"
> non-OVS kernel code, i.e. the control is left to the user.
> 
> Is this function is only called/used in the context of OVS?  If so,
> since you explicitly implement only GTPv1, why bind to GTPv0 port?
> 

I had doubts on how this flow-based GTPv1 code path should fit in, which is why
the GTPv0 and the GTPv1 code pieces are mixed in my changes. Should I
explicitly claim that the flow-based change is for GTPv1 only?

> > +	setup_udp_tunnel_sock(net, sock1u, &tunnel_cfg);
> 
> even here, you're only setting up the v1 and not v0.
> 

same reason as above.

> > +	/* Assume largest header, ie. GTPv0. */
> > +	dev->needed_headroom	= LL_MAX_HEADER +
> > +		sizeof(struct iphdr) +
> > +		sizeof(struct udphdr) +
> > +		sizeof(struct gtp0_header);
> 
> ... and here you're using headroom for a GTPv0 header, despite (I think)
> only supporting GTPv1 from this configuration?

Yes, only GTPv1 is supported.

> 
> > +	err = gtp_hashtable_new(gtp, GTP_PDP_HASHSIZE); // JO: when to free??
> 
> I think that question about when to free needs to be resolved before any
> merge.  Did you check that it persists even after the device is
> closed/removed?

I didn't investigate it. What do you mean by persist?

Thanks
-Jiannan