diff mbox series

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

Message ID 20220127163900.374645-1-marcin.szycik@linux.intel.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series ice: GTP support in switchdev | 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 success Series has 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, 4:39 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 Feb. 5, 2022, 4:34 p.m. UTC | #1 
Hi Marcin, Wojciech,

thanks for the revised patch. In general it looks fine to me.

Do you have a public git tree with your patchset applied?  I'm asking as
we do have automatic testing in place at https://jenkins.osmocom.org/ where I
just need to specify a remote git repo andit will build this kernel and
run the test suite.

Some minor remarks below, all not critical, just some thoughts.

It might make sense to mention in the commit log that this patch by itself
would create GTP-U without GTP ECHO capabilities, and that a subsequent
patch will address this.

> 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. 

I'm wondering if we should make this more explicit, i.e. rather than
implicitly creating the kernel socket automagically, make this mode
explicit upon request by some netlink attribute.

> Sockets are created with the
> commonly known UDP ports used for GTP protocol (GTP0_PORT and
> GTP1U_PORT).

I'm wondering if there are use cases that need to operate on
non-standard ports.  The current module can be used that way (as the
socket is created in user space). If the "kernel socket mode" was
requested explicitly via netlink attribute, one could just as well
pass along the port number[s] this way.
Wojciech Drewek Feb. 8, 2022, 1:30 p.m. UTC | #2 
Hi Harald

Thanks for the response.

> -----Original Message-----
> From: Harald Welte <laforge@osmocom.org>
> Sent: sobota, 5 lutego 2022 17:34
> 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 v3 1/5] gtp: Allow to create GTP device without FDs
> 
> Hi Marcin, Wojciech,
> 
> thanks for the revised patch. In general it looks fine to me.
> 
> Do you have a public git tree with your patchset applied?  I'm asking as
> we do have automatic testing in place at https://jenkins.osmocom.org/ where I
> just need to specify a remote git repo andit will build this kernel and
> run the test suite.
For now we don't have such tree. I will see what we can do.

> 
> Some minor remarks below, all not critical, just some thoughts.
> 
> It might make sense to mention in the commit log that this patch by itself
> would create GTP-U without GTP ECHO capabilities, and that a subsequent
> patch will address this.
Sure, I'll add such comment.

> 
> > 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.
> 
> I'm wondering if we should make this more explicit, i.e. rather than
> implicitly creating the kernel socket automagically, make this mode
> explicit upon request by some netlink attribute.
I agree, it would look cleaner.

> 
> > Sockets are created with the
> > commonly known UDP ports used for GTP protocol (GTP0_PORT and
> > GTP1U_PORT).
> 
> I'm wondering if there are use cases that need to operate on
> non-standard ports.  The current module can be used that way (as the
> socket is created in user space). If the "kernel socket mode" was
> requested explicitly via netlink attribute, one could just as well
> pass along the port number[s] this way.
Yes, it is possible to create socket with any port number using FD approach,
but gtp module still assumes that ports are 2152 and 3386 at least in tx path
(see gtp_push_header).  Implementing this shouldn't be hard but is it crucial?

> 
> --
> - Harald Welte <laforge@osmocom.org>            http://laforge.gnumonks.org/
> ============================================================================
> "Privacy in residential applications is a desirable marketing option."
>                                                   (ETSI EN 300 175-7 Ch. A6)
Marcin Szycik Feb. 9, 2022, 6:04 p.m. UTC | #3 
Hi Harald,

Sorry for long delay in reply.

On 05-Feb-22 17:34, Harald Welte wrote:
> Hi Marcin, Wojciech,
> 
> thanks for the revised patch. In general it looks fine to me.
> 
> Do you have a public git tree with your patchset applied?  I'm asking as
> we do have automatic testing in place at https://jenkins.osmocom.org/ where I
> just need to specify a remote git repo andit will build this kernel and
> run the test suite.

I've created a public fork with our patchset applied, please see [1].

> 
> Some minor remarks below, all not critical, just some thoughts.
> 
> It might make sense to mention in the commit log that this patch by itself
> would create GTP-U without GTP ECHO capabilities, and that a subsequent
> patch will address this.
> 
>> 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. 
> 
> I'm wondering if we should make this more explicit, i.e. rather than
> implicitly creating the kernel socket automagically, make this mode
> explicit upon request by some netlink attribute.
> 
>> Sockets are created with the
>> commonly known UDP ports used for GTP protocol (GTP0_PORT and
>> GTP1U_PORT).
> 
> I'm wondering if there are use cases that need to operate on
> non-standard ports.  The current module can be used that way (as the
> socket is created in user space). If the "kernel socket mode" was
> requested explicitly via netlink attribute, one could just as well
> pass along the port number[s] this way.
> 

[1] https://github.com/mszycik/linux/tree/cpk_switchdev_gtp
Harald Welte Feb. 11, 2022, 9:04 a.m. UTC | #4 
Hi Wojciech,

