| Message ID | 20170314112548.24027-3-aschultz@tpip.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
----- 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
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.
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)
Signed-off-by: Andreas Schultz <aschultz@tpip.net> --- drivers/net/gtp.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/gtp.h | 4 ++++ 2 files changed, 51 insertions(+)