[net-next,2/4] gtp: add genl cmd to enable GTP encapsulation on UDP socket

Submitted by Andreas Schultz on March 14, 2017, 11:25 a.m.

Details

Message ID 20170314112548.24027-3-aschultz@tpip.net
State New
Series "gtp: support multiple APN's per GTP endpoint"
Headers show

Commit Message

Andreas Schultz March 14, 2017, 11:25 a.m.
Signed-off-by: Andreas Schultz <aschultz@tpip.net>
---
 drivers/net/gtp.c        | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/gtp.h |  4 ++++
 2 files changed, 51 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 66616f7..c4cf1b9 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -1244,6 +1244,45 @@  static int gtp_genl_dump_pdp(struct sk_buff *skb,
 	return skb->len;
 }
 
+static int gtp_genl_enable_socket(struct sk_buff *skb, struct genl_info *info)
+{
+	u32 version, fd, hashsize;
+	struct sock *sk;
+
+	if (!info->attrs[GTPA_VERSION] ||
+	    !info->attrs[GTPA_FD])
+		return -EINVAL;
+
+	if (!info->attrs[GTPA_PDP_HASHSIZE])
+		hashsize = 1024;
+	else
+		hashsize = nla_get_u32(info->attrs[IFLA_GTP_PDP_HASHSIZE]);
+
+	version = nla_get_u32(info->attrs[GTPA_VERSION]);
+	fd = nla_get_u32(info->attrs[GTPA_FD]);
+
+	switch (version) {
+	case GTP_V0:
+		sk = gtp_encap_enable_socket(fd, UDP_ENCAP_GTP0, hashsize);
+		break;
+
+	case GTP_V1:
+		sk = gtp_encap_enable_socket(fd, UDP_ENCAP_GTP1U, hashsize);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	if (!sk)
+		return -EINVAL;
+
+	if (IS_ERR(sk))
+		return PTR_ERR(sk);
+
+	return 0;
+}
+
 static struct nla_policy gtp_genl_policy[GTPA_MAX + 1] = {
 	[GTPA_LINK]		= { .type = NLA_U32, },
 	[GTPA_VERSION]		= { .type = NLA_U32, },
@@ -1254,6 +1293,8 @@  static struct nla_policy gtp_genl_policy[GTPA_MAX + 1] = {
 	[GTPA_NET_NS_FD]	= { .type = NLA_U32, },
 	[GTPA_I_TEI]		= { .type = NLA_U32, },
 	[GTPA_O_TEI]		= { .type = NLA_U32, },
+	[GTPA_PDP_HASHSIZE]	= { .type = NLA_U32, },
+	[GTPA_FD]		= { .type = NLA_U32, },
 };
 
 static const struct genl_ops gtp_genl_ops[] = {
@@ -1276,6 +1317,12 @@  static const struct genl_ops gtp_genl_ops[] = {
 		.policy = gtp_genl_policy,
 		.flags = GENL_ADMIN_PERM,
 	},
+	{
+		.cmd = GTP_CMD_ENABLE_SOCKET,
+		.doit = gtp_genl_enable_socket,
+		.policy = gtp_genl_policy,
+		.flags = GENL_ADMIN_PERM,
+	},
 };
 
 static struct genl_family gtp_genl_family __ro_after_init = {
diff --git a/include/uapi/linux/gtp.h b/include/uapi/linux/gtp.h
index 72a04a0..a9e9fe0 100644
--- a/include/uapi/linux/gtp.h
+++ b/include/uapi/linux/gtp.h
@@ -6,6 +6,8 @@  enum gtp_genl_cmds {
 	GTP_CMD_DELPDP,
 	GTP_CMD_GETPDP,
 
+	GTP_CMD_ENABLE_SOCKET,
+
 	GTP_CMD_MAX,
 };
 
@@ -26,6 +28,8 @@  enum gtp_attrs {
 	GTPA_I_TEI,	/* for GTPv1 only */
 	GTPA_O_TEI,	/* for GTPv1 only */
 	GTPA_PAD,
+	GTPA_PDP_HASHSIZE,
+	GTPA_FD,
 	__GTPA_MAX,
 };
 #define GTPA_MAX (__GTPA_MAX + 1)

Comments

Pablo Neira Ayuso March 14, 2017, 11:43 a.m.
On Tue, Mar 14, 2017 at 12:25:46PM +0100, Andreas Schultz wrote:

> @@ -1254,6 +1293,8 @@ static struct nla_policy gtp_genl_policy[GTPA_MAX + 1] = {
>  	[GTPA_NET_NS_FD]	= { .type = NLA_U32, },
>  	[GTPA_I_TEI]		= { .type = NLA_U32, },
>  	[GTPA_O_TEI]		= { .type = NLA_U32, },
> +	[GTPA_PDP_HASHSIZE]	= { .type = NLA_U32, },

This per PDP hashsize attribute clearly doesn't belong here.

Moreover, we now have a rhashtable implementation, so we hopefully we
can get rid of this. It should be very easy to convert this to use
rhashtable, and it is very much desiderable.

> +	[GTPA_FD]		= { .type = NLA_U32, },

This new atttribute has nothing to do with the PDP context.
And enum gtp_attrs *only* describe a PDP context. Adding more
attributes there to mix semantics is not the way to go.

You likely have to inaugurate a new enum. This gtp_attrs enum only
related to the PDP description.

Why not add some interface to attach more sockets to the gtp device
globally? So still the gtp device is the top-level structure. Then add
a netlink attribute to specify to what VRF this tunnel belongs to,
instead of implicitly using the socket to achieve this.

Another possibility is to explicitly have an interface to add
new/delete VRFs, attach sockets to them.

In general, I'm still not convinced this is the right design for this.
Andreas Schultz March 14, 2017, 12:28 p.m.
----- On Mar 14, 2017, at 12:43 PM, pablo pablo@netfilter.org wrote:

> On Tue, Mar 14, 2017 at 12:25:46PM +0100, Andreas Schultz wrote:
> 
>> @@ -1254,6 +1293,8 @@ static struct nla_policy gtp_genl_policy[GTPA_MAX + 1] = {
>>  	[GTPA_NET_NS_FD]	= { .type = NLA_U32, },
>>  	[GTPA_I_TEI]		= { .type = NLA_U32, },
>>  	[GTPA_O_TEI]		= { .type = NLA_U32, },
>> +	[GTPA_PDP_HASHSIZE]	= { .type = NLA_U32, },
> 
> This per PDP hashsize attribute clearly doesn't belong here.
> 
> Moreover, we now have a rhashtable implementation, so we hopefully we
> can get rid of this. It should be very easy to convert this to use
> rhashtable, and it is very much desiderable.

This would mean I have to mix the unrelated rhashtable change with moving the
hash into the socket. This certainly is not desirable either.

So, I'm going to have a look at the rhashtable thing and send a patch first
to convert the hashes to it.

>> +	[GTPA_FD]		= { .type = NLA_U32, },
> 
> This new atttribute has nothing to do with the PDP context.
> And enum gtp_attrs *only* describe a PDP context. Adding more
> attributes there to mix semantics is not the way to go.

You seem to assume that the network device or the APN/VRF is the root entity
for the GTP tunnels. That is IMHO wrong. The 3GPP specification clearly defines
a GTP entity that is completely Independent from an APN or the local IP endpoint.

A GTP entity serves multiple local IP endpoints, It manages outgoing tunnels
by the local APN/VRF, source IP, destination IP and remote tunnel id, incoming
tunnels are managed by the local destination IP, local tunnel id and VRF/APN.

Therefor a PDP context needs the following attributes:

 * local source/destination IP (and port - but that's for different series)
 * remote destination IP
 * local and remote TEID
 * VRF/APN

The local source and destination IP is implicitly contained in the socket, therefor
the socket needs to part of the context. The VRF/APN is contained in the network
device reference. So this also needs to part of the PDP context.

Having either the socket or the network device as the sole root container for a
GTP entity is wrong since the PDP context always refer both.

> You likely have to inaugurate a new enum. This gtp_attrs enum only
> related to the PDP description.
> 
> Why not add some interface to attach more sockets to the gtp device
> globally? So still the gtp device is the top-level structure. 

That is IMHO the wrong model. In a real live setup it likely to have a
few GTP sockets and possibly hundreds if not thousands of network device
attached to them (we already had the discussion why this kind of sharing
makes sense). 
So from a resource perspective alone, having the network device as root makes
no sense.

> Then add
> a netlink attribute to specify to what VRF this tunnel belongs to,
> instead of implicitly using the socket to achieve this.

You got that the wrong way arround. The VRF is already in the PDP context
through the network device reference. The socket is added to the PDP context
to select the outgoing source IP of the GTP tunnel in order to support
multiple GTP source IP's per GTP entity.

> Another possibility is to explicitly have an interface to add
> new/delete VRFs, attach sockets to them.

We already have that interface. It's the create a GTP network interface
genl API. I explained a few lines above why I think that adding sockets to
GTP network devices is wrong.

> In general, I'm still not convinced this is the right design for this.

Following your "add VRF" idea, I would end up with a pseudo network device
that represents a GTP entity. This would be the root instance for all the
VRF's and GTP sockets. Although being a network device, it would not
behave in any way like a network device, it would not handle traffic or
have IP(v6) addresses attached to it.
I would then further have GTP VRF network devices. Those would be "real"
network device that handle traffic and have IP addresses/route attached
to them.

I'm not sure if this pseudo GTP entity root device fits well with
other networking concepts. And more over, I can't really see the need
for such an construct.

This need for an in-kernel root entity seem to come the concept that
the kernel *owns* the tunnels and that tunnel a static and independent
from the user space control instance.

I think that the user space control instance should own the tunnels and
only use the kernel facility to manage them. When the user space instance
goes away, so should the tunnels.
From that perspective,  I want to keep the kernel facilities to the absolute
needed minimum.

Regards 
Andreas
Pablo Neira Ayuso March 14, 2017, 1:39 p.m.
On Tue, Mar 14, 2017 at 01:28:25PM +0100, Andreas Schultz wrote:
> ----- On Mar 14, 2017, at 12:43 PM, pablo pablo@netfilter.org wrote:
[...]
> A GTP entity serves multiple local IP endpoints, It manages outgoing tunnels
> by the local APN/VRF, source IP, destination IP and remote tunnel id, incoming
> tunnels are managed by the local destination IP, local tunnel id and VRF/APN.
> 
> Therefor a PDP context needs the following attributes:
> 
>  * local source/destination IP (and port - but that's for different series)
>  * remote destination IP
>  * local and remote TEID
>  * VRF/APN
[...]
> I'm not sure if this pseudo GTP entity root device fits well with
> other networking concepts. And more over, I can't really see the need
> for such an construct.

Some sort of top-level structure that wraps all these objects is
needed, and that can be a new VRF object itself.

You can add a netlink interface to add/dump/delete VRFs, this VRF
database would be *global* to the GSN. At VRF creation, you attach the
socket and the GTP device. You can share sockets between VRFs. PDP
context objects would be added to the corresponding VRF *not to the
socket*, but actually this will result in inserting this PDP context
into the socket hashtable and the GTP device hashtable.

We need to introduce a default VRF that is assumed to always exist,
and that userspace cannot remove, so things don't break backward. If
no VRF is specified, then we attach things to this default VRF.
Actually, look at this from a different angle: the existing driver is
just supporting *one single VRF* at this moment so we just have to
represent this explicitly. Breaking existing API is a no-go.

This explicit VRF concept would also allow us to dump PDP contexts
that belong to a given VRF, by simply indicating the VRF unique id.
Jamal already requested that we extend iproute2 to have command to
inspect the gtp driver we cannot escape this, we should allow
standalone tools to inspect the gtp datapath as we do with other
existing tunnel drivers, no matter what daemon in userspace implements
the control plane.

[...]
> I think that the user space control instance should own the tunnels and
> only use the kernel facility to manage them. When the user space instance
> goes away, so should the tunnels.

This doesn't allow daemon hot restart for whatever administrative
reason without affecting existing traffic. The kernel owns the
datapath indeed, that include tunnels.

> From that perspective, I want to keep the kernel facilities to the
> absolute needed minimum.

If some simple abstraction that we can insert makes this whole thing
more maintainable, then it makes sense to consider it.

This is all about not exposing the internal layout of the
representation you use for a very specific reason: The more you expose
internal details to userspace, the more problems we'll have to extend
things later on in the future. And don't try to be smart and say:
"Hey, I already know every usecase we will have in the future" because
that is not true.