On Tue, Feb 08, 2022 at 01:30:32PM +0000, Drewek, Wojciech wrote:

> For now we don't have such tree. I will see what we can do.

I would appreciate it, so we can get this tested before it hits net-next.

> > I'm wondering if we should make this more explicit, i.e. rather than
> > implicitly creating the kernel socket automagically, make this mode
> > explicit upon request by some netlink attribute.
>
> I agree, it would look cleaner.

Excellent.

> > > Sockets are created with the
> > > commonly known UDP ports used for GTP protocol (GTP0_PORT and
> > > GTP1U_PORT).
> > 
> > I'm wondering if there are use cases that need to operate on
> > non-standard ports.  The current module can be used that way (as the
> > socket is created in user space). If the "kernel socket mode" was
> > requested explicitly via netlink attribute, one could just as well
> > pass along the port number[s] this way.
>
> Yes, it is possible to create socket with any port number using FD approach,
> but gtp module still assumes that ports are 2152 and 3386 at least in tx path
> (see gtp_push_header).  Implementing this shouldn't be hard but is it crucial?

Not crucial.
Harald Welte Feb. 11, 2022, 9:11 a.m. UTC | #5 
Hi Marcin,

On Wed, Feb 09, 2022 at 07:04:01PM +0100, Marcin Szycik wrote:
> On 05-Feb-22 17:34, Harald Welte wrote:
> > Hi Marcin, Wojciech,
> > 
> > thanks for the revised patch. In general it looks fine to me.
> > 
> > Do you have a public git tree with your patchset applied?  I'm asking as
> > we do have automatic testing in place at https://jenkins.osmocom.org/ where I
> > just need to specify a remote git repo andit will build this kernel and
> > run the test suite.
> 
> I've created a public fork with our patchset applied, please see [1].

Thanks, I've triggered a build, let's hope it works out.  Results should
be at https://jenkins.osmocom.org/jenkins/job/ttcn3-ggsn-test-kernel-git/20/
and detailed logs at https://jenkins.osmocom.org/jenkins/job/ttcn3-ggsn-test-kernel-git/20/console

The same testsuite executed  against master can be seen at
https://jenkins.osmocom.org/jenkins/job/ttcn3-ggsn-test-kernel-latest-torvalds/
[the high amount of test cases failing is due to the lack of IPv6 support in the kernel GTP].

Let's hope your forked repo renders identical test results to upstream!

Regards,
	Harald
Wojciech Drewek Feb. 11, 2022, 10:05 a.m. UTC | #6 
> -----Original Message-----
> From: Harald Welte <laforge@osmocom.org>
> Sent: piątek, 11 lutego 2022 10:12
> 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 v3 1/5] gtp: Allow to create GTP device without FDs
> 
Hi Harald,

Thanks for triggering CI.
Do I see correctly that results for our changes are the same as for master?

Regards,
Wojtek
> Hi Marcin,
> 
> On Wed, Feb 09, 2022 at 07:04:01PM +0100, Marcin Szycik wrote:
> > On 05-Feb-22 17:34, Harald Welte wrote:
> > > Hi Marcin, Wojciech,
> > >
> > > thanks for the revised patch. In general it looks fine to me.
> > >
> > > Do you have a public git tree with your patchset applied?  I'm asking as
> > > we do have automatic testing in place at https://jenkins.osmocom.org/ where I
> > > just need to specify a remote git repo andit will build this kernel and
> > > run the test suite.
> >
> > I've created a public fork with our patchset applied, please see [1].
> 
> Thanks, I've triggered a build, let's hope it works out.  Results should
> be at https://jenkins.osmocom.org/jenkins/job/ttcn3-ggsn-test-kernel-git/20/
> and detailed logs at https://jenkins.osmocom.org/jenkins/job/ttcn3-ggsn-test-kernel-git/20/console
> 
> The same testsuite executed  against master can be seen at
> https://jenkins.osmocom.org/jenkins/job/ttcn3-ggsn-test-kernel-latest-torvalds/
> [the high amount of test cases failing is due to the lack of IPv6 support in the kernel GTP].
> 
> Let's hope your forked repo renders identical test results to upstream!
> 
> Regards,
> 	Harald
> --
> - Harald Welte <laforge@osmocom.org>            http://laforge.gnumonks.org/
> ============================================================================
> "Privacy in residential applications is a desirable marketing option."
>                                                   (ETSI EN 300 175-7 Ch. A6)
Harald Welte Feb. 12, 2022, 11:02 a.m. UTC | #7 
On Fri, Feb 11, 2022 at 10:05:54AM +0000, Drewek, Wojciech wrote:

> Thanks for triggering CI.
> Do I see correctly that results for our changes are the same as for master?

The results are identical to master/net-next, so all good!
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;
 		}
 	}