* Re: [PATCHv2 net] sctp: update frag_point when stream_interleave is set
2018-11-27 11:11 [PATCHv2 net] sctp: update frag_point when stream_interleave is set Xin Long
@ 2018-11-27 21:39 ` Jakub Audykowicz
2018-11-27 21:41 ` Marcelo Ricardo Leitner
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Jakub Audykowicz @ 2018-11-27 21:39 UTC (permalink / raw)
To: Xin Long
Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner,
Neil Horman
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 <jakub.audykowicz@gmail.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> 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:)
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCHv2 net] sctp: update frag_point when stream_interleave is set
2018-11-27 11:11 [PATCHv2 net] sctp: update frag_point when stream_interleave is set Xin Long
2018-11-27 21:39 ` Jakub Audykowicz
@ 2018-11-27 21:41 ` Marcelo Ricardo Leitner
2018-11-28 12:21 ` Neil Horman
2018-11-30 21:13 ` David Miller
3 siblings, 0 replies; 5+ messages in thread
From: Marcelo Ricardo Leitner @ 2018-11-27 21:41 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman, Jakub Audykowicz
On Tue, Nov 27, 2018 at 07:11:50PM +0800, 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 <jakub.audykowicz@gmail.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> 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;
>
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCHv2 net] sctp: update frag_point when stream_interleave is set
2018-11-27 11:11 [PATCHv2 net] sctp: update frag_point when stream_interleave is set Xin Long
2018-11-27 21:39 ` Jakub Audykowicz
2018-11-27 21:41 ` Marcelo Ricardo Leitner
@ 2018-11-28 12:21 ` Neil Horman
2018-11-30 21:13 ` David Miller
3 siblings, 0 replies; 5+ messages in thread
From: Neil Horman @ 2018-11-28 12:21 UTC (permalink / raw)
To: Xin Long
Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner,
Jakub Audykowicz
On Tue, Nov 27, 2018 at 07:11:50PM +0800, 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 <jakub.audykowicz@gmail.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> 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;
>
> --
> 2.1.0
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCHv2 net] sctp: update frag_point when stream_interleave is set
2018-11-27 11:11 [PATCHv2 net] sctp: update frag_point when stream_interleave is set Xin Long
` (2 preceding siblings ...)
2018-11-28 12:21 ` Neil Horman
@ 2018-11-30 21:13 ` David Miller
3 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2018-11-30 21:13 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman, jakub.audykowicz
From: Xin Long <lucien.xin@gmail.com>
Date: Tue, 27 Nov 2018 19:11:50 +0800
> 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 <jakub.audykowicz@gmail.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Applied and queued up for -stable back to v4.18
^ permalink raw reply [flat|nested] 5+ messages in thread