diff mbox series

[PATCHv3,net-next,16/16] sctp: enable udp tunneling socks

Message ID afbaca39fa40eba694bd63c200050a49d8c8df81.1602574012.git.lucien.xin@gmail.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series sctp: Implement RFC6951: UDP Encapsulation of SCTP | expand

Commit Message

Xin Long Oct. 13, 2020, 7:27 a.m. UTC
This patch is to enable udp tunneling socks by calling
sctp_udp_sock_start() in sctp_ctrlsock_init(), and
sctp_udp_sock_stop() in sctp_ctrlsock_exit().

Also add sysctl udp_port to allow changing the listening
sock's port by users.

Wit this patch, the whole sctp over udp feature can be
enabled and used.

v1->v2:
  - Also update ctl_sock udp_port in proc_sctp_do_udp_port()
    where netns udp_port gets changed.
v2->v3:
  - call htons() when setting sk udp_port from netns udp_port.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/protocol.c |  5 +++++
 net/sctp/sysctl.c   | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

Comments

Marcelo Ricardo Leitner Oct. 15, 2020, 5:42 p.m. UTC | #1
Actually..

On Tue, Oct 13, 2020 at 03:27:41PM +0800, Xin Long wrote:
...
> Also add sysctl udp_port to allow changing the listening
> sock's port by users.
...
> ---
>  net/sctp/protocol.c |  5 +++++
>  net/sctp/sysctl.c   | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+)

Xin, sorry for not noticing this earlier, but we need a documentation
update here for this new sysctl. This is important. Please add its
entry in ip-sysctl.rst.

> 
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index be002b7..79fb4b5 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -1469,6 +1469,10 @@ static int __net_init sctp_ctrlsock_init(struct net *net)
>  	if (status)
>  		pr_err("Failed to initialize the SCTP control sock\n");
>  
> +	status = sctp_udp_sock_start(net);
> +	if (status)
> +		pr_err("Failed to initialize the SCTP udp tunneling sock\n");
                                                      ^^^ upper case please.
Nit. There are other occurrences of this.

