[PATCH net] sctp: always set frag_point on pmtu change

[PATCH net] sctp: always set frag_point on pmtu change

[Jakub Audykowicz]

Calling send on a connected SCTP socket results in kernel panic if
spp_pathmtu was configured manually before an association is established
and it was not reconfigured to another value once the association is
established.

Steps to reproduce:
1. Set up a listening SCTP server socket.
2. Set up an SCTP client socket.
3. Configure client socket using setsockopt SCTP_PEER_ADDR_PARAMS with
    spp_pathmtu set to a legal value (e.g. 1000) and
    SPP_PMTUD_DISABLE set in spp_flags.
4. Connect client to server.
5. Send message from client to server.

At this point oom-killer is invoked, which will eventually lead to:
[    5.197262] Out of memory and no killable processes...
[    5.198107] Kernel panic - not syncing: System is deadlocked on memory

Commit 2f5e3c9df693 ("sctp: introduce sctp_assoc_update_frag_point")
introduces sctp_assoc_update_frag_point, but this function is not called
in this case, causing frag_point to be zero:
 void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)
 {
-	if (asoc->pathmtu != pmtu)
+	if (asoc->pathmtu != pmtu) {
 		asoc->pathmtu = pmtu;
+		sctp_assoc_update_frag_point(asoc);
+	}

In this scenario, on association establishment, asoc->pathmtu is already
1000 and pmtu will be as well. Before this commit the frag_point was being
correctly set in the scenario described. Moving the call outside the if
block fixes the issue.

I will be providing a separate patch to lksctp-tools with a simple test
reproducing this problem ("func_tests: frag_point should never be zero").

I have also taken the liberty to introduce a sanity check in chunk.c to
set the frag_point to a non-negative value in order to avoid chunking
endlessly (which is the reason for the eventual panic).

Fixes: 2f5e3c9df693 ("sctp: introduce sctp_assoc_update_frag_point")
Signed-off-by: Jakub Audykowicz <jakub.audykowicz@gmail.com>
---
 include/net/sctp/constants.h |  3 +++
 net/sctp/associola.c         | 13 +++++++------
 net/sctp/chunk.c             |  6 ++++++
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
index 8dadc74c22e7..90316fab6f04 100644
--- a/include/net/sctp/constants.h
+++ b/include/net/sctp/constants.h
@@ -293,6 +293,9 @@ enum { SCTP_MAX_GABS = 16 };
 					 */
 #define SCTP_DEFAULT_MINSEGMENT 512	/* MTU size ... if no mtu disc */
 
+/* An association's fragmentation point should never be non-positive */
+#define SCTP_FRAG_POINT_MIN 1
+
 #define SCTP_SECRET_SIZE 32		/* Number of octets in a 256 bits. */
 
 #define SCTP_SIGNATURE_SIZE 20	        /* size of a SLA-1 signature */
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 6a28b96e779e..44d71a1af62e 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1431,13 +1431,14 @@ void sctp_assoc_update_frag_point(struct sctp_association *asoc)
 
 void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)
 {
-	if (asoc->pathmtu != pmtu) {
-		asoc->pathmtu = pmtu;
-		sctp_assoc_update_frag_point(asoc);
-	}
+	pr_debug("%s: before asoc:%p, pmtu:%d, frag_point:%d\n",
+		__func__, asoc, asoc->pathmtu, asoc->frag_point);
+
+	asoc->pathmtu = pmtu;
+	sctp_assoc_update_frag_point(asoc);
 
-	pr_debug("%s: asoc:%p, pmtu:%d, frag_point:%d\n", __func__, asoc,
-		 asoc->pathmtu, asoc->frag_point);
+	pr_debug("%s: after asoc:%p, pmtu:%d, frag_point:%d\n",
+		__func__, asoc, asoc->pathmtu, asoc->frag_point);
 }
 
 /* Update the association's pmtu and frag_point by going through all the
diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
index ce8087846f05..9f0e64ddbd9c 100644
--- a/net/sctp/chunk.c
+++ b/net/sctp/chunk.c
@@ -190,6 +190,12 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
 	/* This is the biggest possible DATA chunk that can fit into
 	 * the packet
 	 */
+	if (asoc->frag_point < SCTP_FRAG_POINT_MIN) {
+		pr_warn("%s: asoc:%p->frag_point is less than allowed (%d<%d)",
+			__func__, asoc, asoc->frag_point, SCTP_FRAG_POINT_MIN);
+		pr_warn("forcing minimum value to avoid chunking endlessly");
+		asoc->frag_point = SCTP_FRAG_POINT_MIN;
+	}
 	max_data = asoc->frag_point;
 
 	/* If the the peer requested that we authenticate DATA chunks
-- 
2.17.1

Re: [PATCH net] sctp: always set frag_point on pmtu change

[Xin Long]

On Mon, Nov 19, 2018 at 5:49 AM Jakub Audykowicz
<jakub.audykowicz@gmail.com> wrote:
>
> Calling send on a connected SCTP socket results in kernel panic if
> spp_pathmtu was configured manually before an association is established
> and it was not reconfigured to another value once the association is
> established.
>
> Steps to reproduce:
> 1. Set up a listening SCTP server socket.
> 2. Set up an SCTP client socket.
> 3. Configure client socket using setsockopt SCTP_PEER_ADDR_PARAMS with
>     spp_pathmtu set to a legal value (e.g. 1000) and
>     SPP_PMTUD_DISABLE set in spp_flags.
> 4. Connect client to server.
> 5. Send message from client to server.
>
> At this point oom-killer is invoked, which will eventually lead to:
> [    5.197262] Out of memory and no killable processes...
> [    5.198107] Kernel panic - not syncing: System is deadlocked on memory
>
> Commit 2f5e3c9df693 ("sctp: introduce sctp_assoc_update_frag_point")
> introduces sctp_assoc_update_frag_point, but this function is not called
> in this case, causing frag_point to be zero:
>  void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)
>  {
> -       if (asoc->pathmtu != pmtu)
> +       if (asoc->pathmtu != pmtu) {
>                 asoc->pathmtu = pmtu;
> +               sctp_assoc_update_frag_point(asoc);
> +       }
>
> In this scenario, on association establishment, asoc->pathmtu is already
> 1000 and pmtu will be as well. Before this commit the frag_point was being
> correctly set in the scenario described. Moving the call outside the if
> block fixes the issue.
>
> I will be providing a separate patch to lksctp-tools with a simple test
> reproducing this problem ("func_tests: frag_point should never be zero").
>
> I have also taken the liberty to introduce a sanity check in chunk.c to
> set the frag_point to a non-negative value in order to avoid chunking
> endlessly (which is the reason for the eventual panic).
>
> Fixes: 2f5e3c9df693 ("sctp: introduce sctp_assoc_update_frag_point")
> Signed-off-by: Jakub Audykowicz <jakub.audykowicz@gmail.com>
> ---
>  include/net/sctp/constants.h |  3 +++
>  net/sctp/associola.c         | 13 +++++++------
>  net/sctp/chunk.c             |  6 ++++++
>  3 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
> index 8dadc74c22e7..90316fab6f04 100644
> --- a/include/net/sctp/constants.h
> +++ b/include/net/sctp/constants.h
> @@ -293,6 +293,9 @@ enum { SCTP_MAX_GABS = 16 };
>                                          */
>  #define SCTP_DEFAULT_MINSEGMENT 512    /* MTU size ... if no mtu disc */
>
> +/* An association's fragmentation point should never be non-positive */
> +#define SCTP_FRAG_POINT_MIN 1
> +
>  #define SCTP_SECRET_SIZE 32            /* Number of octets in a 256 bits. */
>
>  #define SCTP_SIGNATURE_SIZE 20         /* size of a SLA-1 signature */
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 6a28b96e779e..44d71a1af62e 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -1431,13 +1431,14 @@ void sctp_assoc_update_frag_point(struct sctp_association *asoc)
>
>  void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)
>  {
> -       if (asoc->pathmtu != pmtu) {
> -               asoc->pathmtu = pmtu;
> -               sctp_assoc_update_frag_point(asoc);
> -       }
> +       pr_debug("%s: before asoc:%p, pmtu:%d, frag_point:%d\n",
> +               __func__, asoc, asoc->pathmtu, asoc->frag_point);
> +
> +       asoc->pathmtu = pmtu;
> +       sctp_assoc_update_frag_point(asoc);
>
> -       pr_debug("%s: asoc:%p, pmtu:%d, frag_point:%d\n", __func__, asoc,
> -                asoc->pathmtu, asoc->frag_point);
> +       pr_debug("%s: after asoc:%p, pmtu:%d, frag_point:%d\n",
> +               __func__, asoc, asoc->pathmtu, asoc->frag_point);
>  }
The idea was whenever asoc->pathmtu changes,  frag_point should
be updated, but we missed one place in sctp_association_init().

Another issue is after 4-shakehand, the client's asoc->intl_enable
may be changed from 0 to 1, which means the frag_point should
also be updated, since [1]:

void sctp_assoc_update_frag_point(struct sctp_association *asoc)
{
        int frag = sctp_mtu_payload(sctp_sk(asoc->base.sk), asoc->pathmtu,
                                    sctp_datachk_len(&asoc->stream)); <--- [1]

So one fix for both issues is:

diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
index 0a78cdf..19d596d 100644
--- a/net/sctp/stream_interleave.c
+++ b/net/sctp/stream_interleave.c
@@ -1327,4 +1327,5 @@ void sctp_stream_interleave_init(struct
sctp_stream *stream)
        asoc = container_of(stream, struct sctp_association, stream);
        stream->si = asoc->intl_enable ? &sctp_stream_interleave_1
                                       : &sctp_stream_interleave_0;
+       sctp_assoc_update_frag_point(asoc);
 }


>
>  /* Update the association's pmtu and frag_point by going through all the
> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
> index ce8087846f05..9f0e64ddbd9c 100644
> --- a/net/sctp/chunk.c
> +++ b/net/sctp/chunk.c
> @@ -190,6 +190,12 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>         /* This is the biggest possible DATA chunk that can fit into
>          * the packet
>          */
> +       if (asoc->frag_point < SCTP_FRAG_POINT_MIN) {
> +               pr_warn("%s: asoc:%p->frag_point is less than allowed (%d<%d)",
> +                       __func__, asoc, asoc->frag_point, SCTP_FRAG_POINT_MIN);
> +               pr_warn("forcing minimum value to avoid chunking endlessly");
> +               asoc->frag_point = SCTP_FRAG_POINT_MIN;
> +       }
>         max_data = asoc->frag_point;
This won't happen if we sync frag_point on time like the above.

Re: [PATCH net] sctp: always set frag_point on pmtu change

[Jakub Audykowicz]

On 2018-11-19 08:20, Xin Long wrote:

> On Mon, Nov 19, 2018 at 5:49 AM Jakub Audykowicz
> <jakub.audykowicz@gmail.com> wrote:
>> Calling send on a connected SCTP socket results in kernel panic if
>> spp_pathmtu was configured manually before an association is established
>> and it was not reconfigured to another value once the association is
>> established.
>>
>> Steps to reproduce:
>> 1. Set up a listening SCTP server socket.
>> 2. Set up an SCTP client socket.
>> 3. Configure client socket using setsockopt SCTP_PEER_ADDR_PARAMS with
>>     spp_pathmtu set to a legal value (e.g. 1000) and
>>     SPP_PMTUD_DISABLE set in spp_flags.
>> 4. Connect client to server.
>> 5. Send message from client to server.
>>
>> At this point oom-killer is invoked, which will eventually lead to:
>> [    5.197262] Out of memory and no killable processes...
>> [    5.198107] Kernel panic - not syncing: System is deadlocked on memory
>>
>> Commit 2f5e3c9df693 ("sctp: introduce sctp_assoc_update_frag_point")
>> introduces sctp_assoc_update_frag_point, but this function is not called
>> in this case, causing frag_point to be zero:
>>  void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)
>>  {
>> -       if (asoc->pathmtu != pmtu)
>> +       if (asoc->pathmtu != pmtu) {
>>                 asoc->pathmtu = pmtu;
>> +               sctp_assoc_update_frag_point(asoc);
>> +       }
>>
>> In this scenario, on association establishment, asoc->pathmtu is already
>> 1000 and pmtu will be as well. Before this commit the frag_point was being
>> correctly set in the scenario described. Moving the call outside the if
>> block fixes the issue.
>>
>> I will be providing a separate patch to lksctp-tools with a simple test
>> reproducing this problem ("func_tests: frag_point should never be zero").
>>
>> I have also taken the liberty to introduce a sanity check in chunk.c to
>> set the frag_point to a non-negative value in order to avoid chunking
>> endlessly (which is the reason for the eventual panic).
>>
>> Fixes: 2f5e3c9df693 ("sctp: introduce sctp_assoc_update_frag_point")
>> Signed-off-by: Jakub Audykowicz <jakub.audykowicz@gmail.com>
>> ---
>>  include/net/sctp/constants.h |  3 +++
>>  net/sctp/associola.c         | 13 +++++++------
>>  net/sctp/chunk.c             |  6 ++++++
>>  3 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>> index 8dadc74c22e7..90316fab6f04 100644
>> --- a/include/net/sctp/constants.h
>> +++ b/include/net/sctp/constants.h
>> @@ -293,6 +293,9 @@ enum { SCTP_MAX_GABS = 16 };
>>                                          */
>>  #define SCTP_DEFAULT_MINSEGMENT 512    /* MTU size ... if no mtu disc */
>>
>> +/* An association's fragmentation point should never be non-positive */
>> +#define SCTP_FRAG_POINT_MIN 1
>> +
>>  #define SCTP_SECRET_SIZE 32            /* Number of octets in a 256 bits. */
>>
>>  #define SCTP_SIGNATURE_SIZE 20         /* size of a SLA-1 signature */
>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>> index 6a28b96e779e..44d71a1af62e 100644
>> --- a/net/sctp/associola.c
>> +++ b/net/sctp/associola.c
>> @@ -1431,13 +1431,14 @@ void sctp_assoc_update_frag_point(struct sctp_association *asoc)
>>
>>  void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)
>>  {
>> -       if (asoc->pathmtu != pmtu) {
>> -               asoc->pathmtu = pmtu;
>> -               sctp_assoc_update_frag_point(asoc);
>> -       }
>> +       pr_debug("%s: before asoc:%p, pmtu:%d, frag_point:%d\n",
>> +               __func__, asoc, asoc->pathmtu, asoc->frag_point);
>> +
>> +       asoc->pathmtu = pmtu;
>> +       sctp_assoc_update_frag_point(asoc);
>>
>> -       pr_debug("%s: asoc:%p, pmtu:%d, frag_point:%d\n", __func__, asoc,
>> -                asoc->pathmtu, asoc->frag_point);
>> +       pr_debug("%s: after asoc:%p, pmtu:%d, frag_point:%d\n",
>> +               __func__, asoc, asoc->pathmtu, asoc->frag_point);
>>  }
> The idea was whenever asoc->pathmtu changes,  frag_point should
> be updated, but we missed one place in sctp_association_init().
>
> Another issue is after 4-shakehand, the client's asoc->intl_enable
> may be changed from 0 to 1, which means the frag_point should
> also be updated, since [1]:
>
> void sctp_assoc_update_frag_point(struct sctp_association *asoc)
> {
>         int frag = sctp_mtu_payload(sctp_sk(asoc->base.sk), asoc->pathmtu,
>                                     sctp_datachk_len(&asoc->stream)); <--- [1]
>
> So one fix for both issues is:
>
> diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
> index 0a78cdf..19d596d 100644
> --- a/net/sctp/stream_interleave.c
> +++ b/net/sctp/stream_interleave.c
> @@ -1327,4 +1327,5 @@ void sctp_stream_interleave_init(struct
> sctp_stream *stream)
>         asoc = container_of(stream, struct sctp_association, stream);
>         stream->si = asoc->intl_enable ? &sctp_stream_interleave_1
>                                        : &sctp_stream_interleave_0;
> +       sctp_assoc_update_frag_point(asoc);
>  }
>
>
>>  /* Update the association's pmtu and frag_point by going through all the
>> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
>> index ce8087846f05..9f0e64ddbd9c 100644
>> --- a/net/sctp/chunk.c
>> +++ b/net/sctp/chunk.c
>> @@ -190,6 +190,12 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>>         /* This is the biggest possible DATA chunk that can fit into
>>          * the packet
>>          */
>> +       if (asoc->frag_point < SCTP_FRAG_POINT_MIN) {
>> +               pr_warn("%s: asoc:%p->frag_point is less than allowed (%d<%d)",
>> +                       __func__, asoc, asoc->frag_point, SCTP_FRAG_POINT_MIN);
>> +               pr_warn("forcing minimum value to avoid chunking endlessly");
>> +               asoc->frag_point = SCTP_FRAG_POINT_MIN;
>> +       }
>>         max_data = asoc->frag_point;
> This won't happen if we sync frag_point on time like the above.

I know a better fix has been proposed and acked already but I would like to 
follow this up a bit. I still think there is some value to the changes in this 
patch. First of all, I am not sure the if statement in sctp_assoc_set_pmtu is 
of any practical benefit. I assume its intention is optimization, but I'm 
skeptical. Originally it made little sense, since it's not like assigning the 
same value would be incorrect or costly. Upon introducing 
sctp_assoc_update_frag_point I can see the if being of more use since it is 
theoretically useless to call this function if MTU is the same, but as it 
turns out it might still be useful at what I would estimate to be a negligible 
cost. I propose to do away with this if block.

As for the code in chunk, I'm not too proud of this hacky workaround, but 
I still think there should be some way to avoid chunking endlessly and running 
OOM if the configuration is wrong (here due to an implementation oversight), 
a last-ditch fail-safe of sorts. How do you guys think this could be 
accomplished?

Re: [PATCH net] sctp: always set frag_point on pmtu change

[Marcelo Ricardo Leitner]

