On Sun, Jan 24, 2021 at 6:22 AM Jonas Bonn <jo...@norrbonn.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 <pbshe...@fb.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 <jo...@norrbonn.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",
> >
