Message ID | 20201105103946.18771-1-oss@malat.biz |
---|---|
State | Deferred |
Delegated to: | David Miller |
Headers | show |
Series | sctp: Fix sending when PMTU is less than SCTP_DEFAULT_MINSEGMENT | expand |
Context | Check | Description |
---|---|---|
jkicinski/cover_letter | success | Link |
jkicinski/fixes_present | success | Link |
jkicinski/patch_count | success | Link |
jkicinski/tree_selection | success | Guessed tree name to be net-next |
jkicinski/subject_prefix | warning | Target tree name not specified in the subject |
jkicinski/source_inline | success | Was 0 now: 0 |
jkicinski/verify_signedoff | success | Link |
jkicinski/module_param | success | Was 0 now: 0 |
jkicinski/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
jkicinski/verify_fixes | success | Link |
jkicinski/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 20 lines checked |
jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
jkicinski/header_inline | success | Link |
jkicinski/stable | success | Stable not CCed |
On Thu, Nov 05, 2020 at 11:39:47AM +0100, Petr Malat wrote: > Function sctp_dst_mtu() never returns lower MTU than > SCTP_TRUNC4(SCTP_DEFAULT_MINSEGMENT) even when the actual MTU is less, > in which case we rely on the IP fragmentation and must enable it. This should be being handled at sctp_packet_will_fit(): psize = packet->size; if (packet->transport->asoc) pmtu = packet->transport->asoc->pathmtu; else pmtu = packet->transport->pathmtu; /* Decide if we need to fragment or resubmit later. */ if (psize + chunk_len > pmtu) { /* It's OK to fragment at IP level if any one of the following * is true: * 1. The packet is empty (meaning this chunk is greater * the MTU) * 2. The packet doesn't have any data in it yet and data * requires authentication. */ if (sctp_packet_empty(packet) || (!packet->has_data && chunk->auth)) { /* We no longer do re-fragmentation. * Just fragment at the IP layer, if we * actually hit this condition */ packet->ipfragok = 1; goto out; } Why the above doesn't handle it already?
On Fri, Nov 06, 2020 at 05:46:34AM -0300, Marcelo Ricardo Leitner wrote: > On Thu, Nov 05, 2020 at 11:39:47AM +0100, Petr Malat wrote: > > Function sctp_dst_mtu() never returns lower MTU than > > SCTP_TRUNC4(SCTP_DEFAULT_MINSEGMENT) even when the actual MTU is less, > > in which case we rely on the IP fragmentation and must enable it. > > This should be being handled at sctp_packet_will_fit(): sctp_packet_will_fit() does something a little bit different, it allows fragmentation, if the packet must be longer than the pathmtu set in SCTP structures, which is never less than 512 (see sctp_dst_mtu()) even when the actual mtu is less than 512. One can test it by setting mtu of an interface to e.g. 300, and sending a longer packet (e.g. 400B): > psize = packet->size; > if (packet->transport->asoc) > pmtu = packet->transport->asoc->pathmtu; > else > pmtu = packet->transport->pathmtu; here the returned pmtu will be 512 > > /* Decide if we need to fragment or resubmit later. */ > if (psize + chunk_len > pmtu) { This branch will not be taken as the packet length is less then 512 > } > And the whole function will return SCTP_XMIT_OK without setting ipfragok. I think the idea of never going bellow 512 in sctp_dst_mtu() is to reduce overhead of SCTP headers, which is fine, but when we do that, we must be sure to allow the IP fragmentation, which is currently missing. The other option would be to keep track of the real MTU in pathmtu and perform max(512, pathmtu) in sctp_packet_will_fit() function. Not sure when exactly this got broken, but using MTU less than 512 used to work in 4.9. Petr
On Fri, Nov 06, 2020 at 10:48:24AM +0100, Petr Malat wrote: > On Fri, Nov 06, 2020 at 05:46:34AM -0300, Marcelo Ricardo Leitner wrote: > > On Thu, Nov 05, 2020 at 11:39:47AM +0100, Petr Malat wrote: > > > Function sctp_dst_mtu() never returns lower MTU than > > > SCTP_TRUNC4(SCTP_DEFAULT_MINSEGMENT) even when the actual MTU is less, > > > in which case we rely on the IP fragmentation and must enable it. > > > > This should be being handled at sctp_packet_will_fit(): > > sctp_packet_will_fit() does something a little bit different, it > allows fragmentation, if the packet must be longer than the pathmtu > set in SCTP structures, which is never less than 512 (see > sctp_dst_mtu()) even when the actual mtu is less than 512. > > One can test it by setting mtu of an interface to e.g. 300, > and sending a longer packet (e.g. 400B): > > psize = packet->size; > > if (packet->transport->asoc) > > pmtu = packet->transport->asoc->pathmtu; > > else > > pmtu = packet->transport->pathmtu; > here the returned pmtu will be 512 Thing is, your patch is using the same vars to check for it: + pmtu = tp->asoc ? tp->asoc->pathmtu : tp->pathmtu; > > > > > /* Decide if we need to fragment or resubmit later. */ > > if (psize + chunk_len > pmtu) { > This branch will not be taken as the packet length is less then 512 Right, ok. While then your patch will catch it because pmtu will be SCTP_DEFAULT_MINSEGMENT, as it is checking with '<='. > > > } > > > And the whole function will return SCTP_XMIT_OK without setting > ipfragok. > > I think the idea of never going bellow 512 in sctp_dst_mtu() is to > reduce overhead of SCTP headers, which is fine, but when we do that, > we must be sure to allow the IP fragmentation, which is currently > missing. Hmm. ip frag is probably just worse than higher header/payload overhead. > > The other option would be to keep track of the real MTU in pathmtu > and perform max(512, pathmtu) in sctp_packet_will_fit() function. I need to check where this 512 came from. I don't recall it from top of my head and it's from before git history. Maybe we should just drop this limit, if it's artificial. IPV4_MIN_MTU is 68. > > Not sure when exactly this got broken, but using MTU less than 512 > used to work in 4.9. Uhh, that's a bit old already. If you could narrow it down, that would be nice. Marcelo
On Fri, 6 Nov 2020 07:21:06 -0300 Marcelo Ricardo Leitner wrote: > On Fri, Nov 06, 2020 at 10:48:24AM +0100, Petr Malat wrote: > > On Fri, Nov 06, 2020 at 05:46:34AM -0300, Marcelo Ricardo Leitner wrote: > > > On Thu, Nov 05, 2020 at 11:39:47AM +0100, Petr Malat wrote: > > > > Function sctp_dst_mtu() never returns lower MTU than > > > > SCTP_TRUNC4(SCTP_DEFAULT_MINSEGMENT) even when the actual MTU is less, > > > > in which case we rely on the IP fragmentation and must enable it. > > > > > > This should be being handled at sctp_packet_will_fit(): > > > > sctp_packet_will_fit() does something a little bit different, it > > allows fragmentation, if the packet must be longer than the pathmtu > > set in SCTP structures, which is never less than 512 (see > > sctp_dst_mtu()) even when the actual mtu is less than 512. > > > > One can test it by setting mtu of an interface to e.g. 300, > > and sending a longer packet (e.g. 400B): > > > psize = packet->size; > > > if (packet->transport->asoc) > > > pmtu = packet->transport->asoc->pathmtu; > > > else > > > pmtu = packet->transport->pathmtu; > > here the returned pmtu will be 512 > > Thing is, your patch is using the same vars to check for it: > + pmtu = tp->asoc ? tp->asoc->pathmtu : tp->pathmtu; > > > > > > > > > /* Decide if we need to fragment or resubmit later. */ > > > if (psize + chunk_len > pmtu) { > > This branch will not be taken as the packet length is less then 512 > > Right, ok. While then your patch will catch it because pmtu will be > SCTP_DEFAULT_MINSEGMENT, as it is checking with '<='. > > > > > > } > > > > > And the whole function will return SCTP_XMIT_OK without setting > > ipfragok. > > > > I think the idea of never going bellow 512 in sctp_dst_mtu() is to > > reduce overhead of SCTP headers, which is fine, but when we do that, > > we must be sure to allow the IP fragmentation, which is currently > > missing. > > Hmm. ip frag is probably just worse than higher header/payload > overhead. > > > > > The other option would be to keep track of the real MTU in pathmtu > > and perform max(512, pathmtu) in sctp_packet_will_fit() function. > > I need to check where this 512 came from. I don't recall it from top > of my head and it's from before git history. Maybe we should just drop > this limit, if it's artificial. IPV4_MIN_MTU is 68. > > > > > Not sure when exactly this got broken, but using MTU less than 512 > > used to work in 4.9. > > Uhh, that's a bit old already. If you could narrow it down, that would > be nice. I'm dropping this from patchwork, if you conclude that the patch is good as is please repost, thanks!
diff --git a/net/sctp/output.c b/net/sctp/output.c index 1441eaf460bb..87a96cf6bfa4 100644 --- a/net/sctp/output.c +++ b/net/sctp/output.c @@ -552,6 +552,7 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp) struct sk_buff *head; struct sctphdr *sh; struct sock *sk; + u32 pmtu; pr_debug("%s: packet:%p\n", __func__, packet); if (list_empty(&packet->chunk_list)) @@ -559,6 +560,13 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp) chunk = list_entry(packet->chunk_list.next, struct sctp_chunk, list); sk = chunk->skb->sk; + /* Fragmentation on the IP level if the actual PMTU could be less + * than SCTP_DEFAULT_MINSEGMENT. See sctp_dst_mtu(). + */ + pmtu = tp->asoc ? tp->asoc->pathmtu : tp->pathmtu; + if (pmtu <= SCTP_DEFAULT_MINSEGMENT) + packet->ipfragok = 1; + /* check gso */ if (packet->size > tp->pathmtu && !packet->ipfragok) { if (!sk_can_gso(sk)) {
Function sctp_dst_mtu() never returns lower MTU than SCTP_TRUNC4(SCTP_DEFAULT_MINSEGMENT) even when the actual MTU is less, in which case we rely on the IP fragmentation and must enable it. Signed-off-by: Petr Malat <oss@malat.biz> --- net/sctp/output.c | 8 ++++++++ 1 file changed, 8 insertions(+)