On Tue, Nov 27, 2018 at 11:18:02PM +0100, Jakub Audykowicz wrote:
> On 2018-11-19 08:20, Xin Long wrote:
> 
> > On Mon, Nov 19, 2018 at 5:49 AM Jakub Audykowicz
> > <jakub.audykowicz@gmail.com> wrote:
> >> Calling send on a connected SCTP socket results in kernel panic if
> >> spp_pathmtu was configured manually before an association is established
> >> and it was not reconfigured to another value once the association is
> >> established.
> >>
> >> Steps to reproduce:
> >> 1. Set up a listening SCTP server socket.
> >> 2. Set up an SCTP client socket.
> >> 3. Configure client socket using setsockopt SCTP_PEER_ADDR_PARAMS with
> >>     spp_pathmtu set to a legal value (e.g. 1000) and
> >>     SPP_PMTUD_DISABLE set in spp_flags.
> >> 4. Connect client to server.
> >> 5. Send message from client to server.
> >>
> >> At this point oom-killer is invoked, which will eventually lead to:
> >> [    5.197262] Out of memory and no killable processes...
> >> [    5.198107] Kernel panic - not syncing: System is deadlocked on memory
> >>
> >> Commit 2f5e3c9df693 ("sctp: introduce sctp_assoc_update_frag_point")
> >> introduces sctp_assoc_update_frag_point, but this function is not called
> >> in this case, causing frag_point to be zero:
> >>  void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)
> >>  {
> >> -       if (asoc->pathmtu != pmtu)
> >> +       if (asoc->pathmtu != pmtu) {
> >>                 asoc->pathmtu = pmtu;
> >> +               sctp_assoc_update_frag_point(asoc);
> >> +       }
> >>
> >> In this scenario, on association establishment, asoc->pathmtu is already
> >> 1000 and pmtu will be as well. Before this commit the frag_point was being
> >> correctly set in the scenario described. Moving the call outside the if
> >> block fixes the issue.
> >>
> >> I will be providing a separate patch to lksctp-tools with a simple test
> >> reproducing this problem ("func_tests: frag_point should never be zero").
> >>
> >> I have also taken the liberty to introduce a sanity check in chunk.c to
> >> set the frag_point to a non-negative value in order to avoid chunking
> >> endlessly (which is the reason for the eventual panic).
> >>
> >> Fixes: 2f5e3c9df693 ("sctp: introduce sctp_assoc_update_frag_point")
> >> Signed-off-by: Jakub Audykowicz <jakub.audykowicz@gmail.com>
> >> ---
> >>  include/net/sctp/constants.h |  3 +++
> >>  net/sctp/associola.c         | 13 +++++++------
> >>  net/sctp/chunk.c             |  6 ++++++
> >>  3 files changed, 16 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
> >> index 8dadc74c22e7..90316fab6f04 100644
> >> --- a/include/net/sctp/constants.h
> >> +++ b/include/net/sctp/constants.h
> >> @@ -293,6 +293,9 @@ enum { SCTP_MAX_GABS = 16 };
> >>                                          */
> >>  #define SCTP_DEFAULT_MINSEGMENT 512    /* MTU size ... if no mtu disc */
> >>
> >> +/* An association's fragmentation point should never be non-positive */
> >> +#define SCTP_FRAG_POINT_MIN 1
> >> +
> >>  #define SCTP_SECRET_SIZE 32            /* Number of octets in a 256 bits. */
> >>
> >>  #define SCTP_SIGNATURE_SIZE 20         /* size of a SLA-1 signature */
> >> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> >> index 6a28b96e779e..44d71a1af62e 100644
> >> --- a/net/sctp/associola.c
> >> +++ b/net/sctp/associola.c
> >> @@ -1431,13 +1431,14 @@ void sctp_assoc_update_frag_point(struct sctp_association *asoc)
> >>
> >>  void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)
> >>  {
> >> -       if (asoc->pathmtu != pmtu) {
> >> -               asoc->pathmtu = pmtu;
> >> -               sctp_assoc_update_frag_point(asoc);
> >> -       }
> >> +       pr_debug("%s: before asoc:%p, pmtu:%d, frag_point:%d\n",
> >> +               __func__, asoc, asoc->pathmtu, asoc->frag_point);
> >> +
> >> +       asoc->pathmtu = pmtu;
> >> +       sctp_assoc_update_frag_point(asoc);
> >>
> >> -       pr_debug("%s: asoc:%p, pmtu:%d, frag_point:%d\n", __func__, asoc,
> >> -                asoc->pathmtu, asoc->frag_point);
> >> +       pr_debug("%s: after asoc:%p, pmtu:%d, frag_point:%d\n",
> >> +               __func__, asoc, asoc->pathmtu, asoc->frag_point);
> >>  }
> > The idea was whenever asoc->pathmtu changes,  frag_point should
> > be updated, but we missed one place in sctp_association_init().
> >
> > Another issue is after 4-shakehand, the client's asoc->intl_enable
> > may be changed from 0 to 1, which means the frag_point should
> > also be updated, since [1]:
> >
> > void sctp_assoc_update_frag_point(struct sctp_association *asoc)
> > {
> >         int frag = sctp_mtu_payload(sctp_sk(asoc->base.sk), asoc->pathmtu,
> >                                     sctp_datachk_len(&asoc->stream)); <--- [1]
> >
> > So one fix for both issues is:
> >
> > diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
> > index 0a78cdf..19d596d 100644
> > --- a/net/sctp/stream_interleave.c
> > +++ b/net/sctp/stream_interleave.c
> > @@ -1327,4 +1327,5 @@ void sctp_stream_interleave_init(struct
> > sctp_stream *stream)
> >         asoc = container_of(stream, struct sctp_association, stream);
> >         stream->si = asoc->intl_enable ? &sctp_stream_interleave_1
> >                                        : &sctp_stream_interleave_0;
> > +       sctp_assoc_update_frag_point(asoc);
> >  }
> >
> >
> >>  /* Update the association's pmtu and frag_point by going through all the
> >> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
> >> index ce8087846f05..9f0e64ddbd9c 100644
> >> --- a/net/sctp/chunk.c
> >> +++ b/net/sctp/chunk.c
> >> @@ -190,6 +190,12 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
> >>         /* This is the biggest possible DATA chunk that can fit into
> >>          * the packet
> >>          */
> >> +       if (asoc->frag_point < SCTP_FRAG_POINT_MIN) {
> >> +               pr_warn("%s: asoc:%p->frag_point is less than allowed (%d<%d)",
> >> +                       __func__, asoc, asoc->frag_point, SCTP_FRAG_POINT_MIN);
> >> +               pr_warn("forcing minimum value to avoid chunking endlessly");
> >> +               asoc->frag_point = SCTP_FRAG_POINT_MIN;
> >> +       }
> >>         max_data = asoc->frag_point;
> > This won't happen if we sync frag_point on time like the above.
> 
> I know a better fix has been proposed and acked already but I would like to 
> follow this up a bit. I still think there is some value to the changes in this 

Any time. :-)

> patch. First of all, I am not sure the if statement in sctp_assoc_set_pmtu is 
> of any practical benefit. I assume its intention is optimization, but I'm 
> skeptical. Originally it made little sense, since it's not like assigning the 
> same value would be incorrect or costly. Upon introducing 

Agree. In SCTP stack usually we are not worried by the effects of a
single assignment, especially in control path.

> sctp_assoc_update_frag_point I can see the if being of more use since it is 
> theoretically useless to call this function if MTU is the same, but as it 
> turns out it might still be useful at what I would estimate to be a negligible 
> cost. I propose to do away with this if block.

Now it is more about consistency. If it is already set to a
value and the user sets it again, there should be no behavioral
difference.  We don't want users calling the same function multiple
times for it to "really stick" or so, for example.

The if in there, then, now serves to protect it from this.

> 
> As for the code in chunk, I'm not too proud of this hacky workaround, but 
> I still think there should be some way to avoid chunking endlessly and running 
> OOM if the configuration is wrong (here due to an implementation oversight), 
> a last-ditch fail-safe of sorts. How do you guys think this could be 

Good point.

> accomplished?

Considering that the idea is for sctp_datamsg_from_user to just
consume asoc->frag_point, we can add this check right at the end of
sctp_assoc_update_frag_point. So that we will always set it to a sane
value, no matter what, and warn if it is bogus, for whatever reason.
This way it will also warn right when it got set to a bad value.

Something like (just a draft):

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 685c7ef11eb4..128a4dd609f3 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1427,8 +1427,15 @@ void sctp_assoc_update_frag_point(struct sctp_association *asoc)
 
 	frag = min_t(int, frag, SCTP_MAX_CHUNK_LEN -
 				sctp_datachk_len(&asoc->stream));
+	frag = SCTP_TRUNC4(frag);
 
-	asoc->frag_point = SCTP_TRUNC4(frag);
+	if (frag < SCTP_FRAG_POINT_MIN) {
+		pr_warn_ratelimited("%s: asoc:%p frag_point is less than allowed (%d < %d). Forcing to the minimum value.",
+				    __func__, asoc, asoc->frag_point, SCTP_FRAG_POINT_MIN);
+		frag = SCTP_FRAG_POINT_MIN;
+	}
+
+	asoc->frag_point = frag;
 }
 
 void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)

