[PATCH] sctp: Fix sending when PMTU is less than SCTP_DEFAULT_MINSEGMENT

[PATCH] sctp: Fix sending when PMTU is less than SCTP_DEFAULT_MINSEGMENT

[Petr Malat]

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

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)) {
-- 
2.20.1

Re: [PATCH] sctp: Fix sending when PMTU is less than SCTP_DEFAULT_MINSEGMENT

[Marcelo Ricardo Leitner]

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?

Re: [PATCH] sctp: Fix sending when PMTU is less than SCTP_DEFAULT_MINSEGMENT

[Petr Malat]

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

Re: [PATCH] sctp: Fix sending when PMTU is less than SCTP_DEFAULT_MINSEGMENT

[Marcelo Ricardo Leitner]

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

Re: [PATCH] sctp: Fix sending when PMTU is less than SCTP_DEFAULT_MINSEGMENT

[Jakub Kicinski]

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!