linux-sctp.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] sctp: Fix sending when PMTU is less than SCTP_DEFAULT_MINSEGMENT
@ 2020-11-05 10:39 Petr Malat
  2020-11-06  8:46 ` Marcelo Ricardo Leitner
  0 siblings, 1 reply; 5+ messages in thread
From: Petr Malat @ 2020-11-05 10:39 UTC (permalink / raw)
  To: linux-sctp
  Cc: Petr Malat, Vlad Yasevich, Neil Horman, Marcelo Ricardo Leitner,
	David S. Miller, Jakub Kicinski, netdev, linux-kernel

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] sctp: Fix sending when PMTU is less than SCTP_DEFAULT_MINSEGMENT
  2020-11-05 10:39 [PATCH] sctp: Fix sending when PMTU is less than SCTP_DEFAULT_MINSEGMENT Petr Malat
@ 2020-11-06  8:46 ` Marcelo Ricardo Leitner
  2020-11-06  9:48   ` Petr Malat
  0 siblings, 1 reply; 5+ messages in thread
From: Marcelo Ricardo Leitner @ 2020-11-06  8:46 UTC (permalink / raw)
  To: Petr Malat
  Cc: linux-sctp, Vlad Yasevich, Neil Horman, David S. Miller,
	Jakub Kicinski, netdev, linux-kernel

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?


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] sctp: Fix sending when PMTU is less than SCTP_DEFAULT_MINSEGMENT
  2020-11-06  8:46 ` Marcelo Ricardo Leitner
@ 2020-11-06  9:48   ` Petr Malat
  2020-11-06 10:21     ` Marcelo Ricardo Leitner
  0 siblings, 1 reply; 5+ messages in thread
From: Petr Malat @ 2020-11-06  9:48 UTC (permalink / raw)
  To: Marcelo Ricardo Leitner
  Cc: linux-sctp, Vlad Yasevich, Neil Horman, David S. Miller,
	Jakub Kicinski, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] sctp: Fix sending when PMTU is less than SCTP_DEFAULT_MINSEGMENT
  2020-11-06  9:48   ` Petr Malat
@ 2020-11-06 10:21     ` Marcelo Ricardo Leitner
  2020-11-09 22:57       ` Jakub Kicinski
  0 siblings, 1 reply; 5+ messages in thread
From: Marcelo Ricardo Leitner @ 2020-11-06 10:21 UTC (permalink / raw)
  To: Petr Malat
  Cc: linux-sctp, Vlad Yasevich, Neil Horman, David S. Miller,
	Jakub Kicinski, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] sctp: Fix sending when PMTU is less than SCTP_DEFAULT_MINSEGMENT
  2020-11-06 10:21     ` Marcelo Ricardo Leitner
@ 2020-11-09 22:57       ` Jakub Kicinski
  0 siblings, 0 replies; 5+ messages in thread
From: Jakub Kicinski @ 2020-11-09 22:57 UTC (permalink / raw)
  To: Marcelo Ricardo Leitner
  Cc: Petr Malat, linux-sctp, Vlad Yasevich, Neil Horman,
	David S. Miller, netdev, linux-kernel

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!

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-11-09 22:57 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-05 10:39 [PATCH] sctp: Fix sending when PMTU is less than SCTP_DEFAULT_MINSEGMENT Petr Malat
2020-11-06  8:46 ` Marcelo Ricardo Leitner
2020-11-06  9:48   ` Petr Malat
2020-11-06 10:21     ` Marcelo Ricardo Leitner
2020-11-09 22:57       ` Jakub Kicinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).