Message-ID: <CAOrHB_DFv8_5CJ7GjUHT4qpyJUkgeWyX0KefYaZ-iZkz0UgaAQ@mail.gmail.com>
Date:   Thu, 28 Jan 2021 13:29:10 -0800
From:   Pravin Shelar <pravin.ovn@...il.com>
To:     Jonas Bonn <jonas@...rbonn.se>
Cc:     Harald Welte <laforge@...monks.org>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Pravin B Shelar <pbshelar@...com>,
        Jakub Kicinski <kuba@...nel.org>,
        Pablo Neira Ayuso <pablo@...filter.org>
Subject: Re: [RFC PATCH 15/16] gtp: add ability to send GTP controls headers

On Sun, Jan 24, 2021 at 6:22 AM Jonas Bonn <jonas@...rbonn.se> wrote:
>
> Hi Pravin,
>
> So, this whole GTP metadata thing has me a bit confused.
>
> You've defined a metadata structure like this:
>
> struct gtpu_metadata {
>          __u8    ver;
>          __u8    flags;
>          __u8    type;
> };
>
> Here ver is the version of the metadata structure itself, which is fine.
> 'flags' corresponds to the 3 flag bits of GTP header's first byte:  E,
> S, and PN.
> 'type' corresponds to the 'message type' field of the GTP header.
>
> The 'control header' (strange name) example below allows the flags to be
> set; however, setting these flags alone is insufficient because each one
> indicates the presence of additional fields in the header and there's
> nothing in the code to account for that.
>
> If E is set, extension headers would need to be added.
> If S is set, a sequence number field would need to be added.
> If PN is set, a PDU-number header would need to be added.
>
> It's not clear to me who sets up this metadata in the first place.  Is
> that where OVS or eBPF come in?  Can you give some pointers to how this
> works?
>

Receive path: LWT extracts tunnel metadata into tunnel-metadata
struct. This object has 5-tuple info from outer header and tunnel key.
When there is presence of extension header there is no way to store
the info standard tunnel-metadata object. That is when the optional
section of tunnel-metadata comes in the play.
As you can see the packet data from GTP header onwards is still pushed
to the device, so consumers of LWT can look at tunnel-metadata and
make sense of the inner packet that is received on the device.
OVS does exactly the same. When it receives a GTP packet with optional
metadata, it looks at flags and parses the inner packet and extension
header accordingly.

xmit path: it is set by LWT tunnel user, OVS or eBPF code. it needs to
set the metadata in tunnel dst along with the 5-tuple data and tunel
ID. This is how it can steer the packet at the right tunnel using a
single tunnel net device.

> Couple of comments below....
>
> On 23/01/2021 20:59, Jonas Bonn wrote:
> > From: Pravin B Shelar <pbshelar@...com>
> >
> > Please explain how this patch actually works... creation of the control
> > header makes sense, but I don't understand how sending of a
> > control header is actually triggered.
> >
> > Signed-off-by: Jonas Bonn <jonas@...rbonn.se>
> > ---
> >   drivers/net/gtp.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 42 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
> > index 668ed8a4836e..bbce2671de2d 100644
> > --- a/drivers/net/gtp.c
> > +++ b/drivers/net/gtp.c
> > @@ -683,6 +683,38 @@ static void gtp_push_header(struct sk_buff *skb, struct pdp_ctx *pctx,
> >       }
> >   }
> >
> > +static inline int gtp1_push_control_header(struct sk_buff *skb,
>
> I'm not enamored with the name 'control header' because it makes sound
> like this is some GTP-C thing.  The GTP module is really only about
> GTP-U and the function itself just sets up a GTP-U header.
>
sure. lets call ext_hdr.

>
> > +                                        struct pdp_ctx *pctx,
> > +                                        struct gtpu_metadata *opts,
> > +                                        struct net_device *dev)
> > +{
> > +     struct gtp1_header *gtp1c;
> > +     int payload_len;
> > +
> > +     if (opts->ver != GTP_METADATA_V1)
> > +             return -ENOENT;
> > +
> > +     if (opts->type == 0xFE) {
> > +             /* for end marker ignore skb data. */
> > +             netdev_dbg(dev, "xmit pkt with null data");
> > +             pskb_trim(skb, 0);
> > +     }
> > +     if (skb_cow_head(skb, sizeof(*gtp1c)) < 0)
> > +             return -ENOMEM;
> > +
> > +     payload_len = skb->len;
> > +
> > +     gtp1c = skb_push(skb, sizeof(*gtp1c));
> > +
> > +     gtp1c->flags    = opts->flags;
> > +     gtp1c->type     = opts->type;
> > +     gtp1c->length   = htons(payload_len);
> > +     gtp1c->tid      = htonl(pctx->u.v1.o_tei);
> > +     netdev_dbg(dev, "xmit control pkt: ver %d flags %x type %x pkt len %d tid %x",
> > +                opts->ver, opts->flags, opts->type, skb->len, pctx->u.v1.o_tei);
> > +     return 0;
> > +}
>
> There's nothing really special about that above function aside from the
> facts that it takes 'opts' as an argument.  Can't we just merge this
> with the regular 'gtp_push_header' function?  Do you have plans for this
> beyond what's here that would complicated by doing so?
>

Yes, we already have usecase for handling GTP PDU session container
related extension header for 5G UPF endpoitnt.



> /Jonas
>
>
> > +
> >   static int gtp_xmit_ip4(struct sk_buff *skb, struct net_device *dev)
> >   {
> >       struct gtp_dev *gtp = netdev_priv(dev);
> > @@ -807,7 +839,16 @@ static int gtp_xmit_ip4(struct sk_buff *skb, struct net_device *dev)
> >
> >       skb_set_inner_protocol(skb, skb->protocol);
> >
> > -     gtp_push_header(skb, pctx, &port);
> > +     if (unlikely(opts)) {
> > +             port = htons(GTP1U_PORT);
> > +             r = gtp1_push_control_header(skb, pctx, opts, dev);
> > +             if (r) {
> > +                     netdev_info(dev, "cntr pkt error %d", r);
> > +                     goto err_rt;
> > +             }
> > +     } else {
> > +             gtp_push_header(skb, pctx, &port);
> > +     }
> >
> >       iph = ip_hdr(skb);
> >       netdev_dbg(dev, "gtp -> IP src: %pI4 dst: %pI4\n",
> >

Powered by blists - more mailing lists