> +
>  	return status;
...
> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
> +	if (write && ret == 0) {
> +		struct sock *sk = net->sctp.ctl_sock;
> +
> +		if (new_value > max || new_value < min)
> +			return -EINVAL;
> +
> +		net->sctp.udp_port = new_value;
> +		sctp_udp_sock_stop(net);

So, if it would be disabling the encapsulation, it shouldn't be
calling _start() then, right? Like

		if (new_value)
			ret = sctp_udp_sock_start(net);
		
Otherwise _start() here will call ..._bind() with port 0, which then
will be a random one.

> +		ret = sctp_udp_sock_start(net);
> +		if (ret)
> +			net->sctp.udp_port = 0;
> +
> +		/* Update the value in the control socket */
> +		lock_sock(sk);
> +		sctp_sk(sk)->udp_port = htons(net->sctp.udp_port);
> +		release_sock(sk);
> +	}
> +
> +	return ret;
> +}
> +
>  int sctp_sysctl_net_register(struct net *net)
>  {
>  	struct ctl_table *table;
> -- 
> 2.1.0
>
David Laight Oct. 15, 2020, 9:23 p.m. UTC | #2
From: Marcelo Ricardo Leitner
> Sent: 15 October 2020 18:43
> 
> Actually..
> 
> On Tue, Oct 13, 2020 at 03:27:41PM +0800, Xin Long wrote:
> ...
> > Also add sysctl udp_port to allow changing the listening
> > sock's port by users.

I've not read through this change...

But surely the UDP port (or ports) that you need to use
will depend on the remote system's configuration.

So they need to be a property of the socket, not a
system-wide value.

I can easily imagine my M3UA code connecting to different
network providers which have decided to use different
UDP port numbers.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Michael Tuexen Oct. 15, 2020, 9:35 p.m. UTC | #3
> On 15. Oct 2020, at 23:23, David Laight <David.Laight@ACULAB.COM> wrote:
> 
> From: Marcelo Ricardo Leitner
>> Sent: 15 October 2020 18:43
>> 
>> Actually..
>> 
>> On Tue, Oct 13, 2020 at 03:27:41PM +0800, Xin Long wrote:
>> ...
>>> Also add sysctl udp_port to allow changing the listening
>>> sock's port by users.
> 
> I've not read through this change...
> 
> But surely the UDP port (or ports) that you need to use
> will depend on the remote system's configuration.
The local encapsulation port can be system wide, the remote encapsulation port
is per remote address. See https://tools.ietf.org/html/rfc6951#section-5.1

Best regards
Michael
> 
> So they need to be a property of the socket, not a
> system-wide value.
> 
> I can easily imagine my M3UA code connecting to different
> network providers which have decided to use different
> UDP port numbers.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
Xin Long Oct. 16, 2020, 7:08 a.m. UTC | #4
On Fri, Oct 16, 2020 at 1:42 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> Actually..
>
> On Tue, Oct 13, 2020 at 03:27:41PM +0800, Xin Long wrote:
> ...
> > Also add sysctl udp_port to allow changing the listening
> > sock's port by users.
> ...
> > ---
> >  net/sctp/protocol.c |  5 +++++
> >  net/sctp/sysctl.c   | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 55 insertions(+)
>
> Xin, sorry for not noticing this earlier, but we need a documentation
> update here for this new sysctl. This is important. Please add its
> entry in ip-sysctl.rst.
no problem, I will add it.

>
> >
> > diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> > index be002b7..79fb4b5 100644
> > --- a/net/sctp/protocol.c
> > +++ b/net/sctp/protocol.c
> > @@ -1469,6 +1469,10 @@ static int __net_init sctp_ctrlsock_init(struct net *net)
> >       if (status)
> >               pr_err("Failed to initialize the SCTP control sock\n");
> >
> > +     status = sctp_udp_sock_start(net);
> > +     if (status)
> > +             pr_err("Failed to initialize the SCTP udp tunneling sock\n");
>                                                       ^^^ upper case please.
> Nit. There are other occurrences of this.
You mean we can remove this log, as it's been handled well in
sctp_udp_sock_start()?

>
> > +
> >       return status;
> ...
> > +     ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
> > +     if (write && ret == 0) {
> > +             struct sock *sk = net->sctp.ctl_sock;
> > +
> > +             if (new_value > max || new_value < min)
> > +                     return -EINVAL;
> > +
> > +             net->sctp.udp_port = new_value;
> > +             sctp_udp_sock_stop(net);
>
> So, if it would be disabling the encapsulation, it shouldn't be
> calling _start() then, right? Like
>
>                 if (new_value)
>                         ret = sctp_udp_sock_start(net);
>
> Otherwise _start() here will call ..._bind() with port 0, which then
> will be a random one.
right, somehow I thought it wouldn't bind with port 0.

Thanks.
>
> > +             ret = sctp_udp_sock_start(net);
> > +             if (ret)
> > +                     net->sctp.udp_port = 0;
> > +
> > +             /* Update the value in the control socket */
> > +             lock_sock(sk);
> > +             sctp_sk(sk)->udp_port = htons(net->sctp.udp_port);
> > +             release_sock(sk);
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> >  int sctp_sysctl_net_register(struct net *net)
> >  {
> >       struct ctl_table *table;
> > --
> > 2.1.0
> >
Xin Long Oct. 19, 2020, 10:40 a.m. UTC | #5
On Fri, Oct 16, 2020 at 3:08 PM Xin Long <lucien.xin@gmail.com> wrote:
>
> On Fri, Oct 16, 2020 at 1:42 AM Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> >
> > Actually..
> >
> > On Tue, Oct 13, 2020 at 03:27:41PM +0800, Xin Long wrote:
> > ...
> > > Also add sysctl udp_port to allow changing the listening
> > > sock's port by users.
> > ...
> > > ---
> > >  net/sctp/protocol.c |  5 +++++
> > >  net/sctp/sysctl.c   | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 55 insertions(+)
> >
> > Xin, sorry for not noticing this earlier, but we need a documentation
> > update here for this new sysctl. This is important. Please add its
> > entry in ip-sysctl.rst.
> no problem, I will add it.
>
> >
> > >
> > > diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> > > index be002b7..79fb4b5 100644
> > > --- a/net/sctp/protocol.c
> > > +++ b/net/sctp/protocol.c
> > > @@ -1469,6 +1469,10 @@ static int __net_init sctp_ctrlsock_init(struct net *net)
> > >       if (status)
> > >               pr_err("Failed to initialize the SCTP control sock\n");
> > >
> > > +     status = sctp_udp_sock_start(net);
> > > +     if (status)
> > > +             pr_err("Failed to initialize the SCTP udp tunneling sock\n");
> >                                                       ^^^ upper case please.
> > Nit. There are other occurrences of this.
> You mean we can remove this log, as it's been handled well in
> sctp_udp_sock_start()?
This one is actually OK :D
I've updated 'udp' with 'UDP' for all code annotations in this patchset.

will post v4.

Thanks.


>
> >
> > > +
> > >       return status;
> > ...
> > > +     ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
> > > +     if (write && ret == 0) {
> > > +             struct sock *sk = net->sctp.ctl_sock;
> > > +
> > > +             if (new_value > max || new_value < min)
> > > +                     return -EINVAL;
> > > +
> > > +             net->sctp.udp_port = new_value;
> > > +             sctp_udp_sock_stop(net);
> >
> > So, if it would be disabling the encapsulation, it shouldn't be
> > calling _start() then, right? Like
> >
> >                 if (new_value)
> >                         ret = sctp_udp_sock_start(net);
> >
> > Otherwise _start() here will call ..._bind() with port 0, which then
> > will be a random one.
> right, somehow I thought it wouldn't bind with port 0.
>
> Thanks.
> >
> > > +             ret = sctp_udp_sock_start(net);
> > > +             if (ret)
> > > +                     net->sctp.udp_port = 0;
> > > +
> > > +             /* Update the value in the control socket */
> > > +             lock_sock(sk);
> > > +             sctp_sk(sk)->udp_port = htons(net->sctp.udp_port);
> > > +             release_sock(sk);
> > > +     }
> > > +
> > > +     return ret;
> > > +}
> > > +
> > >  int sctp_sysctl_net_register(struct net *net)
> > >  {
> > >       struct ctl_table *table;
> > > --
> > > 2.1.0
> > >
diff mbox series

Patch

diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index be002b7..79fb4b5 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -1469,6 +1469,10 @@  static int __net_init sctp_ctrlsock_init(struct net *net)
 	if (status)
 		pr_err("Failed to initialize the SCTP control sock\n");
 
+	status = sctp_udp_sock_start(net);
+	if (status)
+		pr_err("Failed to initialize the SCTP udp tunneling sock\n");
+
 	return status;
 }
 
@@ -1476,6 +1480,7 @@  static void __net_exit sctp_ctrlsock_exit(struct net *net)
 {
 	/* Free the control endpoint.  */
 	inet_ctl_sock_destroy(net->sctp.ctl_sock);
+	sctp_udp_sock_stop(net);
 }
 
 static struct pernet_operations sctp_ctrlsock_ops = {
diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
index ecc1b5e..4395659 100644
--- a/net/sctp/sysctl.c
+++ b/net/sctp/sysctl.c
@@ -49,6 +49,8 @@  static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
 				void *buffer, size_t *lenp, loff_t *ppos);
 static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write, void *buffer,
 				size_t *lenp, loff_t *ppos);
+static int proc_sctp_do_udp_port(struct ctl_table *ctl, int write, void *buffer,
+				 size_t *lenp, loff_t *ppos);
 static int proc_sctp_do_alpha_beta(struct ctl_table *ctl, int write,
 				   void *buffer, size_t *lenp, loff_t *ppos);
 static int proc_sctp_do_auth(struct ctl_table *ctl, int write,
@@ -292,6 +294,15 @@  static struct ctl_table sctp_net_table[] = {
 		.proc_handler	= proc_dointvec,
 	},
 	{
+		.procname	= "udp_port",
+		.data		= &init_net.sctp.udp_port,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_sctp_do_udp_port,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= &udp_port_max,
+	},
+	{
 		.procname	= "encap_port",
 		.data		= &init_net.sctp.encap_port,
 		.maxlen		= sizeof(int),
@@ -487,6 +498,45 @@  static int proc_sctp_do_auth(struct ctl_table *ctl, int write,
 	return ret;
 }
 
+static int proc_sctp_do_udp_port(struct ctl_table *ctl, int write,
+				 void *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct net *net = current->nsproxy->net_ns;
+	unsigned int min = *(unsigned int *)ctl->extra1;
+	unsigned int max = *(unsigned int *)ctl->extra2;
+	struct ctl_table tbl;
+	int ret, new_value;
+
+	memset(&tbl, 0, sizeof(struct ctl_table));
+	tbl.maxlen = sizeof(unsigned int);
+
+	if (write)
+		tbl.data = &new_value;
+	else
+		tbl.data = &net->sctp.udp_port;
+
+	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
+	if (write && ret == 0) {
+		struct sock *sk = net->sctp.ctl_sock;
+
+		if (new_value > max || new_value < min)
+			return -EINVAL;
+
+		net->sctp.udp_port = new_value;
+		sctp_udp_sock_stop(net);
+		ret = sctp_udp_sock_start(net);
+		if (ret)
+			net->sctp.udp_port = 0;
+
+		/* Update the value in the control socket */
+		lock_sock(sk);
+		sctp_sk(sk)->udp_port = htons(net->sctp.udp_port);
+		release_sock(sk);
+	}
+
+	return ret;
+}
+
 int sctp_sysctl_net_register(struct net *net)
 {
 	struct ctl_table *table;