| 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 |
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 >
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)
> 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) >
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 > >
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 --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;
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(+)