netdev - Re: [RFC PATCH 15/16] gtp: add ability to send GTP controls headers

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: <bf6de363-8e32-aca0-1803-a041c0f55650@norrbonn.se>
Date:   Sun, 24 Jan 2021 15:21:21 +0100
From:   Jonas Bonn <jonas@...rbonn.se>
To:     laforge@...monks.org, netdev@...r.kernel.org, pbshelar@...com,
        kuba@...nel.org
Cc:     pablo@...filter.org
Subject: Re: [RFC PATCH 15/16] gtp: add ability to send GTP controls headers

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?

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.

> +					   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?

/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",
>