diff mbox series

[RFC,net-next,v2,1/5] gtp: Allow to create GTP device without FDs

Message ID 20220127125725.125915-1-marcin.szycik@linux.intel.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series [RFC,net-next,v2,1/5] gtp: Allow to create GTP device without FDs | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter warning Series does not have a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 0 this patch: 2
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 0 this patch: 2
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 115 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Marcin Szycik Jan. 27, 2022, 12:57 p.m. UTC 
From: Wojciech Drewek <wojciech.drewek@intel.com>

Currently, when the user wants to create GTP device, he has to
provide file handles to the sockets created in userspace (IFLA_GTP_FD0,
IFLA_GTP_FD1). This behaviour is not ideal, considering the option of
adding support for GTP device creation through ip link. Ip link
application is not a good place to create such sockets.

This patch allows to create GTP device without providing
IFLA_GTP_FD0 and IFLA_GTP_FD1 arguments. If the user does not
provide file handles to the sockets, then GTP module takes care
of creating UDP sockets by itself. Sockets are created with the
commonly known UDP ports used for GTP protocol (GTP0_PORT and
GTP1U_PORT). In this case we don't have to provide encap_destroy
because no extra deinitialization is needed, everything is covered
by udp_tunnel_sock_release.

Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
---
 drivers/net/gtp.c | 74 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 67 insertions(+), 7 deletions(-)

Comments

Harald Welte Jan. 27, 2022, 2:40 p.m. UTC | #1 
Hi Wojciech,

thanks for your contribution, I think in general it is a good idea.

However, I do not think this can be merged, as the resulting system would
not be possible to use in a spec-compliant way.

> Currently, when the user wants to create GTP device, he has to
> provide file handles to the sockets created in userspace (IFLA_GTP_FD0,
> IFLA_GTP_FD1). This behaviour is not ideal, considering the option of
> adding support for GTP device creation through ip link. Ip link
> application is not a good place to create such sockets.