Note that it should have only 1 pr_warn_ratelimited (no line breaks)
and be rate limited because the application could exploit this to
trigger endless warnings and having it logged multiple times won't
help.

Thanks,
  Marcelo

Re: [PATCH net] sctp: always set frag_point on pmtu change

[Marcelo Ricardo Leitner]

On Wed, Nov 28, 2018 at 12:08:38AM -0200, Marcelo Ricardo Leitner wrote:
> On Tue, Nov 27, 2018 at 11:18:02PM +0100, Jakub Audykowicz wrote:
> > On 2018-11-19 08:20, Xin Long wrote:
> > 
> > > On Mon, Nov 19, 2018 at 5:49 AM Jakub Audykowicz
> > > <jakub.audykowicz@gmail.com> wrote:
> > >> Calling send on a connected SCTP socket results in kernel panic if
> > >> spp_pathmtu was configured manually before an association is established
> > >> and it was not reconfigured to another value once the association is
> > >> established.
> > >>
> > >> Steps to reproduce:
> > >> 1. Set up a listening SCTP server socket.
> > >> 2. Set up an SCTP client socket.
> > >> 3. Configure client socket using setsockopt SCTP_PEER_ADDR_PARAMS with
> > >>     spp_pathmtu set to a legal value (e.g. 1000) and
> > >>     SPP_PMTUD_DISABLE set in spp_flags.
> > >> 4. Connect client to server.
> > >> 5. Send message from client to server.
> > >>
> > >> At this point oom-killer is invoked, which will eventually lead to:
> > >> [    5.197262] Out of memory and no killable processes...
> > >> [    5.198107] Kernel panic - not syncing: System is deadlocked on memory
> > >>
> > >> Commit 2f5e3c9df693 ("sctp: introduce sctp_assoc_update_frag_point")
> > >> introduces sctp_assoc_update_frag_point, but this function is not called
> > >> in this case, causing frag_point to be zero:
> > >>  void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)
> > >>  {
> > >> -       if (asoc->pathmtu != pmtu)
> > >> +       if (asoc->pathmtu != pmtu) {
> > >>                 asoc->pathmtu = pmtu;
> > >> +               sctp_assoc_update_frag_point(asoc);
> > >> +       }
> > >>
> > >> In this scenario, on association establishment, asoc->pathmtu is already
> > >> 1000 and pmtu will be as well. Before this commit the frag_point was being
> > >> correctly set in the scenario described. Moving the call outside the if
> > >> block fixes the issue.
> > >>
> > >> I will be providing a separate patch to lksctp-tools with a simple test
> > >> reproducing this problem ("func_tests: frag_point should never be zero").
> > >>
> > >> I have also taken the liberty to introduce a sanity check in chunk.c to
> > >> set the frag_point to a non-negative value in order to avoid chunking
> > >> endlessly (which is the reason for the eventual panic).
> > >>
> > >> Fixes: 2f5e3c9df693 ("sctp: introduce sctp_assoc_update_frag_point")
> > >> Signed-off-by: Jakub Audykowicz <jakub.audykowicz@gmail.com>
> > >> ---
> > >>  include/net/sctp/constants.h |  3 +++
> > >>  net/sctp/associola.c         | 13 +++++++------
> > >>  net/sctp/chunk.c             |  6 ++++++
> > >>  3 files changed, 16 insertions(+), 6 deletions(-)
> > >>
> > >> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
> > >> index 8dadc74c22e7..90316fab6f04 100644
> > >> --- a/include/net/sctp/constants.h
> > >> +++ b/include/net/sctp/constants.h
> > >> @@ -293,6 +293,9 @@ enum { SCTP_MAX_GABS = 16 };
> > >>                                          */
> > >>  #define SCTP_DEFAULT_MINSEGMENT 512    /* MTU size ... if no mtu disc */
> > >>
> > >> +/* An association's fragmentation point should never be non-positive */
> > >> +#define SCTP_FRAG_POINT_MIN 1
> > >> +
> > >>  #define SCTP_SECRET_SIZE 32            /* Number of octets in a 256 bits. */
> > >>
> > >>  #define SCTP_SIGNATURE_SIZE 20         /* size of a SLA-1 signature */
> > >> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> > >> index 6a28b96e779e..44d71a1af62e 100644
> > >> --- a/net/sctp/associola.c
> > >> +++ b/net/sctp/associola.c
> > >> @@ -1431,13 +1431,14 @@ void sctp_assoc_update_frag_point(struct sctp_association *asoc)
> > >>
> > >>  void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)
> > >>  {
> > >> -       if (asoc->pathmtu != pmtu) {
> > >> -               asoc->pathmtu = pmtu;
> > >> -               sctp_assoc_update_frag_point(asoc);
> > >> -       }
> > >> +       pr_debug("%s: before asoc:%p, pmtu:%d, frag_point:%d\n",
> > >> +               __func__, asoc, asoc->pathmtu, asoc->frag_point);
> > >> +
> > >> +       asoc->pathmtu = pmtu;
> > >> +       sctp_assoc_update_frag_point(asoc);
> > >>
> > >> -       pr_debug("%s: asoc:%p, pmtu:%d, frag_point:%d\n", __func__, asoc,
> > >> -                asoc->pathmtu, asoc->frag_point);
> > >> +       pr_debug("%s: after asoc:%p, pmtu:%d, frag_point:%d\n",
> > >> +               __func__, asoc, asoc->pathmtu, asoc->frag_point);
> > >>  }
> > > The idea was whenever asoc->pathmtu changes,  frag_point should
> > > be updated, but we missed one place in sctp_association_init().
> > >
> > > Another issue is after 4-shakehand, the client's asoc->intl_enable
> > > may be changed from 0 to 1, which means the frag_point should
> > > also be updated, since [1]:
> > >
> > > void sctp_assoc_update_frag_point(struct sctp_association *asoc)
> > > {
> > >         int frag = sctp_mtu_payload(sctp_sk(asoc->base.sk), asoc->pathmtu,
> > >                                     sctp_datachk_len(&asoc->stream)); <--- [1]
> > >
> > > So one fix for both issues is:
> > >
> > > diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
> > > index 0a78cdf..19d596d 100644
> > > --- a/net/sctp/stream_interleave.c
> > > +++ b/net/sctp/stream_interleave.c
> > > @@ -1327,4 +1327,5 @@ void sctp_stream_interleave_init(struct
> > > sctp_stream *stream)
> > >         asoc = container_of(stream, struct sctp_association, stream);
> > >         stream->si = asoc->intl_enable ? &sctp_stream_interleave_1
> > >                                        : &sctp_stream_interleave_0;
> > > +       sctp_assoc_update_frag_point(asoc);
> > >  }
> > >
> > >
> > >>  /* Update the association's pmtu and frag_point by going through all the
> > >> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
> > >> index ce8087846f05..9f0e64ddbd9c 100644
> > >> --- a/net/sctp/chunk.c
> > >> +++ b/net/sctp/chunk.c
> > >> @@ -190,6 +190,12 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
> > >>         /* This is the biggest possible DATA chunk that can fit into
> > >>          * the packet
> > >>          */
> > >> +       if (asoc->frag_point < SCTP_FRAG_POINT_MIN) {
> > >> +               pr_warn("%s: asoc:%p->frag_point is less than allowed (%d<%d)",
> > >> +                       __func__, asoc, asoc->frag_point, SCTP_FRAG_POINT_MIN);
> > >> +               pr_warn("forcing minimum value to avoid chunking endlessly");
> > >> +               asoc->frag_point = SCTP_FRAG_POINT_MIN;
> > >> +       }
> > >>         max_data = asoc->frag_point;
> > > This won't happen if we sync frag_point on time like the above.
> > 
> > I know a better fix has been proposed and acked already but I would like to 
> > follow this up a bit. I still think there is some value to the changes in this 
> 
> Any time. :-)
> 
> > patch. First of all, I am not sure the if statement in sctp_assoc_set_pmtu is 
> > of any practical benefit. I assume its intention is optimization, but I'm 
> > skeptical. Originally it made little sense, since it's not like assigning the 
> > same value would be incorrect or costly. Upon introducing 
> 
> Agree. In SCTP stack usually we are not worried by the effects of a
> single assignment, especially in control path.
> 
> > sctp_assoc_update_frag_point I can see the if being of more use since it is 
> > theoretically useless to call this function if MTU is the same, but as it 
> > turns out it might still be useful at what I would estimate to be a negligible 
> > cost. I propose to do away with this if block.
> 
> Now it is more about consistency. If it is already set to a
> value and the user sets it again, there should be no behavioral
> difference.  We don't want users calling the same function multiple
> times for it to "really stick" or so, for example.
> 
> The if in there, then, now serves to protect it from this.
> 
> > 
> > As for the code in chunk, I'm not too proud of this hacky workaround, but 
> > I still think there should be some way to avoid chunking endlessly and running 
> > OOM if the configuration is wrong (here due to an implementation oversight), 
> > a last-ditch fail-safe of sorts. How do you guys think this could be 
> 
> Good point.
> 
> > accomplished?
> 
> Considering that the idea is for sctp_datamsg_from_user to just
> consume asoc->frag_point, we can add this check right at the end of
> sctp_assoc_update_frag_point. So that we will always set it to a sane
> value, no matter what, and warn if it is bogus, for whatever reason.
> This way it will also warn right when it got set to a bad value.
> 
> Something like (just a draft):
> 
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 685c7ef11eb4..128a4dd609f3 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -1427,8 +1427,15 @@ void sctp_assoc_update_frag_point(struct sctp_association *asoc)
>  
>  	frag = min_t(int, frag, SCTP_MAX_CHUNK_LEN -
>  				sctp_datachk_len(&asoc->stream));
> +	frag = SCTP_TRUNC4(frag);
>  
> -	asoc->frag_point = SCTP_TRUNC4(frag);
> +	if (frag < SCTP_FRAG_POINT_MIN) {
> +		pr_warn_ratelimited("%s: asoc:%p frag_point is less than allowed (%d < %d). Forcing to the minimum value.",
> +				    __func__, asoc, asoc->frag_point, SCTP_FRAG_POINT_MIN);
> +		frag = SCTP_FRAG_POINT_MIN;

We don't need a new define here because it should be bounded to the
same values as user_frag is, like in sctp_setsockopt_maxseg.

> +	}
> +
> +	asoc->frag_point = frag;
>  }
>  
>  void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)

