| Message ID | 7ff312f910ada8893fa4db57d341c628d1122640.1601387231.git.lucien.xin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | sctp: Implement RFC6951: UDP Encapsulation of SCTP | expand |
Hi Xin,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Xin-Long/sctp-Implement-RFC6951-UDP-Encapsulation-of-SCTP/20200929-215159
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 280095713ce244e8dbdfb059cdca695baa72230a
config: c6x-randconfig-r023-20200929 (attached as .config)
compiler: c6x-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/38f71b63a83947b86b356d8af1ba077027d27deb
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Xin-Long/sctp-Implement-RFC6951-UDP-Encapsulation-of-SCTP/20200929-215159
git checkout 38f71b63a83947b86b356d8af1ba077027d27deb
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=c6x
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from include/net/sctp/checksum.h:27,
from net/netfilter/nf_nat_proto.c:16:
include/net/sctp/sctp.h: In function 'sctp_mtu_payload':
>> include/net/sctp/sctp.h:583:31: error: 'struct net' has no member named 'sctp'; did you mean 'ct'?
583 | if (sock_net(&sp->inet.sk)->sctp.udp_port)
| ^~~~
| ct
vim +583 include/net/sctp/sctp.h
572
573 /* Calculate max payload size given a MTU, or the total overhead if
574 * given MTU is zero
575 */
576 static inline __u32 sctp_mtu_payload(const struct sctp_sock *sp,
577 __u32 mtu, __u32 extra)
578 {
579 __u32 overhead = sizeof(struct sctphdr) + extra;
580
581 if (sp) {
582 overhead += sp->pf->af->net_header_len;
> 583 if (sock_net(&sp->inet.sk)->sctp.udp_port)
584 overhead += sizeof(struct udphdr);
585 } else {
586 overhead += sizeof(struct ipv6hdr);
587 }
588
589 if (WARN_ON_ONCE(mtu && mtu <= overhead))
590 mtu = overhead;
591
592 return mtu ? mtu - overhead : overhead;
593 }
594
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Tue, Sep 29, 2020 at 09:49:03PM +0800, Xin Long wrote: > sctp_mtu_payload() is for calculating the frag size before making > chunks from a msg. So we should only add udphdr size to overhead > when udp socks are listening, as only then sctp can handling the "handle" ^^^^ > incoming sctp over udp packets and outgoing sctp over udp packets > will be possible. > > Note that we can't do this according to transport->encap_port, as > different transports may be set to different values, while the > chunks were made before choosing the transport, we could not be > able to meet all rfc6951#section-5.6 requires. I don't follow this last part. I guess you're referring to the fact that it won't grow back the PMTU if it is not encapsulating anymore. If that's it, then changelog should be different here. As is, it seems it is not abiding by the RFC, but it is, as that's a 'SHOULD'. Maybe s/requires\.$/recommends./ ?
On Wed, Sep 30, 2020 at 03:00:42AM +0800, kernel test robot wrote: > Hi Xin, > > Thank you for the patch! Yet something to improve: I wonder how are you planning to fix this. It is quite entangled. This is not performance critical. Maybe the cleanest way out is to move it to a .c file. Adding a #if defined(CONFIG_IP_SCTP) || defined(CONFIG_IP_SCTP_MODULE) in there doesn't seem good. > In file included from include/net/sctp/checksum.h:27, > from net/netfilter/nf_nat_proto.c:16: > include/net/sctp/sctp.h: In function 'sctp_mtu_payload': > >> include/net/sctp/sctp.h:583:31: error: 'struct net' has no member named 'sctp'; did you mean 'ct'? > 583 | if (sock_net(&sp->inet.sk)->sctp.udp_port) > | ^~~~ > | ct >
On Sat, Oct 3, 2020 at 12:07 PM Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > On Tue, Sep 29, 2020 at 09:49:03PM +0800, Xin Long wrote: > > sctp_mtu_payload() is for calculating the frag size before making > > chunks from a msg. So we should only add udphdr size to overhead > > when udp socks are listening, as only then sctp can handling the > "handle" ^^^^ right. :D > > incoming sctp over udp packets and outgoing sctp over udp packets > > will be possible. > > > > Note that we can't do this according to transport->encap_port, as > > different transports may be set to different values, while the > > chunks were made before choosing the transport, we could not be > > able to meet all rfc6951#section-5.6 requires. > > I don't follow this last part. I guess you're referring to the fact > that it won't grow back the PMTU if it is not encapsulating anymore. > If that's it, then changelog should be different here. As is, it > seems it is not abiding by the RFC, but it is, as that's a 'SHOULD'. > > Maybe s/requires\.$/recommends./ ? Yes, it's a "should". What the code can only do is "the Path MTU SHOULD be increased by the size of the UDP header" when udp listening port is disabled.
On Sat, Oct 3, 2020 at 12:08 PM Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > On Wed, Sep 30, 2020 at 03:00:42AM +0800, kernel test robot wrote: > > Hi Xin, > > > > Thank you for the patch! Yet something to improve: > > I wonder how are you planning to fix this. It is quite entangled. > This is not performance critical. Maybe the cleanest way out is to > move it to a .c file. > > Adding a > #if defined(CONFIG_IP_SCTP) || defined(CONFIG_IP_SCTP_MODULE) > in there doesn't seem good. > > > In file included from include/net/sctp/checksum.h:27, > > from net/netfilter/nf_nat_proto.c:16: > > include/net/sctp/sctp.h: In function 'sctp_mtu_payload': > > >> include/net/sctp/sctp.h:583:31: error: 'struct net' has no member named 'sctp'; did you mean 'ct'? > > 583 | if (sock_net(&sp->inet.sk)->sctp.udp_port) > > | ^~~~ > > | ct > > Here is actually another problem, I'm still thinking how to fix it. Now sctp_mtu_payload() returns different value depending on net->sctp.udp_port. but net->sctp.udp_port can be changed by "sysctl -w" anytime. so: In sctp_packet_config() it gets overhead/headroom by calling sctp_mtu_payload(). When 'udp_port' is 0, it's IP+MAC header size. Then if 'udp_port' is changed to 9899 by 'sysctl -w', udphdr will also be added to the packet in sctp_v4_xmit(), and later the headroom may not be enough for IP+MAC headers. I'm thinking to add sctp_sock->udp_port, and it'll be set when the sock is created with net->udp_port. but not sure if we should update sctp_sock->udp_port with net->udp_port when sending packets?
On Sat, Oct 3, 2020 at 4:12 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Sat, Oct 3, 2020 at 12:08 PM Marcelo Ricardo Leitner > <marcelo.leitner@gmail.com> wrote: > > > > On Wed, Sep 30, 2020 at 03:00:42AM +0800, kernel test robot wrote: > > > Hi Xin, > > > > > > Thank you for the patch! Yet something to improve: > > > > I wonder how are you planning to fix this. It is quite entangled. > > This is not performance critical. Maybe the cleanest way out is to > > move it to a .c file. > > > > Adding a > > #if defined(CONFIG_IP_SCTP) || defined(CONFIG_IP_SCTP_MODULE) > > in there doesn't seem good. > > > > > In file included from include/net/sctp/checksum.h:27, > > > from net/netfilter/nf_nat_proto.c:16: > > > include/net/sctp/sctp.h: In function 'sctp_mtu_payload': > > > >> include/net/sctp/sctp.h:583:31: error: 'struct net' has no member named 'sctp'; did you mean 'ct'? > > > 583 | if (sock_net(&sp->inet.sk)->sctp.udp_port) > > > | ^~~~ > > > | ct > > > > Here is actually another problem, I'm still thinking how to fix it. > > Now sctp_mtu_payload() returns different value depending on > net->sctp.udp_port. but net->sctp.udp_port can be changed by > "sysctl -w" anytime. so: > > In sctp_packet_config() it gets overhead/headroom by calling > sctp_mtu_payload(). When 'udp_port' is 0, it's IP+MAC header > size. Then if 'udp_port' is changed to 9899 by 'sysctl -w', > udphdr will also be added to the packet in sctp_v4_xmit(), > and later the headroom may not be enough for IP+MAC headers. > > I'm thinking to add sctp_sock->udp_port, and it'll be set when > the sock is created with net->udp_port. but not sure if we should > update sctp_sock->udp_port with net->udp_port when sending packets? something like: diff --git a/net/sctp/output.c b/net/sctp/output.c index 6614c9fdc51e..c379d805b9df 100644 --- a/net/sctp/output.c +++ b/net/sctp/output.c @@ -91,6 +91,7 @@ void sctp_packet_config(struct sctp_packet *packet, __u32 vtag, if (asoc) { sk = asoc->base.sk; sp = sctp_sk(sk); + sctp_sock_check_udp_port(sock_net(sk), sp, asoc); } packet->overhead = sctp_mtu_payload(sp, 0, 0); packet->size = packet->overhead; diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c index 21d0ff1c6ab9..f5aba9086d33 100644 --- a/net/sctp/sm_make_chunk.c +++ b/net/sctp/sm_make_chunk.c @@ -1250,6 +1250,7 @@ static inline struct sctp_chunk *sctp_make_op_error_limited( if (asoc) { size = min_t(size_t, size, asoc->pathmtu); sp = sctp_sk(asoc->base.sk); + sctp_sock_check_udp_port(sock_net(sk), sp, asoc); } size = sctp_mtu_payload(sp, size, sizeof(struct sctp_errhdr)); diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c index 8edab1533057..3e7f81d63d2e 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -6276,6 +6276,8 @@ static struct sctp_packet *sctp_ootb_pkt_new( sctp_transport_route(transport, (union sctp_addr *)&chunk->dest, sctp_sk(net->sctp.ctl_sock)); + sctp_sock_check_udp_port(net, net->sctp.ctl_sock, NULL); + packet = &transport->packet; sctp_packet_init(packet, transport, sport, dport); sctp_packet_config(packet, vtag, 0); diff --git a/net/sctp/socket.c b/net/sctp/socket.c index d793dfa94682..e5de5f98be0c 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -1550,6 +1550,17 @@ static int sctp_error(struct sock *sk, int flags, int err) return err; } +void sctp_sock_check_udp_port(struct net* net, struct sctp_sock *sp, + struct sctp_association *asoc) +{ + if (likely(sp->udp_port == net->sctp.udp_port)) + return; + + if (asoc && (!sp->udp_port || !net->sctp.udp_port)) + sctp_assoc_update_frag_point(asoc); + sp->udp_port = net->sctp.udp_port; +} + /* API 3.1.3 sendmsg() - UDP Style Syntax * * An application uses sendmsg() and recvmsg() calls to transmit data to @@ -1795,6 +1806,8 @@ static int sctp_sendmsg_to_asoc(struct sctp_association *asoc, goto err; } + sctp_sock_check_udp_port(net, sp, asoc); + if (sp->disable_fragments && msg_len > asoc->frag_point) { err = -EMSGSIZE; goto err;
On Sat, Oct 3, 2020 at 7:23 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Sat, Oct 3, 2020 at 4:12 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > On Sat, Oct 3, 2020 at 12:08 PM Marcelo Ricardo Leitner > > <marcelo.leitner@gmail.com> wrote: > > > > > > On Wed, Sep 30, 2020 at 03:00:42AM +0800, kernel test robot wrote: > > > > Hi Xin, > > > > > > > > Thank you for the patch! Yet something to improve: > > > > > > I wonder how are you planning to fix this. It is quite entangled. > > > This is not performance critical. Maybe the cleanest way out is to > > > move it to a .c file. > > > > > > Adding a > > > #if defined(CONFIG_IP_SCTP) || defined(CONFIG_IP_SCTP_MODULE) > > > in there doesn't seem good. > > > > > > > In file included from include/net/sctp/checksum.h:27, > > > > from net/netfilter/nf_nat_proto.c:16: > > > > include/net/sctp/sctp.h: In function 'sctp_mtu_payload': > > > > >> include/net/sctp/sctp.h:583:31: error: 'struct net' has no member named 'sctp'; did you mean 'ct'? > > > > 583 | if (sock_net(&sp->inet.sk)->sctp.udp_port) > > > > | ^~~~ > > > > | ct > > > > > > Here is actually another problem, I'm still thinking how to fix it. > > > > Now sctp_mtu_payload() returns different value depending on > > net->sctp.udp_port. but net->sctp.udp_port can be changed by > > "sysctl -w" anytime. so: > > > > In sctp_packet_config() it gets overhead/headroom by calling > > sctp_mtu_payload(). When 'udp_port' is 0, it's IP+MAC header > > size. Then if 'udp_port' is changed to 9899 by 'sysctl -w', > > udphdr will also be added to the packet in sctp_v4_xmit(), > > and later the headroom may not be enough for IP+MAC headers. > > > > I'm thinking to add sctp_sock->udp_port, and it'll be set when > > the sock is created with net->udp_port. but not sure if we should > > update sctp_sock->udp_port with net->udp_port when sending packets? > something like: > > diff --git a/net/sctp/output.c b/net/sctp/output.c > index 6614c9fdc51e..c379d805b9df 100644 > --- a/net/sctp/output.c > +++ b/net/sctp/output.c > @@ -91,6 +91,7 @@ void sctp_packet_config(struct sctp_packet *packet, > __u32 vtag, > if (asoc) { > sk = asoc->base.sk; > sp = sctp_sk(sk); > + sctp_sock_check_udp_port(sock_net(sk), sp, asoc); > } > packet->overhead = sctp_mtu_payload(sp, 0, 0); > packet->size = packet->overhead; > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c > index 21d0ff1c6ab9..f5aba9086d33 100644 > --- a/net/sctp/sm_make_chunk.c > +++ b/net/sctp/sm_make_chunk.c > @@ -1250,6 +1250,7 @@ static inline struct sctp_chunk > *sctp_make_op_error_limited( > if (asoc) { > size = min_t(size_t, size, asoc->pathmtu); > sp = sctp_sk(asoc->base.sk); > + sctp_sock_check_udp_port(sock_net(sk), sp, asoc); > } > > size = sctp_mtu_payload(sp, size, sizeof(struct sctp_errhdr)); > diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c > index 8edab1533057..3e7f81d63d2e 100644 > --- a/net/sctp/sm_statefuns.c > +++ b/net/sctp/sm_statefuns.c > @@ -6276,6 +6276,8 @@ static struct sctp_packet *sctp_ootb_pkt_new( > sctp_transport_route(transport, (union sctp_addr *)&chunk->dest, > sctp_sk(net->sctp.ctl_sock)); > > + sctp_sock_check_udp_port(net, net->sctp.ctl_sock, NULL); > + > packet = &transport->packet; > sctp_packet_init(packet, transport, sport, dport); > sctp_packet_config(packet, vtag, 0); > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index d793dfa94682..e5de5f98be0c 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -1550,6 +1550,17 @@ static int sctp_error(struct sock *sk, int > flags, int err) > return err; > } > > +void sctp_sock_check_udp_port(struct net* net, struct sctp_sock *sp, > + struct sctp_association *asoc) > +{ > + if (likely(sp->udp_port == net->sctp.udp_port)) > + return; > + > + if (asoc && (!sp->udp_port || !net->sctp.udp_port)) > + sctp_assoc_update_frag_point(asoc); > + sp->udp_port = net->sctp.udp_port; > +} > + > /* API 3.1.3 sendmsg() - UDP Style Syntax > * > * An application uses sendmsg() and recvmsg() calls to transmit data to > @@ -1795,6 +1806,8 @@ static int sctp_sendmsg_to_asoc(struct > sctp_association *asoc, > goto err; > } > > + sctp_sock_check_udp_port(net, sp, asoc); > + > if (sp->disable_fragments && msg_len > asoc->frag_point) { > err = -EMSGSIZE; > goto err; diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h index 6408bbb1b95d..86f74f2fe6de 100644 --- a/include/net/sctp/sctp.h +++ b/include/net/sctp/sctp.h @@ -580,7 +580,7 @@ static inline __u32 sctp_mtu_payload(const struct sctp_sock *sp, if (sp) { overhead += sp->pf->af->net_header_len; - if (sock_net(&sp->inet.sk)->sctp.udp_port) + if (sp->udp_port) overhead += sizeof(struct udphdr); } else { overhead += sizeof(struct ipv6hdr); diff --git a/net/sctp/output.c b/net/sctp/output.c index 6614c9fdc51e..c96b13ec72f4 100644 --- a/net/sctp/output.c +++ b/net/sctp/output.c @@ -91,6 +91,14 @@ void sctp_packet_config(struct sctp_packet *packet, __u32 vtag, if (asoc) { sk = asoc->base.sk; sp = sctp_sk(sk); + + if (unlikely(sp->udp_port != sock_net(sk)->sctp.udp_port)) { + __u16 port = sock_net(sk)->sctp.udp_port; + + if (!sp->udp_port || !port) + sctp_assoc_update_frag_point(asoc); + sp->udp_port = port; + } } diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c index 8edab1533057..8deb9d1554e9 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -6276,6 +6276,8 @@ static struct sctp_packet *sctp_ootb_pkt_new( sctp_transport_route(transport, (union sctp_addr *)&chunk->dest, sctp_sk(net->sctp.ctl_sock)); + sctp_sk(net->sctp.ctl_sock)->udp_port = net->sctp.udp_port; + packet = &transport->packet; sctp_packet_init(packet, transport, sport, dport); sctp_packet_config(packet, vtag, 0); Actually doing this is enough, more simple and clear.
On Sat, Oct 03, 2020 at 08:24:34PM +0800, Xin Long wrote: > On Sat, Oct 3, 2020 at 7:23 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > On Sat, Oct 3, 2020 at 4:12 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > On Sat, Oct 3, 2020 at 12:08 PM Marcelo Ricardo Leitner > > > <marcelo.leitner@gmail.com> wrote: > > > > > > > > On Wed, Sep 30, 2020 at 03:00:42AM +0800, kernel test robot wrote: > > > > > Hi Xin, > > > > > > > > > > Thank you for the patch! Yet something to improve: > > > > > > > > I wonder how are you planning to fix this. It is quite entangled. > > > > This is not performance critical. Maybe the cleanest way out is to > > > > move it to a .c file. > > > > > > > > Adding a > > > > #if defined(CONFIG_IP_SCTP) || defined(CONFIG_IP_SCTP_MODULE) > > > > in there doesn't seem good. > > > > > > > > > In file included from include/net/sctp/checksum.h:27, > > > > > from net/netfilter/nf_nat_proto.c:16: > > > > > include/net/sctp/sctp.h: In function 'sctp_mtu_payload': > > > > > >> include/net/sctp/sctp.h:583:31: error: 'struct net' has no member named 'sctp'; did you mean 'ct'? > > > > > 583 | if (sock_net(&sp->inet.sk)->sctp.udp_port) > > > > > | ^~~~ > > > > > | ct > > > > > > > > Here is actually another problem, I'm still thinking how to fix it. > > > > > > Now sctp_mtu_payload() returns different value depending on > > > net->sctp.udp_port. but net->sctp.udp_port can be changed by > > > "sysctl -w" anytime. so: Good point. > > > > > > In sctp_packet_config() it gets overhead/headroom by calling > > > sctp_mtu_payload(). When 'udp_port' is 0, it's IP+MAC header > > > size. Then if 'udp_port' is changed to 9899 by 'sysctl -w', > > > udphdr will also be added to the packet in sctp_v4_xmit(), > > > and later the headroom may not be enough for IP+MAC headers. > > > > > > I'm thinking to add sctp_sock->udp_port, and it'll be set when > > > the sock is created with net->udp_port. but not sure if we should > > > update sctp_sock->udp_port with net->udp_port when sending packets? I don't think so, > > something like: ... > diff --git a/net/sctp/output.c b/net/sctp/output.c > index 6614c9fdc51e..c96b13ec72f4 100644 > --- a/net/sctp/output.c > +++ b/net/sctp/output.c > @@ -91,6 +91,14 @@ void sctp_packet_config(struct sctp_packet *packet, > __u32 vtag, > if (asoc) { > sk = asoc->base.sk; > sp = sctp_sk(sk); > + > + if (unlikely(sp->udp_port != sock_net(sk)->sctp.udp_port)) { RFC6951 has: 6.1. Get or Set the Remote UDP Encapsulation Port Number (SCTP_REMOTE_UDP_ENCAPS_PORT) ... sue_assoc_id: This parameter is ignored for one-to-one style sockets. For one-to-many style sockets, the application may fill in an association identifier or SCTP_FUTURE_ASSOC for this query. It is an error to use SCTP_{CURRENT|ALL}_ASSOC in sue_assoc_id. sue_address: This specifies which address is of interest. If a wildcard address is provided, it applies only to future paths. So I'm not seeing a reason to have a system wide knob that takes effect in run time like this. Enable, start apps, and they keep behaving as initially configured. Need to disable? Restart the apps/sockets. Thoughts? > + __u16 port = sock_net(sk)->sctp.udp_port; > + > + if (!sp->udp_port || !port) > + sctp_assoc_update_frag_point(asoc); > + sp->udp_port = port; > + } > }
> On 5. Oct 2020, at 21:01, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > On Sat, Oct 03, 2020 at 08:24:34PM +0800, Xin Long wrote: >> On Sat, Oct 3, 2020 at 7:23 PM Xin Long <lucien.xin@gmail.com> wrote: >>> >>> On Sat, Oct 3, 2020 at 4:12 PM Xin Long <lucien.xin@gmail.com> wrote: >>>> >>>> On Sat, Oct 3, 2020 at 12:08 PM Marcelo Ricardo Leitner >>>> <marcelo.leitner@gmail.com> wrote: >>>>> >>>>> On Wed, Sep 30, 2020 at 03:00:42AM +0800, kernel test robot wrote: >>>>>> Hi Xin, >>>>>> >>>>>> Thank you for the patch! Yet something to improve: >>>>> >>>>> I wonder how are you planning to fix this. It is quite entangled. >>>>> This is not performance critical. Maybe the cleanest way out is to >>>>> move it to a .c file. >>>>> >>>>> Adding a >>>>> #if defined(CONFIG_IP_SCTP) || defined(CONFIG_IP_SCTP_MODULE) >>>>> in there doesn't seem good. >>>>> >>>>>> In file included from include/net/sctp/checksum.h:27, >>>>>> from net/netfilter/nf_nat_proto.c:16: >>>>>> include/net/sctp/sctp.h: In function 'sctp_mtu_payload': >>>>>>>> include/net/sctp/sctp.h:583:31: error: 'struct net' has no member named 'sctp'; did you mean 'ct'? >>>>>> 583 | if (sock_net(&sp->inet.sk)->sctp.udp_port) >>>>>> | ^~~~ >>>>>> | ct >>>>>> >>>> Here is actually another problem, I'm still thinking how to fix it. >>>> >>>> Now sctp_mtu_payload() returns different value depending on >>>> net->sctp.udp_port. but net->sctp.udp_port can be changed by >>>> "sysctl -w" anytime. so: > > Good point. > >>>> >>>> In sctp_packet_config() it gets overhead/headroom by calling >>>> sctp_mtu_payload(). When 'udp_port' is 0, it's IP+MAC header >>>> size. Then if 'udp_port' is changed to 9899 by 'sysctl -w', >>>> udphdr will also be added to the packet in sctp_v4_xmit(), >>>> and later the headroom may not be enough for IP+MAC headers. >>>> >>>> I'm thinking to add sctp_sock->udp_port, and it'll be set when >>>> the sock is created with net->udp_port. but not sure if we should >>>> update sctp_sock->udp_port with net->udp_port when sending packets? > > I don't think so, > >>> something like: > ... >> diff --git a/net/sctp/output.c b/net/sctp/output.c >> index 6614c9fdc51e..c96b13ec72f4 100644 >> --- a/net/sctp/output.c >> +++ b/net/sctp/output.c >> @@ -91,6 +91,14 @@ void sctp_packet_config(struct sctp_packet *packet, >> __u32 vtag, >> if (asoc) { >> sk = asoc->base.sk; >> sp = sctp_sk(sk); >> + >> + if (unlikely(sp->udp_port != sock_net(sk)->sctp.udp_port)) { > > RFC6951 has: > > 6.1. Get or Set the Remote UDP Encapsulation Port Number > (SCTP_REMOTE_UDP_ENCAPS_PORT) > ... > sue_assoc_id: This parameter is ignored for one-to-one style > sockets. For one-to-many style sockets, the application may fill > in an association identifier or SCTP_FUTURE_ASSOC for this query. > It is an error to use SCTP_{CURRENT|ALL}_ASSOC in sue_assoc_id. > > sue_address: This specifies which address is of interest. If a > wildcard address is provided, it applies only to future paths. > > So I'm not seeing a reason to have a system wide knob that takes > effect in run time like this. > Enable, start apps, and they keep behaving as initially configured. > Need to disable? Restart the apps/sockets. > > Thoughts? Just note about how things are implemented in FreeBSD. This is not about how it has to be implemented, but how it can be implemented. The local UDP encaps port is controlled by a system wide sysctl. sudo sysctl net.inet.sctp.udp_tunneling_port=9899 will use from that point on port 9899 the local encaps port. The local encaps port can't be controlled by the application. sudo sysctl net.inet.sctp.udp_tunneling_port=9900 will change this port number instantly to 9900. I don't see a use case for this, but it is possible. sudo sysctl net.inet.sctp.udp_tunneling_port=0 disables the sending and receiving of UDP encapsulated packets. The application can only control the remote encapsulation port on a per remote address base. This is mostly relevant for the client side wanting to use UDP encapsulation. Best regards Michael > >> + __u16 port = sock_net(sk)->sctp.udp_port; >> + >> + if (!sp->udp_port || !port) >> + sctp_assoc_update_frag_point(asoc); >> + sp->udp_port = port; >> + } >> }
On Tue, Oct 6, 2020 at 3:01 AM Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > On Sat, Oct 03, 2020 at 08:24:34PM +0800, Xin Long wrote: > > On Sat, Oct 3, 2020 at 7:23 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > On Sat, Oct 3, 2020 at 4:12 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > On Sat, Oct 3, 2020 at 12:08 PM Marcelo Ricardo Leitner > > > > <marcelo.leitner@gmail.com> wrote: > > > > > > > > > > On Wed, Sep 30, 2020 at 03:00:42AM +0800, kernel test robot wrote: > > > > > > Hi Xin, > > > > > > > > > > > > Thank you for the patch! Yet something to improve: > > > > > > > > > > I wonder how are you planning to fix this. It is quite entangled. > > > > > This is not performance critical. Maybe the cleanest way out is to > > > > > move it to a .c file. > > > > > > > > > > Adding a > > > > > #if defined(CONFIG_IP_SCTP) || defined(CONFIG_IP_SCTP_MODULE) > > > > > in there doesn't seem good. > > > > > > > > > > > In file included from include/net/sctp/checksum.h:27, > > > > > > from net/netfilter/nf_nat_proto.c:16: > > > > > > include/net/sctp/sctp.h: In function 'sctp_mtu_payload': > > > > > > >> include/net/sctp/sctp.h:583:31: error: 'struct net' has no member named 'sctp'; did you mean 'ct'? > > > > > > 583 | if (sock_net(&sp->inet.sk)->sctp.udp_port) > > > > > > | ^~~~ > > > > > > | ct > > > > > > > > > > Here is actually another problem, I'm still thinking how to fix it. > > > > > > > > Now sctp_mtu_payload() returns different value depending on > > > > net->sctp.udp_port. but net->sctp.udp_port can be changed by > > > > "sysctl -w" anytime. so: > > Good point. > > > > > > > > > In sctp_packet_config() it gets overhead/headroom by calling > > > > sctp_mtu_payload(). When 'udp_port' is 0, it's IP+MAC header > > > > size. Then if 'udp_port' is changed to 9899 by 'sysctl -w', > > > > udphdr will also be added to the packet in sctp_v4_xmit(), > > > > and later the headroom may not be enough for IP+MAC headers. > > > > > > > > I'm thinking to add sctp_sock->udp_port, and it'll be set when > > > > the sock is created with net->udp_port. but not sure if we should > > > > update sctp_sock->udp_port with net->udp_port when sending packets? > > I don't think so, > > > > something like: > ... > > diff --git a/net/sctp/output.c b/net/sctp/output.c > > index 6614c9fdc51e..c96b13ec72f4 100644 > > --- a/net/sctp/output.c > > +++ b/net/sctp/output.c > > @@ -91,6 +91,14 @@ void sctp_packet_config(struct sctp_packet *packet, > > __u32 vtag, > > if (asoc) { > > sk = asoc->base.sk; > > sp = sctp_sk(sk); > > + > > + if (unlikely(sp->udp_port != sock_net(sk)->sctp.udp_port)) { > > RFC6951 has: > > 6.1. Get or Set the Remote UDP Encapsulation Port Number > (SCTP_REMOTE_UDP_ENCAPS_PORT) > ... > sue_assoc_id: This parameter is ignored for one-to-one style > sockets. For one-to-many style sockets, the application may fill > in an association identifier or SCTP_FUTURE_ASSOC for this query. > It is an error to use SCTP_{CURRENT|ALL}_ASSOC in sue_assoc_id. > > sue_address: This specifies which address is of interest. If a > wildcard address is provided, it applies only to future paths. > > So I'm not seeing a reason to have a system wide knob that takes > effect in run time like this. > Enable, start apps, and they keep behaving as initially configured. > Need to disable? Restart the apps/sockets. > > Thoughts? Right, not to update it on tx path makes more sense. Thanks. > > > + __u16 port = sock_net(sk)->sctp.udp_port; > > + > > + if (!sp->udp_port || !port) > > + sctp_assoc_update_frag_point(asoc); > > + sp->udp_port = port; > > + } > > }
diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h index bfd87a0..6408bbb 100644 --- a/include/net/sctp/sctp.h +++ b/include/net/sctp/sctp.h @@ -578,10 +578,13 @@ static inline __u32 sctp_mtu_payload(const struct sctp_sock *sp, { __u32 overhead = sizeof(struct sctphdr) + extra; - if (sp) + if (sp) { overhead += sp->pf->af->net_header_len; - else + if (sock_net(&sp->inet.sk)->sctp.udp_port) + overhead += sizeof(struct udphdr); + } else { overhead += sizeof(struct ipv6hdr); + } if (WARN_ON_ONCE(mtu && mtu <= overhead)) mtu = overhead;
sctp_mtu_payload() is for calculating the frag size before making chunks from a msg. So we should only add udphdr size to overhead when udp socks are listening, as only then sctp can handling the incoming sctp over udp packets and outgoing sctp over udp packets will be possible. Note that we can't do this according to transport->encap_port, as different transports may be set to different values, while the chunks were made before choosing the transport, we could not be able to meet all rfc6951#section-5.6 requires. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- include/net/sctp/sctp.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)