netdev - Re: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap

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:   Fri, 14 Jul 2017 00:55:11 +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 1/3] gtp: refactor to support flow-based gtp
 encap and decap

Hi Harald,

> On 7/13/17, 12:26 AM, "Harald Welte" <laforge@...monks.org> wrote:
>·
> >  static inline void gtp_set_pktinfo_ipv4(struct gtp_pktinfo *pktinfo,
> >           struct sock *sk, struct iphdr *iph,
> > -         struct pdp_ctx *pctx, struct rtable *rt,
> > -         struct flowi4 *fl4,
> > +         struct rtable *rt, struct flowi4 *fl4,
> >           struct net_device *dev)
> >  {
> >  [...]
> > + __be32 tun_id;
>·
> you are breaking GTPv0 functionality here.  GTPv0 has 64 bit tunnel
> identifiers, and this function is called both from GTPv1 and GTPv0
> context.
>·
> This makes me wonder how you did verify that your changes do not break
> the existing operation with both GTPv0 and GTPv1?
>·

Good catch. I only fully tested the GTPv1 path against oai-cn. Will fix
this and test the GTPv0 path as well.

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? 

> > + // flow-based GTP1U encap
> > + info = skb_tunnel_info(skb);
> > + if (gtp->collect_md && info && ntohs(info->key.tp_dst) == GTP1U_PORT) {
>·
> I think it's typically safe to assume that GTP is only operated on
> standard ports, but it is something you chould/should think about, i.e.
> whether you want that kind of restriction.  In the existing use case, we
> have the v0/v1 information stored in the per-pdp context structure.
>·

The reason I’m checking GTP1U_PORT here is to filter GTP1U traffic. 
It possible to pass a port number from ovs into the gtp module. I will 
investigate how to support programmable port. 

> > +   tun_id  = htonl(pctx->u.v1.o_tei);
>·
> here is where you're assuming GTPv1 in two ways from code that is called
> from both v0 and v1.
> * you're dereferencing a v1 specific element in the pctx union
> * you're storing the result in a 32bit variable
>·

Right, will fix this for GTPv0.

> >   gtp = netdev_priv(dev);
> > + gtp->net = src_net;
>·
> Isn't this a generic change that's independent of your work on OVS GTP?

It is meant to be OVS independent. What makes it not? Should I leave 
this field un-initialized?

Thanks
-Jiannan