But please scratch this patch. It wouldn't have helped with the
situation because the lack of calling it is what caused the issue in
the first place. That said, original patch is the best way I think.
Notes below still apply, though.

It is tough to find that silver line on how much of sanity checks the
code should or should not do. My reasoning here is based on the impact
this issue has. Maybe others have different opinions?

> 
> Note that it should have only 1 pr_warn_ratelimited (no line breaks)
> and be rate limited because the application could exploit this to
> trigger endless warnings and having it logged multiple times won't
> help.
> 
> Thanks,
>   Marcelo
> 

Re: [PATCH net] sctp: always set frag_point on pmtu change

[Jakub Audykowicz]

On 2018-11-28 12:26, Marcelo Ricardo Leitner wrote:

> On Wed, Nov 28, 2018 at 12:08:38AM -0200, Marcelo Ricardo Leitner wrote:
>> On Tue, Nov 27, 2018 at 11:18:02PM +0100, Jakub Audykowicz wrote:
>>> On 2018-11-19 08:20, Xin Long wrote:
>>>
>>>> On Mon, Nov 19, 2018 at 5:49 AM Jakub Audykowicz
>>>> <jakub.audykowicz@gmail.com> wrote:
>>>>> Calling send on a connected SCTP socket results in kernel panic if
>>>>> spp_pathmtu was configured manually before an association is established
>>>>> and it was not reconfigured to another value once the association is
>>>>> established.
>>>>>
>>>>> Steps to reproduce:
>>>>> 1. Set up a listening SCTP server socket.
>>>>> 2. Set up an SCTP client socket.
>>>>> 3. Configure client socket using setsockopt SCTP_PEER_ADDR_PARAMS with
>>>>>     spp_pathmtu set to a legal value (e.g. 1000) and
>>>>>     SPP_PMTUD_DISABLE set in spp_flags.
>>>>> 4. Connect client to server.
>>>>> 5. Send message from client to server.
>>>>>
>>>>> At this point oom-killer is invoked, which will eventually lead to:
>>>>> [    5.197262] Out of memory and no killable processes...
>>>>> [    5.198107] Kernel panic - not syncing: System is deadlocked on memory
>>>>>
>>>>> Commit 2f5e3c9df693 ("sctp: introduce sctp_assoc_update_frag_point")
>>>>> introduces sctp_assoc_update_frag_point, but this function is not called
>>>>> in this case, causing frag_point to be zero:
>>>>>  void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)
>>>>>  {
>>>>> -       if (asoc->pathmtu != pmtu)
>>>>> +       if (asoc->pathmtu != pmtu) {
>>>>>                 asoc->pathmtu = pmtu;
>>>>> +               sctp_assoc_update_frag_point(asoc);
>>>>> +       }
>>>>>
>>>>> In this scenario, on association establishment, asoc->pathmtu is already
>>>>> 1000 and pmtu will be as well. Before this commit the frag_point was being
>>>>> correctly set in the scenario described. Moving the call outside the if
>>>>> block fixes the issue.
>>>>>
>>>>> I will be providing a separate patch to lksctp-tools with a simple test
>>>>> reproducing this problem ("func_tests: frag_point should never be zero").
>>>>>
>>>>> I have also taken the liberty to introduce a sanity check in chunk.c to
>>>>> set the frag_point to a non-negative value in order to avoid chunking
>>>>> endlessly (which is the reason for the eventual panic).
>>>>>
>>>>> Fixes: 2f5e3c9df693 ("sctp: introduce sctp_assoc_update_frag_point")
>>>>> Signed-off-by: Jakub Audykowicz <jakub.audykowicz@gmail.com>
>>>>> ---
>>>>>  include/net/sctp/constants.h |  3 +++
>>>>>  net/sctp/associola.c         | 13 +++++++------
>>>>>  net/sctp/chunk.c             |  6 ++++++
>>>>>  3 files changed, 16 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>>>>> index 8dadc74c22e7..90316fab6f04 100644
>>>>> --- a/include/net/sctp/constants.h
>>>>> +++ b/include/net/sctp/constants.h
>>>>> @@ -293,6 +293,9 @@ enum { SCTP_MAX_GABS = 16 };
>>>>>                                          */
>>>>>  #define SCTP_DEFAULT_MINSEGMENT 512    /* MTU size ... if no mtu disc */
>>>>>
>>>>> +/* An association's fragmentation point should never be non-positive */
>>>>> +#define SCTP_FRAG_POINT_MIN 1
>>>>> +
>>>>>  #define SCTP_SECRET_SIZE 32            /* Number of octets in a 256 bits. */
>>>>>
>>>>>  #define SCTP_SIGNATURE_SIZE 20         /* size of a SLA-1 signature */
>>>>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>>>>> index 6a28b96e779e..44d71a1af62e 100644
>>>>> --- a/net/sctp/associola.c
>>>>> +++ b/net/sctp/associola.c
>>>>> @@ -1431,13 +1431,14 @@ void sctp_assoc_update_frag_point(struct sctp_association *asoc)
>>>>>
>>>>>  void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)
>>>>>  {
>>>>> -       if (asoc->pathmtu != pmtu) {
>>>>> -               asoc->pathmtu = pmtu;
>>>>> -               sctp_assoc_update_frag_point(asoc);
>>>>> -       }
>>>>> +       pr_debug("%s: before asoc:%p, pmtu:%d, frag_point:%d\n",
>>>>> +               __func__, asoc, asoc->pathmtu, asoc->frag_point);
>>>>> +
>>>>> +       asoc->pathmtu = pmtu;
>>>>> +       sctp_assoc_update_frag_point(asoc);
>>>>>
>>>>> -       pr_debug("%s: asoc:%p, pmtu:%d, frag_point:%d\n", __func__, asoc,
>>>>> -                asoc->pathmtu, asoc->frag_point);
>>>>> +       pr_debug("%s: after asoc:%p, pmtu:%d, frag_point:%d\n",
>>>>> +               __func__, asoc, asoc->pathmtu, asoc->frag_point);
>>>>>  }
>>>> The idea was whenever asoc->pathmtu changes,  frag_point should
>>>> be updated, but we missed one place in sctp_association_init().
>>>>
>>>> Another issue is after 4-shakehand, the client's asoc->intl_enable
>>>> may be changed from 0 to 1, which means the frag_point should
>>>> also be updated, since [1]:
>>>>
>>>> void sctp_assoc_update_frag_point(struct sctp_association *asoc)
>>>> {
>>>>         int frag = sctp_mtu_payload(sctp_sk(asoc->base.sk), asoc->pathmtu,
>>>>                                     sctp_datachk_len(&asoc->stream)); <--- [1]
>>>>
>>>> So one fix for both issues is:
>>>>
>>>> diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
>>>> index 0a78cdf..19d596d 100644
>>>> --- a/net/sctp/stream_interleave.c
>>>> +++ b/net/sctp/stream_interleave.c
>>>> @@ -1327,4 +1327,5 @@ void sctp_stream_interleave_init(struct
>>>> sctp_stream *stream)
>>>>         asoc = container_of(stream, struct sctp_association, stream);
>>>>         stream->si = asoc->intl_enable ? &sctp_stream_interleave_1
>>>>                                        : &sctp_stream_interleave_0;
>>>> +       sctp_assoc_update_frag_point(asoc);
>>>>  }
>>>>
>>>>
>>>>>  /* Update the association's pmtu and frag_point by going through all the
>>>>> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
>>>>> index ce8087846f05..9f0e64ddbd9c 100644
>>>>> --- a/net/sctp/chunk.c
>>>>> +++ b/net/sctp/chunk.c
>>>>> @@ -190,6 +190,12 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>>>>>         /* This is the biggest possible DATA chunk that can fit into
>>>>>          * the packet
>>>>>          */
>>>>> +       if (asoc->frag_point < SCTP_FRAG_POINT_MIN) {
>>>>> +               pr_warn("%s: asoc:%p->frag_point is less than allowed (%d<%d)",
>>>>> +                       __func__, asoc, asoc->frag_point, SCTP_FRAG_POINT_MIN);
>>>>> +               pr_warn("forcing minimum value to avoid chunking endlessly");
>>>>> +               asoc->frag_point = SCTP_FRAG_POINT_MIN;
>>>>> +       }
>>>>>         max_data = asoc->frag_point;
>>>> This won't happen if we sync frag_point on time like the above.
>>> I know a better fix has been proposed and acked already but I would like to 
>>> follow this up a bit. I still think there is some value to the changes in this 
>> Any time. :-)
>>
>>> patch. First of all, I am not sure the if statement in sctp_assoc_set_pmtu is 
>>> of any practical benefit. I assume its intention is optimization, but I'm 
>>> skeptical. Originally it made little sense, since it's not like assigning the 
>>> same value would be incorrect or costly. Upon introducing 
>> Agree. In SCTP stack usually we are not worried by the effects of a
>> single assignment, especially in control path.
>>
>>> sctp_assoc_update_frag_point I can see the if being of more use since it is 
>>> theoretically useless to call this function if MTU is the same, but as it 
>>> turns out it might still be useful at what I would estimate to be a negligible 
>>> cost. I propose to do away with this if block.
>> Now it is more about consistency. If it is already set to a
>> value and the user sets it again, there should be no behavioral
>> difference.  We don't want users calling the same function multiple
>> times for it to "really stick" or so, for example.
>>
>> The if in there, then, now serves to protect it from this.
>>
>>> As for the code in chunk, I'm not too proud of this hacky workaround, but 
>>> I still think there should be some way to avoid chunking endlessly and running 
>>> OOM if the configuration is wrong (here due to an implementation oversight), 
>>> a last-ditch fail-safe of sorts. How do you guys think this could be 
>> Good point.
>>
>>> accomplished?
>> Considering that the idea is for sctp_datamsg_from_user to just
>> consume asoc->frag_point, we can add this check right at the end of
>> sctp_assoc_update_frag_point. So that we will always set it to a sane
>> value, no matter what, and warn if it is bogus, for whatever reason.
>> This way it will also warn right when it got set to a bad value.
>>
>> Something like (just a draft):
>>
>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>> index 685c7ef11eb4..128a4dd609f3 100644
>> --- a/net/sctp/associola.c
>> +++ b/net/sctp/associola.c
>> @@ -1427,8 +1427,15 @@ void sctp_assoc_update_frag_point(struct sctp_association *asoc)
>>  
>>  	frag = min_t(int, frag, SCTP_MAX_CHUNK_LEN -
>>  				sctp_datachk_len(&asoc->stream));
>> +	frag = SCTP_TRUNC4(frag);
>>  
>> -	asoc->frag_point = SCTP_TRUNC4(frag);
>> +	if (frag < SCTP_FRAG_POINT_MIN) {
>> +		pr_warn_ratelimited("%s: asoc:%p frag_point is less than allowed (%d < %d). Forcing to the minimum value.",
>> +				    __func__, asoc, asoc->frag_point, SCTP_FRAG_POINT_MIN);
>> +		frag = SCTP_FRAG_POINT_MIN;
> We don't need a new define here because it should be bounded to the
> same values as user_frag is, like in sctp_setsockopt_maxseg.
>
>> +	}
>> +
>> +	asoc->frag_point = frag;
>>  }
>>  
>>  void sctp_assoc_set_pmtu(struct sctp_association *asoc, __u32 pmtu)
> But please scratch this patch. It wouldn't have helped with the
> situation because the lack of calling it is what caused the issue in
> the first place. That said, original patch is the best way I think.
> Notes below still apply, though.
>
> It is tough to find that silver line on how much of sanity checks the
> code should or should not do. My reasoning here is based on the impact
> this issue has. Maybe others have different opinions?
>
>> Note that it should have only 1 pr_warn_ratelimited (no line breaks)
>> and be rate limited because the application could exploit this to
>> trigger endless warnings and having it logged multiple times won't
>> help.
>>
>> Thanks,
>>   Marcelo
>>
OK, let's forget about that "if" :)
Coming back to the sanity check, I came up with something like below,
based on the code from sctp_setsockopt_maxseg, like you mentioned.
I may have overcomplicated things since I didn't know how to accomplish
the same thing without passing sctp_sock* to sctp_datamsg_from_user.
I wanted to avoid calling sctp_min_frag_point unless absolutely
necessary, so I just check the frag_point against the zero that is
causing the eventual kernel panic.
 
diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index ab9242e51d9e..7e67c0257b3f 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -620,4 +620,15 @@ static inline bool sctp_transport_pmtu_check(struct sctp_transport *t)
 	return false;
 }
 
+static inline __u16 sctp_data_chunk_size(struct sctp_association *asoc)
+{
+    return asoc ? sctp_datachk_len(&asoc->stream) :
+                  sizeof(struct sctp_data_chunk);
+}
+
+static inline __u32 sctp_min_frag_point(struct sctp_sock *sp, __u16 datasize)
+{
+    return sctp_mtu_payload(sp, SCTP_DEFAULT_MINSEGMENT, datasize);
+}
+
 #endif /* __net_sctp_h__ */
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index a11f93790476..d09b5de73c92 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -543,7 +543,8 @@ struct sctp_datamsg {
 
 struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *,
 					    struct sctp_sndrcvinfo *,
-					    struct iov_iter *);
+					    struct iov_iter *,
+					    struct sctp_sock *);
 void sctp_datamsg_free(struct sctp_datamsg *);
 void sctp_datamsg_put(struct sctp_datamsg *);
 void sctp_chunk_fail(struct sctp_chunk *, int error);
diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
index ce8087846f05..753c2c123767 100644
--- a/net/sctp/chunk.c
+++ b/net/sctp/chunk.c
@@ -164,7 +164,8 @@ static void sctp_datamsg_assign(struct sctp_datamsg *msg, struct sctp_chunk *chu
  */
 struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
 					    struct sctp_sndrcvinfo *sinfo,
-					    struct iov_iter *from)
+					    struct iov_iter *from,
+					    struct sctp_sock *sp)
 {
 	size_t len, first_len, max_data, remaining;
 	size_t msg_len = iov_iter_count(from);
@@ -173,6 +174,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
 	struct sctp_chunk *chunk;
 	struct sctp_datamsg *msg;
 	int err;
+	__u32 min_frag_point;
 
 	msg = sctp_datamsg_new(GFP_KERNEL);
 	if (!msg)
@@ -190,6 +192,12 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
 	/* This is the biggest possible DATA chunk that can fit into
 	 * the packet
 	 */
+	if (unlikely(asoc->frag_point == 0)) {
+		min_frag_point = sctp_min_frag_point(sp, sctp_data_chunk_size(asoc));
+		pr_warn_ratelimited("%s: asoc:%p frag_point is too low (%d < %d), using default minimum",
+			__func__, asoc, asoc->frag_point, min_frag_point);
+		asoc->frag_point = min_frag_point;
+	}
 	max_data = asoc->frag_point;
 
 	/* If the the peer requested that we authenticate DATA chunks
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index bf618d1b41fd..28d711609ef1 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1938,7 +1938,7 @@ static int sctp_sendmsg_to_asoc(struct sctp_association *asoc,
 		pr_debug("%s: we associated primitively\n", __func__);
 	}
 
-	datamsg = sctp_datamsg_from_user(asoc, sinfo, &msg->msg_iter);
+	datamsg = sctp_datamsg_from_user(asoc, sinfo, &msg->msg_iter, sp);
 	if (IS_ERR(datamsg)) {
 		err = PTR_ERR(datamsg);
 		goto err;
@@ -3321,11 +3321,9 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval, unsigned
 
 	if (val) {
 		int min_len, max_len;
-		__u16 datasize = asoc ? sctp_datachk_len(&asoc->stream) :
-				 sizeof(struct sctp_data_chunk);
+		__u16 datasize = sctp_data_chunk_size(asoc);
 
-		min_len = sctp_mtu_payload(sp, SCTP_DEFAULT_MINSEGMENT,
-					   datasize);
+		min_len = sctp_min_frag_point(sp, datasize);
 		max_len = SCTP_MAX_CHUNK_LEN - datasize;
 
 		if (val < min_len || val > max_len)

Re: [PATCH net] sctp: always set frag_point on pmtu change

[Marcelo Ricardo Leitner]

On Tue, Dec 04, 2018 at 06:00:51PM +0100, Jakub Audykowicz wrote:
...
> OK, let's forget about that "if" :)
> Coming back to the sanity check, I came up with something like below,
> based on the code from sctp_setsockopt_maxseg, like you mentioned.
> I may have overcomplicated things since I didn't know how to accomplish
> the same thing without passing sctp_sock* to sctp_datamsg_from_user.

Yep. More below.

> I wanted to avoid calling sctp_min_frag_point unless absolutely
> necessary, so I just check the frag_point against the zero that is
> causing the eventual kernel panic.

Makes sense.

>  
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index ab9242e51d9e..7e67c0257b3f 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -620,4 +620,15 @@ static inline bool sctp_transport_pmtu_check(struct sctp_transport *t)
>  	return false;
>  }
>  
> +static inline __u16 sctp_data_chunk_size(struct sctp_association *asoc)
> +{
> +    return asoc ? sctp_datachk_len(&asoc->stream) :
> +                  sizeof(struct sctp_data_chunk);

I don't think we need another layer on top of data chunk sizes here.
We don't need this asoc check by the sendmsg callpath because it
cannot be NULL by then. That said, I think we have avoid this helper,
for now at least.

One other way would be to include the check into sctp_datachk_len(),
but we currently have 9 calls to that but only 1 requires the check.

As asoc is initialized and considering the fix we just had in this
area, stream->si will also be initialized so you should be good to
just call sctp_datachk_len() directly.

> +}
> +
> +static inline __u32 sctp_min_frag_point(struct sctp_sock *sp, __u16 datasize)
> +{
> +    return sctp_mtu_payload(sp, SCTP_DEFAULT_MINSEGMENT, datasize);
> +}

This is a good helper to have. Makes it clearer.

> +
>  #endif /* __net_sctp_h__ */
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index a11f93790476..d09b5de73c92 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -543,7 +543,8 @@ struct sctp_datamsg {
>  
>  struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *,
>  					    struct sctp_sndrcvinfo *,
> -					    struct iov_iter *);
> +					    struct iov_iter *,
> +					    struct sctp_sock *);
>  void sctp_datamsg_free(struct sctp_datamsg *);
>  void sctp_datamsg_put(struct sctp_datamsg *);
>  void sctp_chunk_fail(struct sctp_chunk *, int error);
> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
> index ce8087846f05..753c2c123767 100644
> --- a/net/sctp/chunk.c
> +++ b/net/sctp/chunk.c
> @@ -164,7 +164,8 @@ static void sctp_datamsg_assign(struct sctp_datamsg *msg, struct sctp_chunk *chu
>   */
>  struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>  					    struct sctp_sndrcvinfo *sinfo,
> -					    struct iov_iter *from)
> +					    struct iov_iter *from,
> +					    struct sctp_sock *sp)
>  {
>  	size_t len, first_len, max_data, remaining;
>  	size_t msg_len = iov_iter_count(from);
> @@ -173,6 +174,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>  	struct sctp_chunk *chunk;
>  	struct sctp_datamsg *msg;
>  	int err;
> +	__u32 min_frag_point;

Reverse christmass tree.. swap the last two lines:
+	__u32 min_frag_point;
 	int err;
But I guess we don't need this var anymore:

>  
>  	msg = sctp_datamsg_new(GFP_KERNEL);
>  	if (!msg)
> @@ -190,6 +192,12 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>  	/* This is the biggest possible DATA chunk that can fit into
>  	 * the packet
>  	 */
> +	if (unlikely(asoc->frag_point == 0)) {
                     !asoc->frag_point   instead

You can get to sctp_sock here with: sctp_sk(asoc->base.sk)
		struct sctp_sock *sp = sctp_sk(asoc->base.sk);

> +		min_frag_point = sctp_min_frag_point(sp, sctp_data_chunk_size(asoc));
> +		pr_warn_ratelimited("%s: asoc:%p frag_point is too low (%d < %d), using default minimum",
> +			__func__, asoc, asoc->frag_point, min_frag_point);
> +		asoc->frag_point = min_frag_point;

No no. Lets not fix up things here. If we do this assignment, we may
make the disparity even worse.
With that, we can work only on max_data and avoid the need of min_frag_point.
  	max_data = asoc->frag_point;
	if (unlikely(!max_data)) {
		...
		max_data = ...
	}

> +	}
>  	max_data = asoc->frag_point;
>  
>  	/* If the the peer requested that we authenticate DATA chunks
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index bf618d1b41fd..28d711609ef1 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1938,7 +1938,7 @@ static int sctp_sendmsg_to_asoc(struct sctp_association *asoc,
>  		pr_debug("%s: we associated primitively\n", __func__);
>  	}
>  
> -	datamsg = sctp_datamsg_from_user(asoc, sinfo, &msg->msg_iter);
> +	datamsg = sctp_datamsg_from_user(asoc, sinfo, &msg->msg_iter, sp);
>  	if (IS_ERR(datamsg)) {
>  		err = PTR_ERR(datamsg);
>  		goto err;
> @@ -3321,11 +3321,9 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval, unsigned
>  
>  	if (val) {
>  		int min_len, max_len;
> -		__u16 datasize = asoc ? sctp_datachk_len(&asoc->stream) :
> -				 sizeof(struct sctp_data_chunk);
> +		__u16 datasize = sctp_data_chunk_size(asoc);
>  
> -		min_len = sctp_mtu_payload(sp, SCTP_DEFAULT_MINSEGMENT,
> -					   datasize);
> +		min_len = sctp_min_frag_point(sp, datasize);
>  		max_len = SCTP_MAX_CHUNK_LEN - datasize;
>  
>  		if (val < min_len || val > max_len)
> 

Re: [PATCH net] sctp: always set frag_point on pmtu change

[Jakub Audykowicz]

On 2018-12-04 18:45, Marcelo Ricardo Leitner wrote:

> On Tue, Dec 04, 2018 at 06:00:51PM +0100, Jakub Audykowicz wrote:
> ...
>> OK, let's forget about that "if" :)
>> Coming back to the sanity check, I came up with something like below,
>> based on the code from sctp_setsockopt_maxseg, like you mentioned.
>> I may have overcomplicated things since I didn't know how to accomplish
>> the same thing without passing sctp_sock* to sctp_datamsg_from_user.
> Yep. More below.
>
>> I wanted to avoid calling sctp_min_frag_point unless absolutely
>> necessary, so I just check the frag_point against the zero that is
>> causing the eventual kernel panic.
> Makes sense.
>
>>  
>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>> index ab9242e51d9e..7e67c0257b3f 100644
>> --- a/include/net/sctp/sctp.h
>> +++ b/include/net/sctp/sctp.h
>> @@ -620,4 +620,15 @@ static inline bool sctp_transport_pmtu_check(struct sctp_transport *t)
>>  	return false;
>>  }
>>  
>> +static inline __u16 sctp_data_chunk_size(struct sctp_association *asoc)
>> +{
>> +    return asoc ? sctp_datachk_len(&asoc->stream) :
>> +                  sizeof(struct sctp_data_chunk);
> I don't think we need another layer on top of data chunk sizes here.
> We don't need this asoc check by the sendmsg callpath because it
> cannot be NULL by then. That said, I think we have avoid this helper,
> for now at least.
>
> One other way would be to include the check into sctp_datachk_len(),
> but we currently have 9 calls to that but only 1 requires the check.
>
> As asoc is initialized and considering the fix we just had in this
> area, stream->si will also be initialized so you should be good to
> just call sctp_datachk_len() directly.
>
>> +}
>> +
>> +static inline __u32 sctp_min_frag_point(struct sctp_sock *sp, __u16 datasize)
>> +{
>> +    return sctp_mtu_payload(sp, SCTP_DEFAULT_MINSEGMENT, datasize);
>> +}
> This is a good helper to have. Makes it clearer.
>
>> +
>>  #endif /* __net_sctp_h__ */
>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>> index a11f93790476..d09b5de73c92 100644
>> --- a/include/net/sctp/structs.h
>> +++ b/include/net/sctp/structs.h
>> @@ -543,7 +543,8 @@ struct sctp_datamsg {
>>  
>>  struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *,
>>  					    struct sctp_sndrcvinfo *,
>> -					    struct iov_iter *);
>> +					    struct iov_iter *,
>> +					    struct sctp_sock *);
>>  void sctp_datamsg_free(struct sctp_datamsg *);
>>  void sctp_datamsg_put(struct sctp_datamsg *);
>>  void sctp_chunk_fail(struct sctp_chunk *, int error);
>> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
>> index ce8087846f05..753c2c123767 100644
>> --- a/net/sctp/chunk.c
>> +++ b/net/sctp/chunk.c
>> @@ -164,7 +164,8 @@ static void sctp_datamsg_assign(struct sctp_datamsg *msg, struct sctp_chunk *chu
>>   */
>>  struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>>  					    struct sctp_sndrcvinfo *sinfo,
>> -					    struct iov_iter *from)
>> +					    struct iov_iter *from,
>> +					    struct sctp_sock *sp)
>>  {
>>  	size_t len, first_len, max_data, remaining;
>>  	size_t msg_len = iov_iter_count(from);
>> @@ -173,6 +174,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>>  	struct sctp_chunk *chunk;
>>  	struct sctp_datamsg *msg;
>>  	int err;
>> +	__u32 min_frag_point;
> Reverse christmass tree.. swap the last two lines:
> +	__u32 min_frag_point;
>  	int err;
> But I guess we don't need this var anymore:
>
>>  
>>  	msg = sctp_datamsg_new(GFP_KERNEL);
>>  	if (!msg)
>> @@ -190,6 +192,12 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>>  	/* This is the biggest possible DATA chunk that can fit into
>>  	 * the packet
>>  	 */
>> +	if (unlikely(asoc->frag_point == 0)) {
>                      !asoc->frag_point   instead
>
> You can get to sctp_sock here with: sctp_sk(asoc->base.sk)
> 		struct sctp_sock *sp = sctp_sk(asoc->base.sk);
>
>> +		min_frag_point = sctp_min_frag_point(sp, sctp_data_chunk_size(asoc));
>> +		pr_warn_ratelimited("%s: asoc:%p frag_point is too low (%d < %d), using default minimum",
>> +			__func__, asoc, asoc->frag_point, min_frag_point);
>> +		asoc->frag_point = min_frag_point;
> No no. Lets not fix up things here. If we do this assignment, we may
> make the disparity even worse.
> With that, we can work only on max_data and avoid the need of min_frag_point.
>   	max_data = asoc->frag_point;
> 	if (unlikely(!max_data)) {
> 		...
> 		max_data = ...
> 	}
>
>> +	}
>>  	max_data = asoc->frag_point;
>>  
>>  	/* If the the peer requested that we authenticate DATA chunks
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index bf618d1b41fd..28d711609ef1 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -1938,7 +1938,7 @@ static int sctp_sendmsg_to_asoc(struct sctp_association *asoc,
>>  		pr_debug("%s: we associated primitively\n", __func__);
>>  	}
>>  
>> -	datamsg = sctp_datamsg_from_user(asoc, sinfo, &msg->msg_iter);
>> +	datamsg = sctp_datamsg_from_user(asoc, sinfo, &msg->msg_iter, sp);
>>  	if (IS_ERR(datamsg)) {
>>  		err = PTR_ERR(datamsg);
>>  		goto err;
>> @@ -3321,11 +3321,9 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval, unsigned
>>  
>>  	if (val) {
>>  		int min_len, max_len;
>> -		__u16 datasize = asoc ? sctp_datachk_len(&asoc->stream) :
>> -				 sizeof(struct sctp_data_chunk);
>> +		__u16 datasize = sctp_data_chunk_size(asoc);
>>  
>> -		min_len = sctp_mtu_payload(sp, SCTP_DEFAULT_MINSEGMENT,
>> -					   datasize);
>> +		min_len = sctp_min_frag_point(sp, datasize);
>>  		max_len = SCTP_MAX_CHUNK_LEN - datasize;
>>  
>>  		if (val < min_len || val > max_len)
>>
Thanks, I've taken your remarks into account and ended up with this 
simple solution:

diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index ab9242e51d9e..3487686f2cf5 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -620,4 +620,9 @@ static inline bool sctp_transport_pmtu_check(struct sctp_transport *t)
 	return false;
 }
 
+static inline __u32 sctp_min_frag_point(struct sctp_sock *sp, __u16 datasize)
+{
+    return sctp_mtu_payload(sp, SCTP_DEFAULT_MINSEGMENT, datasize);
+}
+
 #endif /* __net_sctp_h__ */
diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
index ce8087846f05..dc12c2ba487f 100644
--- a/net/sctp/chunk.c
+++ b/net/sctp/chunk.c
@@ -191,6 +191,12 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
 	 * the packet
 	 */
 	max_data = asoc->frag_point;
+	if (unlikely(!max_data)) {
+		pr_warn_ratelimited("%s: asoc:%p frag_point is zero, forcing max_data to default minimum",
+			__func__, asoc);
+		max_data = sctp_min_frag_point(
+			sctp_sk(asoc->base.sk), sctp_datachk_len(&asoc->stream));
+	}
 
 	/* If the the peer requested that we authenticate DATA chunks
 	 * we need to account for bundling of the AUTH chunks along with
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index bf618d1b41fd..b8cebd5a87e5 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3324,8 +3324,7 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval, unsigned
 		__u16 datasize = asoc ? sctp_datachk_len(&asoc->stream) :
 				 sizeof(struct sctp_data_chunk);
 
-		min_len = sctp_mtu_payload(sp, SCTP_DEFAULT_MINSEGMENT,
-					   datasize);
+		min_len = sctp_min_frag_point(sp, datasize);
 		max_len = SCTP_MAX_CHUNK_LEN - datasize;
 
 		if (val < min_len || val > max_len)

 

Re: [PATCH net] sctp: always set frag_point on pmtu change

[Marcelo Ricardo Leitner]

On Tue, Dec 04, 2018 at 07:51:54PM +0100, Jakub Audykowicz wrote:
...
> Thanks, I've taken your remarks into account and ended up with this 
> simple solution:

LGTM! Thanks

> 
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index ab9242e51d9e..3487686f2cf5 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -620,4 +620,9 @@ static inline bool sctp_transport_pmtu_check(struct sctp_transport *t)
>  	return false;
>  }
>  
> +static inline __u32 sctp_min_frag_point(struct sctp_sock *sp, __u16 datasize)
> +{
> +    return sctp_mtu_payload(sp, SCTP_DEFAULT_MINSEGMENT, datasize);

Not sure how final the patch is but be sure to run checkpatch.pl on
it before submitting it officially. It flagged some issues like the
above indentation using spaces, for example.

> +}
> +
>  #endif /* __net_sctp_h__ */
> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
> index ce8087846f05..dc12c2ba487f 100644
> --- a/net/sctp/chunk.c
> +++ b/net/sctp/chunk.c
> @@ -191,6 +191,12 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>  	 * the packet
>  	 */
>  	max_data = asoc->frag_point;
> +	if (unlikely(!max_data)) {
> +		pr_warn_ratelimited("%s: asoc:%p frag_point is zero, forcing max_data to default minimum",
> +			__func__, asoc);
> +		max_data = sctp_min_frag_point(
> +			sctp_sk(asoc->base.sk), sctp_datachk_len(&asoc->stream));
> +	}
>  
>  	/* If the the peer requested that we authenticate DATA chunks
>  	 * we need to account for bundling of the AUTH chunks along with
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index bf618d1b41fd..b8cebd5a87e5 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -3324,8 +3324,7 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval, unsigned
>  		__u16 datasize = asoc ? sctp_datachk_len(&asoc->stream) :
>  				 sizeof(struct sctp_data_chunk);
>  
> -		min_len = sctp_mtu_payload(sp, SCTP_DEFAULT_MINSEGMENT,
> -					   datasize);
> +		min_len = sctp_min_frag_point(sp, datasize);
>  		max_len = SCTP_MAX_CHUNK_LEN - datasize;
>  
>  		if (val < min_len || val > max_len)
> 
>  
> 

Re: [PATCH net] sctp: always set frag_point on pmtu change

[Jakub Audykowicz]

On 2018-12-04 19:58, Marcelo Ricardo Leitner wrote:

> On Tue, Dec 04, 2018 at 07:51:54PM +0100, Jakub Audykowicz wrote:
> ...
>> Thanks, I've taken your remarks into account and ended up with this 
>> simple solution:
> LGTM! Thanks
>
>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>> index ab9242e51d9e..3487686f2cf5 100644
>> --- a/include/net/sctp/sctp.h
>> +++ b/include/net/sctp/sctp.h
>> @@ -620,4 +620,9 @@ static inline bool sctp_transport_pmtu_check(struct sctp_transport *t)
>>  	return false;
>>  }
>>  
>> +static inline __u32 sctp_min_frag_point(struct sctp_sock *sp, __u16 datasize)
>> +{
>> +    return sctp_mtu_payload(sp, SCTP_DEFAULT_MINSEGMENT, datasize);
> Not sure how final the patch is but be sure to run checkpatch.pl on
> it before submitting it officially. It flagged some issues like the
> above indentation using spaces, for example.

Should be fine now. I have checkpatch as a post-commit hook but I was
not committing when creating these quick diffs :).

>
>> +}
>> +
>>  #endif /* __net_sctp_h__ */
>> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
>> index ce8087846f05..dc12c2ba487f 100644
>> --- a/net/sctp/chunk.c
>> +++ b/net/sctp/chunk.c
>> @@ -191,6 +191,12 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>>  	 * the packet
>>  	 */
>>  	max_data = asoc->frag_point;
>> +	if (unlikely(!max_data)) {
>> +		pr_warn_ratelimited("%s: asoc:%p frag_point is zero, forcing max_data to default minimum",
>> +			__func__, asoc);
>> +		max_data = sctp_min_frag_point(
>> +			sctp_sk(asoc->base.sk), sctp_datachk_len(&asoc->stream));
>> +	}
>>  
>>  	/* If the the peer requested that we authenticate DATA chunks
>>  	 * we need to account for bundling of the AUTH chunks along with
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index bf618d1b41fd..b8cebd5a87e5 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -3324,8 +3324,7 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval, unsigned
>>  		__u16 datasize = asoc ? sctp_datachk_len(&asoc->stream) :
>>  				 sizeof(struct sctp_data_chunk);
>>  
>> -		min_len = sctp_mtu_payload(sp, SCTP_DEFAULT_MINSEGMENT,
>> -					   datasize);
>> +		min_len = sctp_min_frag_point(sp, datasize);
>>  		max_len = SCTP_MAX_CHUNK_LEN - datasize;
>>  
>>  		if (val < min_len || val > max_len)
>>
>>  
>>
>>