[RFC PATCH net-next v3 1/5] gtp: Allow to create GTP device without FDs
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(-) 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; } } -- 2.31.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. -- - 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)
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)
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 <laforge@osmocom.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
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.
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/consol... The same testsuite executed against master can be seen at https://jenkins.osmocom.org/jenkins/job/ttcn3-ggsn-test-kernel-latest-torval... [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)
-----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/consol...
The same testsuite executed against master can be seen at https://jenkins.osmocom.org/jenkins/job/ttcn3-ggsn-test-kernel-latest-torval... [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)
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! -- - 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)
participants (4)
-
Drewek, Wojciech -
Harald Welte -
Harald Welte -
Marcin Szycik