The GTP kernel module in its past and current form only handles G-PDU packets
and not any other packets.  So it relies on always having a user space process
[the one with the socket you want to make optional to handle other frames,
such as GTP ECHO.

So if you apply your patch, you will end up creating a GTP-U instance which
does not respond to echo requests, which is in violation of 3GPP specs and
which will create problems in production.

So if you want to make this optional, you'd also have to implement GTP-U ECHO handling
in the kernel, and require that in-kernel handling to be enabled when creating a GTP
device without the socket file descriptors.

Regards,
	Harald
Wojciech Drewek Jan. 27, 2022, 7:14 p.m. UTC | #2 
Hi Harald,

Thanks for the response,
I'll take a look on how GTP-U ECHO works and I'll try to implement it.

Regards,
Wojtek

> -----Original Message-----
> From: Harald Welte <laforge@gnumonks.org>
> Sent: czwartek, 27 stycznia 2022 15:41
> To: Marcin Szycik <marcin.szycik@linux.intel.com>
> Cc: netdev@vger.kernel.org; michal.swiatkowski@linux.intel.com; Drewek, Wojciech <wojciech.drewek@intel.com>;
> davem@davemloft.net; kuba@kernel.org; pablo@netfilter.org; osmocom-net-gprs@lists.osmocom.org
> Subject: Re: [RFC PATCH net-next v2 1/5] gtp: Allow to create GTP device without FDs
> 
> Hi Wojciech,
> 
> thanks for your contribution, I think in general it is a good idea.
> 
> However, I do not think this can be merged, as the resulting system would
> not be possible to use in a spec-compliant way.
> 
> > Currently, when the user wants to create GTP device, he has to
> > provide file handles to the sockets created in userspace (IFLA_GTP_FD0,
> > IFLA_GTP_FD1). This behaviour is not ideal, considering the option of
> > adding support for GTP device creation through ip link. Ip link
> > application is not a good place to create such sockets.
> 
> The GTP kernel module in its past and current form only handles G-PDU packets
> and not any other packets.  So it relies on always having a user space process
> [the one with the socket you want to make optional to handle other frames,
> such as GTP ECHO.
> 
> So if you apply your patch, you will end up creating a GTP-U instance which
> does not respond to echo requests, which is in violation of 3GPP specs and
> which will create problems in production.
> 
> So if you want to make this optional, you'd also have to implement GTP-U ECHO handling
> in the kernel, and require that in-kernel handling to be enabled when creating a GTP
> device without the socket file descriptors.
> 
> Regards,
> 	Harald
> 
> --
> - Harald Welte <laforge@gnumonks.org>           http://laforge.gnumonks.org/
> ============================================================================
> "Privacy in residential applications is a desirable marketing option."
>                                                   (ETSI EN 300 175-7 Ch. A6)
diff mbox series

Patch

diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index 24e5c54d06c1..a2ad0af913cb 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -66,8 +66,10 @@  struct gtp_dev {
 
 	struct sock		*sk0;
 	struct sock		*sk1u;
+	u8			sk_created;
 
 	struct net_device	*dev;
+	struct net		*net;
 
 	unsigned int		role;
 	unsigned int		hash_size;
@@ -320,8 +322,16 @@  static void gtp_encap_disable_sock(struct sock *sk)
 
 static void gtp_encap_disable(struct gtp_dev *gtp)
 {
-	gtp_encap_disable_sock(gtp->sk0);
-	gtp_encap_disable_sock(gtp->sk1u);
+	if (gtp->sk_created) {
+		udp_tunnel_sock_release(gtp->sk0->sk_socket);
+		udp_tunnel_sock_release(gtp->sk1u->sk_socket);
+		gtp->sk_created = false;
+		gtp->sk0 = NULL;
+		gtp->sk1u = NULL;
+	} else {
+		gtp_encap_disable_sock(gtp->sk0);
+		gtp_encap_disable_sock(gtp->sk1u);
+	}
 }
 
 /* UDP encapsulation receive handler. See net/ipv4/udp.c.
@@ -664,9 +674,6 @@  static int gtp_newlink(struct net *src_net, struct net_device *dev,
 	struct gtp_net *gn;
 	int hashsize, err;
 
-	if (!data[IFLA_GTP_FD0] && !data[IFLA_GTP_FD1])
-		return -EINVAL;
-
 	gtp = netdev_priv(dev);
 
 	if (!data[IFLA_GTP_PDP_HASHSIZE]) {
@@ -677,6 +684,8 @@  static int gtp_newlink(struct net *src_net, struct net_device *dev,
 			hashsize = 1024;
 	}
 
+	gtp->net = src_net;
+
 	err = gtp_hashtable_new(gtp, hashsize);
 	if (err < 0)
 		return err;
@@ -844,6 +853,38 @@  static struct sock *gtp_encap_enable_socket(int fd, int type,
 	return sk;
 }
 
+static struct sock *gtp_encap_create_sock(int type, struct gtp_dev *gtp)
+{
+	struct udp_tunnel_sock_cfg tuncfg = {};
+	struct udp_port_cfg udp_conf = {
+		.local_ip.s_addr	= htonl(INADDR_ANY),
+		.family			= AF_INET,
+	};
+	struct net *net = gtp->net;
+	struct socket *sock;
+	int err;
+
+	if (type == UDP_ENCAP_GTP0)
+		udp_conf.local_udp_port = GTP0_PORT;
+	else if (type == UDP_ENCAP_GTP1U)
+		udp_conf.local_udp_port = GTP1U_PORT;
+	else
+		return ERR_PTR(-EINVAL);
+
+	err = udp_sock_create(net, &udp_conf, &sock);
+	if (err)
+		return ERR_PTR(err);
+
+	tuncfg.sk_user_data = gtp;
+	tuncfg.encap_type = type;
+	tuncfg.encap_rcv = gtp_encap_recv;
+	tuncfg.encap_destroy = NULL;
+
+	setup_udp_tunnel_sock(net, sock, &tuncfg);
+
+	return sock->sk;
+}
+
 static int gtp_encap_enable(struct gtp_dev *gtp, struct nlattr *data[])
 {
 	struct sock *sk1u = NULL;
@@ -868,11 +909,30 @@  static int gtp_encap_enable(struct gtp_dev *gtp, struct nlattr *data[])
 		}
 	}
 
+	if (!data[IFLA_GTP_FD0] && !data[IFLA_GTP_FD1]) {
+		sk0 = gtp_encap_create_sock(UDP_ENCAP_GTP0, gtp);
+		if (IS_ERR(sk0))
+			return PTR_ERR(sk0);
+
+		sk1u = gtp_encap_create_sock(UDP_ENCAP_GTP1U, gtp);
+		if (IS_ERR(sk1u)) {
+			udp_tunnel_sock_release(sk0->sk_socket);
+			return PTR_ERR(sk1u);
+		}
+		gtp->sk_created = true;
+	}
+
 	if (data[IFLA_GTP_ROLE]) {
 		role = nla_get_u32(data[IFLA_GTP_ROLE]);
 		if (role > GTP_ROLE_SGSN) {
-			gtp_encap_disable_sock(sk0);
-			gtp_encap_disable_sock(sk1u);
+			if (gtp->sk_created) {
+				udp_tunnel_sock_release(sk0->sk_socket);
+				udp_tunnel_sock_release(sk1u->sk_socket);
+				gtp->sk_created = false;
+			} else {
+				gtp_encap_disable_sock(sk0);
+				gtp_encap_disable_sock(sk1u);
+			}
 			return -EINVAL;
 		}
 	}