Hi Pravin,
I'm going to submit a new series without the GTP_METADATA bits. I think
the whole "collect metadata" approach is fine, but the way GTP header
information is passed through the tunnel via metadata needs a bit more
thought. See below...
On 23/01/2021 20:59, Jonas Bonn wrote:
From: Pravin B Shelar <pbshe...@fb.com>
+static int gtp_set_tun_dst(struct pdp_ctx *pctx, struct sk_buff *skb,
+ unsigned int hdrlen)
+{
+ struct metadata_dst *tun_dst;
+ struct gtp1_header *gtp1;
+ int opts_len = 0;
+ __be64 tid;
+
+ gtp1 = (struct gtp1_header *)(skb->data + sizeof(struct udphdr));
+
+ tid = key32_to_tunnel_id(gtp1->tid);
+
+ if (unlikely(gtp1->flags & GTP1_F_MASK))
+ opts_len = sizeof(struct gtpu_metadata);
This decides that GTP metadata is required if any of the S, E, and PN
bits are set in the header. However:
i) even when any of those bits are set, none of the extra headers are
actually added to the metadata so it's somewhat pointless to even bother
reporting that they're set
ii) the more interesting case is that you might want to report reception
of an end marker through the tunnel; that however, is signalled by way
of the GTP header type and not via the flags; but, see below...
+
+ tun_dst = udp_tun_rx_dst(skb,
+ pctx->sk->sk_family, TUNNEL_KEY, tid, opts_len);
+ if (!tun_dst) {
+ netdev_dbg(pctx->dev, "Failed to allocate tun_dst");
+ goto err;
+ }
The problem, as I see it, is that end marker messages don't actually
contain an inner packet, so you won't be able to set up a destination
for them. The above fails and you never hit the metadata path below.
+
+ netdev_dbg(pctx->dev, "attaching metadata_dst to skb, gtp ver %d hdrlen
%d\n",
+ pctx->gtp_version, hdrlen);
+ if (unlikely(opts_len)) {
+ struct gtpu_metadata *opts;
+
+ opts = ip_tunnel_info_opts(&tun_dst->u.tun_info);
+ opts->ver = GTP_METADATA_V1;
+ opts->flags = gtp1->flags;
+ opts->type = gtp1->type;
+ netdev_dbg(pctx->dev, "recved control pkt: flag %x type: %d\n",
+ opts->flags, opts->type);
+ tun_dst->u.tun_info.key.tun_flags |= TUNNEL_GTPU_OPT;
+ tun_dst->u.tun_info.options_len = opts_len;
+ skb->protocol = htons(0xffff); /* Unknown */
+ }
Assuming that you do hit this code and are able to set the 'type' field
in the metadata, who is going to be the recipient. After you pull the
GTP headers, the SKB is presumably zero-length...
/Jonas
