Re: [PATCH net] sctp: fix an issue that plpmtu can never go to complete state

On Thu, May 18, 2023 at 6:18 AM Paolo Abeni [off-list ref] wrote:

On Tue, 2023-05-16 at 21:22 -0400, Xin Long wrote:

quoted

When doing plpmtu probe, the probe size is growing every time when it
receives the ACK during the Search state until the probe fails. When
the failure occurs, pl.probe_high is set and it goes to the Complete
state.

However, if the link pmtu is huge, like 65535 in loopback_dev, the probe
eventually keeps using SCTP_MAX_PLPMTU as the probe size and never fails.
Because of that, pl.probe_high can not be set, and the plpmtu probe can
never go to the Complete state.

Fix it by setting pl.probe_high to SCTP_MAX_PLPMTU when the probe size
grows to SCTP_MAX_PLPMTU in sctp_transport_pl_recv(). Also, increase
the probe size only when the next is less than SCTP_MAX_PLPMTU.

Fixes: b87641aff9e7 ("sctp: do state transition when a probe succeeds on HB ACK recv path")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/transport.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index 2f66a2006517..b0ccfaa4c1d1 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c

@@ -324,9 +324,11 @@ bool sctp_transport_pl_recv(struct sctp_transport *t)
              t->pl.probe_size += SCTP_PL_BIG_STEP;
      } else if (t->pl.state == SCTP_PL_SEARCH) {
              if (!t->pl.probe_high) {
-                     t->pl.probe_size = min(t->pl.probe_size + SCTP_PL_BIG_STEP,
-                                            SCTP_MAX_PLPMTU);
-                     return false;
+                     if (t->pl.probe_size + SCTP_PL_BIG_STEP < SCTP_MAX_PLPMTU) {
+                             t->pl.probe_size += SCTP_PL_BIG_STEP;
+                             return false;
+                     }
+                     t->pl.probe_high = SCTP_MAX_PLPMTU;

It looks like this way the probed mtu can't reach SCTP_MAX_PLPMTU
anymore, while it was possible before.

indeed.

What about something alike:

                if (!t->pl.probe_high) {
                        if (t->pl.probe_size < SCTP_MAX_PLPMTU) {
                                t->pl.probe_size = min(t->pl.probe_size + SCTP_PL_BIG_STEP,
                                                       SCTP_MAX_PLPMTU);
                                return false;
                        }
                        t->pl.probe_high = SCTP_MAX_PLPMTU;

looks good.

will post v2.

Thanks.

quoted

              }
              t->pl.probe_size += SCTP_PL_MIN_STEP;
              if (t->pl.probe_size >= t->pl.probe_high) {

@@ -341,7 +343,8 @@ bool sctp_transport_pl_recv(struct sctp_transport *t)
      } else if (t->pl.state == SCTP_PL_COMPLETE) {
              /* Raise probe_size again after 30 * interval in Search Complete */
              t->pl.state = SCTP_PL_SEARCH; /* Search Complete -> Search */
-             t->pl.probe_size += SCTP_PL_MIN_STEP;
+             if (t->pl.probe_size + SCTP_PL_MIN_STEP < SCTP_MAX_PLPMTU)
+                     t->pl.probe_size += SCTP_PL_MIN_STEP;

In a similar way, should the above check be:

                if (t->pl.probe_size + SCTP_PL_MIN_STEP <= SCTP_MAX_PLPMTU)
                        t->pl.probe_size += SCTP_PL_MIN_STEP;

or simply:
                t->pl.probe_size = min(t->pl.probe_size + SCTP_PL_MIN_STEP, SCTP_MAX_PLPMTU)

quoted

Cheers,

Paolo