Re: [PATCH net] sctp: always set frag_point on pmtu change
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
>>>> 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
>>>>> ---
>>>>> 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) {
>>>>>
Re: [PATCH net] sctp: always set frag_point on pmtu change
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_dat
[PATCH net] sctp: frag_point sanity check
If for some reason an association's fragmentation point is zero,
sctp_datamsg_from_user will try to endlessly try to divide a message
into zero-sized chunks. This eventually causes kernel panic due to
running out of memory.
Although this situation is quite unlikely, it has occurred before as
reported. I propose to add this simple last-ditch sanity check due to
the severity of the potential consequences.
Signed-off-by: Jakub Audykowicz
---
include/net/sctp/sctp.h | 5 +
net/sctp/chunk.c| 6 ++
net/sctp/socket.c | 3 +--
3 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index ab9242e51d9e..2abbc15824af 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..d5b91bc8a377 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)) {
+ max_data = sctp_min_frag_point(sctp_sk(asoc->base.sk),
+ sctp_datachk_len(&asoc->stream));
+ pr_warn_ratelimited("%s: asoc:%p frag_point is zero, forcing
max_data to default minimum (%d)",
+ __func__, asoc, max_data);
+ }
/* 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)
--
2.17.1
Re: [PATCH net] sctp: always set frag_point on pmtu change
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)
>>
>>
>>
>>
[PATCH net] sctp: fix pr_warn max_data argument type mismatch
My previous patch introduced a compilation warning regarding a type
mismatch (int vs size_t). This is a one-letter fix for good housekeeping.
Signed-off-by: Jakub Audykowicz
---
net/sctp/chunk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
index d5b91bc8a377..ee5638358ad5 100644
--- a/net/sctp/chunk.c
+++ b/net/sctp/chunk.c
@@ -194,7 +194,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct
sctp_association *asoc,
if (unlikely(!max_data)) {
max_data = sctp_min_frag_point(sctp_sk(asoc->base.sk),
sctp_datachk_len(&asoc->stream));
- pr_warn_ratelimited("%s: asoc:%p frag_point is zero, forcing
max_data to default minimum (%d)",
+ pr_warn_ratelimited("%s: asoc:%p frag_point is zero, forcing
max_data to default minimum (%ld)",
__func__, asoc, max_data);
}
--
2.17.1
Re: [PATCH net] sctp: fix pr_warn max_data argument type mismatch
On 2018-12-06 09:03, David Miller wrote: > From: Jakub Audykowicz > Date: Thu, 6 Dec 2018 08:58:37 +0100 > >> My previous patch introduced a compilation warning regarding a type >> mismatch (int vs size_t). This is a one-letter fix for good housekeeping. >> >> Signed-off-by: Jakub Audykowicz > Still wrong and I fixed it when I applied your patch. > > You need to use the 'Z' prefix for size_t, so %Zu in this case. Right, I just realized that as well, thanks!
[PATCH net] sctp: always set frag_point on pmtu change
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
---
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: [PATCHv2 net] sctp: update frag_point when stream_interleave is set
On 2018-11-27 12:11, Xin Long wrote:
> sctp_assoc_update_frag_point() should be called whenever asoc->pathmtu
> changes, but we missed one place in sctp_association_init(). It would
> cause frag_point is zero when sending data.
>
> As says in Jakub's reproducer, if sp->pathmtu is set by socketopt, the
> new asoc->pathmtu inherits it in sctp_association_init(). Later when
> transports are added and their pmtu >= asoc->pathmtu, it will never
> call sctp_assoc_update_frag_point() to set frag_point.
>
> This patch is to fix it by updating frag_point after asoc->pathmtu is
> set as sp->pathmtu in sctp_association_init(). Note that it moved them
> after sctp_stream_init(), as stream->si needs to be set first.
>
> Frag_point's calculation is also related with datachunk's type, so it
> needs to update frag_point when stream->si may be changed in
> sctp_process_init().
>
> v1->v2:
> - call sctp_assoc_update_frag_point() separately in sctp_process_init
> and sctp_association_init, per Marcelo's suggestion.
>
> Fixes: 2f5e3c9df693 ("sctp: introduce sctp_assoc_update_frag_point")
> Reported-by: Jakub Audykowicz
> Signed-off-by: Xin Long
> ---
> net/sctp/associola.c | 7 ---
> net/sctp/sm_make_chunk.c | 3 +++
> 2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 6a28b96..dd77ec3 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -118,9 +118,6 @@ static struct sctp_association *sctp_association_init(
> asoc->flowlabel = sp->flowlabel;
> asoc->dscp = sp->dscp;
>
> - /* Initialize default path MTU. */
> - asoc->pathmtu = sp->pathmtu;
> -
> /* Set association default SACK delay */
> asoc->sackdelay = msecs_to_jiffies(sp->sackdelay);
> asoc->sackfreq = sp->sackfreq;
> @@ -252,6 +249,10 @@ static struct sctp_association *sctp_association_init(
>0, gfp))
> goto fail_init;
>
> + /* Initialize default path MTU. */
> + asoc->pathmtu = sp->pathmtu;
> + sctp_assoc_update_frag_point(asoc);
> +
> /* Assume that peer would support both address types unless we are
>* told otherwise.
>*/
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 4a4fd19..f4ac6c5 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -2462,6 +2462,9 @@ int sctp_process_init(struct sctp_association *asoc,
> struct sctp_chunk *chunk,
>asoc->c.sinit_max_instreams, gfp))
> goto clean_up;
>
> + /* Update frag_point when stream_interleave may get changed. */
> + sctp_assoc_update_frag_point(asoc);
> +
> if (!asoc->temp && sctp_assoc_set_id(asoc, gfp))
> goto clean_up;
>
For what it's worth, I can confirm this works as a fix for my issue:)
Re: [PATCH net] sctp: always set frag_point on pmtu change
On 2018-11-19 08:20, Xin Long wrote:
> On Mon, Nov 19, 2018 at 5:49 AM Jakub Audykowicz
> 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
>> ---
>> 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)
> {
&g
