[PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Xin Long]

Now when using stream reconfig to add out streams, stream->out
will get re-allocated, and all old streams' information will
be copied to the new ones and the old ones will be freed.

So without stream->out_curr updated, next time when trying to
send from stream->out_curr stream, a panic would be caused.

This patch is to check and update stream->out_curr when
allocating stream_out.

v1->v2:
  - define fa_index() to get elem index from stream->out_curr.

Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
Reported-by: Ying Xu <yinxu@redhat.com>
Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/stream.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index 3892e76..30e7809 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
 	}
 }
 
+static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
+{
+	size_t index = 0;
+
+	while (count--) {
+		if (elem == flex_array_get(fa, index))
+			break;
+		index++;
+	}
+
+	return index;
+}
+
 /* Migrates chunks from stream queues to new stream queues if needed,
  * but not across associations. Also, removes those chunks to streams
  * higher than the new max.
@@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
 
 	if (stream->out) {
 		fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
+		if (stream->out_curr) {
+			size_t index = fa_index(stream->out, stream->out_curr,
+						stream->outcnt);
+
+			BUG_ON(index == stream->outcnt);
+			stream->out_curr = flex_array_get(out, index);
+		}
 		fa_free(stream->out);
 	}
 
-- 
2.1.0

[PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Xin Long]

Now when using stream reconfig to add out streams, stream->out
will get re-allocated, and all old streams' information will
be copied to the new ones and the old ones will be freed.

So without stream->out_curr updated, next time when trying to
send from stream->out_curr stream, a panic would be caused.

This patch is to check and update stream->out_curr when
allocating stream_out.

v1->v2:
  - define fa_index() to get elem index from stream->out_curr.

Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
Reported-by: Ying Xu <yinxu@redhat.com>
Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/stream.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index 3892e76..30e7809 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
 	}
 }
 
+static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
+{
+	size_t index = 0;
+
+	while (count--) {
+		if (elem = flex_array_get(fa, index))
+			break;
+		index++;
+	}
+
+	return index;
+}
+
 /* Migrates chunks from stream queues to new stream queues if needed,
  * but not across associations. Also, removes those chunks to streams
  * higher than the new max.
@@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
 
 	if (stream->out) {
 		fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
+		if (stream->out_curr) {
+			size_t index = fa_index(stream->out, stream->out_curr,
+						stream->outcnt);
+
+			BUG_ON(index = stream->outcnt);
+			stream->out_curr = flex_array_get(out, index);
+		}
 		fa_free(stream->out);
 	}
 
-- 
2.1.0

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Neil Horman]

On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> Now when using stream reconfig to add out streams, stream->out
> will get re-allocated, and all old streams' information will
> be copied to the new ones and the old ones will be freed.
> 
> So without stream->out_curr updated, next time when trying to
> send from stream->out_curr stream, a panic would be caused.
> 
> This patch is to check and update stream->out_curr when
> allocating stream_out.
> 
> v1->v2:
>   - define fa_index() to get elem index from stream->out_curr.
> 
> Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> Reported-by: Ying Xu <yinxu@redhat.com>
> Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/stream.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index 3892e76..30e7809 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
>  	}
>  }
>  
> +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> +{
> +	size_t index = 0;
> +
> +	while (count--) {
> +		if (elem == flex_array_get(fa, index))
> +			break;
> +		index++;
> +	}
> +
> +	return index;
> +}
> +
>  /* Migrates chunks from stream queues to new stream queues if needed,
>   * but not across associations. Also, removes those chunks to streams
>   * higher than the new max.
> @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
>  
>  	if (stream->out) {
>  		fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> +		if (stream->out_curr) {
> +			size_t index = fa_index(stream->out, stream->out_curr,
> +						stream->outcnt);
> +
> +			BUG_ON(index == stream->outcnt);
> +			stream->out_curr = flex_array_get(out, index);
> +		}
>  		fa_free(stream->out);
>  	}
>  
> -- 
> 2.1.0
> 
> 

I'm having a hard time understanding why, as I noted earlier, you don't just
write a function in the flex_array code that can resize the number of elements
in your array.  If you do that, you can avoid both all the copying, and the need
to lookup the in-use pointer again

Neil

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Neil Horman]

On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> Now when using stream reconfig to add out streams, stream->out
> will get re-allocated, and all old streams' information will
> be copied to the new ones and the old ones will be freed.
> 
> So without stream->out_curr updated, next time when trying to
> send from stream->out_curr stream, a panic would be caused.
> 
> This patch is to check and update stream->out_curr when
> allocating stream_out.
> 
> v1->v2:
>   - define fa_index() to get elem index from stream->out_curr.
> 
> Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> Reported-by: Ying Xu <yinxu@redhat.com>
> Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/stream.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index 3892e76..30e7809 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
>  	}
>  }
>  
> +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> +{
> +	size_t index = 0;
> +
> +	while (count--) {
> +		if (elem = flex_array_get(fa, index))
> +			break;
> +		index++;
> +	}
> +
> +	return index;
> +}
> +
>  /* Migrates chunks from stream queues to new stream queues if needed,
>   * but not across associations. Also, removes those chunks to streams
>   * higher than the new max.
> @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
>  
>  	if (stream->out) {
>  		fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> +		if (stream->out_curr) {
> +			size_t index = fa_index(stream->out, stream->out_curr,
> +						stream->outcnt);
> +
> +			BUG_ON(index = stream->outcnt);
> +			stream->out_curr = flex_array_get(out, index);
> +		}
>  		fa_free(stream->out);
>  	}
>  
> -- 
> 2.1.0
> 
> 

I'm having a hard time understanding why, as I noted earlier, you don't just
write a function in the flex_array code that can resize the number of elements
in your array.  If you do that, you can avoid both all the copying, and the need
to lookup the in-use pointer again

Neil

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Xin Long]

On Thu, Nov 29, 2018 at 9:50 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > Now when using stream reconfig to add out streams, stream->out
> > will get re-allocated, and all old streams' information will
> > be copied to the new ones and the old ones will be freed.
> >
> > So without stream->out_curr updated, next time when trying to
> > send from stream->out_curr stream, a panic would be caused.
> >
> > This patch is to check and update stream->out_curr when
> > allocating stream_out.
> >
> > v1->v2:
> >   - define fa_index() to get elem index from stream->out_curr.
> >
> > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > Reported-by: Ying Xu <yinxu@redhat.com>
> > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/sctp/stream.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > index 3892e76..30e7809 100644
> > --- a/net/sctp/stream.c
> > +++ b/net/sctp/stream.c
> > @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
> >       }
> >  }
> >
> > +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> > +{
> > +     size_t index = 0;
> > +
> > +     while (count--) {
> > +             if (elem == flex_array_get(fa, index))
> > +                     break;
> > +             index++;
> > +     }
> > +
> > +     return index;
> > +}
> > +
> >  /* Migrates chunks from stream queues to new stream queues if needed,
> >   * but not across associations. Also, removes those chunks to streams
> >   * higher than the new max.
> > @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> >
> >       if (stream->out) {
> >               fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> > +             if (stream->out_curr) {
> > +                     size_t index = fa_index(stream->out, stream->out_curr,
> > +                                             stream->outcnt);
> > +
> > +                     BUG_ON(index == stream->outcnt);
> > +                     stream->out_curr = flex_array_get(out, index);
> > +             }
> >               fa_free(stream->out);
> >       }
> >
> > --
> > 2.1.0
> >
> >
>
> I'm having a hard time understanding why, as I noted earlier, you don't just
> write a function in the flex_array code that can resize the number of elements
> in your array.  If you do that, you can avoid both all the copying, and the need
> to lookup the in-use pointer again
didn't want to touch the flex_array code, but you're right,
it would avoid both the copying and the lookup. I will
have a try tomorrow in flex_array.c, thanks.

>
> Neil
>

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Xin Long]

On Thu, Nov 29, 2018 at 9:50 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > Now when using stream reconfig to add out streams, stream->out
> > will get re-allocated, and all old streams' information will
> > be copied to the new ones and the old ones will be freed.
> >
> > So without stream->out_curr updated, next time when trying to
> > send from stream->out_curr stream, a panic would be caused.
> >
> > This patch is to check and update stream->out_curr when
> > allocating stream_out.
> >
> > v1->v2:
> >   - define fa_index() to get elem index from stream->out_curr.
> >
> > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > Reported-by: Ying Xu <yinxu@redhat.com>
> > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/sctp/stream.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > index 3892e76..30e7809 100644
> > --- a/net/sctp/stream.c
> > +++ b/net/sctp/stream.c
> > @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
> >       }
> >  }
> >
> > +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> > +{
> > +     size_t index = 0;
> > +
> > +     while (count--) {
> > +             if (elem = flex_array_get(fa, index))
> > +                     break;
> > +             index++;
> > +     }
> > +
> > +     return index;
> > +}
> > +
> >  /* Migrates chunks from stream queues to new stream queues if needed,
> >   * but not across associations. Also, removes those chunks to streams
> >   * higher than the new max.
> > @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> >
> >       if (stream->out) {
> >               fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> > +             if (stream->out_curr) {
> > +                     size_t index = fa_index(stream->out, stream->out_curr,
> > +                                             stream->outcnt);
> > +
> > +                     BUG_ON(index = stream->outcnt);
> > +                     stream->out_curr = flex_array_get(out, index);
> > +             }
> >               fa_free(stream->out);
> >       }
> >
> > --
> > 2.1.0
> >
> >
>
> I'm having a hard time understanding why, as I noted earlier, you don't just
> write a function in the flex_array code that can resize the number of elements
> in your array.  If you do that, you can avoid both all the copying, and the need
> to lookup the in-use pointer again
didn't want to touch the flex_array code, but you're right,
it would avoid both the copying and the lookup. I will
have a try tomorrow in flex_array.c, thanks.

>
> Neil
>

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Neil Horman]

On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> Now when using stream reconfig to add out streams, stream->out
> will get re-allocated, and all old streams' information will
> be copied to the new ones and the old ones will be freed.
> 
> So without stream->out_curr updated, next time when trying to
> send from stream->out_curr stream, a panic would be caused.
> 
> This patch is to check and update stream->out_curr when
> allocating stream_out.
> 
> v1->v2:
>   - define fa_index() to get elem index from stream->out_curr.
> 
> Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> Reported-by: Ying Xu <yinxu@redhat.com>
> Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/stream.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index 3892e76..30e7809 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
>  	}
>  }
>  
> +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> +{
> +	size_t index = 0;
> +
> +	while (count--) {
> +		if (elem == flex_array_get(fa, index))
> +			break;
> +		index++;
> +	}
> +
> +	return index;
> +}
> +
>  /* Migrates chunks from stream queues to new stream queues if needed,
>   * but not across associations. Also, removes those chunks to streams
>   * higher than the new max.
> @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
>  
>  	if (stream->out) {
>  		fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> +		if (stream->out_curr) {
> +			size_t index = fa_index(stream->out, stream->out_curr,
> +						stream->outcnt);
> +
> +			BUG_ON(index == stream->outcnt);
> +			stream->out_curr = flex_array_get(out, index);
> +		}
>  		fa_free(stream->out);
>  	}
>  
> -- 
> 2.1.0
> 
> 

This is the sort of thing I'm talking about. Its a little more code, but if you
augment the flex_array api like this, you can preform a resize operation on your
existing flex array, and you can avoid all the copying, and need to update
pointers maintained outside the array.  Note this code isn't tested at all, but
its close to what I think should work.


diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
index b94fa61b51fb..7fa1f27a91b5 100644
--- a/include/linux/flex_array.h
+++ b/include/linux/flex_array.h
@@ -73,6 +73,8 @@ struct flex_array {
 struct flex_array *flex_array_alloc(int element_size, unsigned int total,
 		gfp_t flags);
 
+struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags);
+
 /**
  * flex_array_prealloc() - Ensures that memory for the elements indexed in the
  * range defined by start and nr_elements has been allocated.
diff --git a/lib/flex_array.c b/lib/flex_array.c
index 2eed22fa507c..f8d54af3891b 100644
--- a/lib/flex_array.c
+++ b/lib/flex_array.c
@@ -109,6 +109,7 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
 	ret->total_nr_elements = total;
 	ret->elems_per_part = elems_per_part;
 	ret->reciprocal_elems = reciprocal_elems;
+	ret->elements_used = 0;
 	if (elements_fit_in_base(ret) && !(flags & __GFP_ZERO))
 		memset(&ret->parts[0], FLEX_ARRAY_FREE,
 						FLEX_ARRAY_BASE_BYTES_LEFT);
@@ -116,6 +117,53 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
 }
 EXPORT_SYMBOL(flex_array_alloc);
 
+static int flex_array_last_element_index(struct flex_array *fa)
+{
+	struct flex_array_part *part;
+	int part_nr;
+	int i,j;
+
+	if (elements_fit_in_base(fa)) {
+		part = (struct flex_array_part *)&fa->parts[0];
+		for (i = fa->elems_per_part; i >= 0; i--)
+			if (part->elements[i] != FLEX_ARRAY_FREE)
+				return i;
+	}
+
+	i = fa->total_nr_elements;
+	for (part_nr = 0; part_nr < FLEX_ARRAY_NR_BASE_PTRS; part_nr++) {
+		part = fa->parts[part_nr];
+		if (!part) {
+			i -= fa->elems_per_part;
+			continue;
+		}
+		for (j = fa->elems_per_part; j >= 0; j--, i--)
+			if (part->elements[j] != FLEX_ARRAY_FREE)
+				goto out;
+	}
+out:
+	return i;
+}
+
+struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags)
+{
+	if (total >= fa->total_nr_elements) {
+		/* Grow case */
+		if (total > max_size)
+			return ERR_PTR(-ETOOBIG);
+		fa->total_nr_elements = total;
+	} else {
+		/* Shrink case */
+		/* Drop any pages we don't need*/
+		flex_array_shrink(fa);
+		if (flex_array_last_element_index(fa) >= total)
+			return ERR_PTR(-ESIZE);
+		fa->total_nr_elements = total;
+	}
+	return fa;
+}
+EXPORT_SYMBOL(flex_array_resize);
+
 static int fa_element_to_part_nr(struct flex_array *fa,
 					unsigned int element_nr)
 {

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Neil Horman]

On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> Now when using stream reconfig to add out streams, stream->out
> will get re-allocated, and all old streams' information will
> be copied to the new ones and the old ones will be freed.
> 
> So without stream->out_curr updated, next time when trying to
> send from stream->out_curr stream, a panic would be caused.
> 
> This patch is to check and update stream->out_curr when
> allocating stream_out.
> 
> v1->v2:
>   - define fa_index() to get elem index from stream->out_curr.
> 
> Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> Reported-by: Ying Xu <yinxu@redhat.com>
> Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/stream.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index 3892e76..30e7809 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
>  	}
>  }
>  
> +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> +{
> +	size_t index = 0;
> +
> +	while (count--) {
> +		if (elem = flex_array_get(fa, index))
> +			break;
> +		index++;
> +	}
> +
> +	return index;
> +}
> +
>  /* Migrates chunks from stream queues to new stream queues if needed,
>   * but not across associations. Also, removes those chunks to streams
>   * higher than the new max.
> @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
>  
>  	if (stream->out) {
>  		fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> +		if (stream->out_curr) {
> +			size_t index = fa_index(stream->out, stream->out_curr,
> +						stream->outcnt);
> +
> +			BUG_ON(index = stream->outcnt);
> +			stream->out_curr = flex_array_get(out, index);
> +		}
>  		fa_free(stream->out);
>  	}
>  
> -- 
> 2.1.0
> 
> 

This is the sort of thing I'm talking about. Its a little more code, but if you
augment the flex_array api like this, you can preform a resize operation on your
existing flex array, and you can avoid all the copying, and need to update
pointers maintained outside the array.  Note this code isn't tested at all, but
its close to what I think should work.


diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
index b94fa61b51fb..7fa1f27a91b5 100644
--- a/include/linux/flex_array.h
+++ b/include/linux/flex_array.h
@@ -73,6 +73,8 @@ struct flex_array {
 struct flex_array *flex_array_alloc(int element_size, unsigned int total,
 		gfp_t flags);
 
+struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags);
+
 /**
  * flex_array_prealloc() - Ensures that memory for the elements indexed in the
  * range defined by start and nr_elements has been allocated.
diff --git a/lib/flex_array.c b/lib/flex_array.c
index 2eed22fa507c..f8d54af3891b 100644
--- a/lib/flex_array.c
+++ b/lib/flex_array.c
@@ -109,6 +109,7 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
 	ret->total_nr_elements = total;
 	ret->elems_per_part = elems_per_part;
 	ret->reciprocal_elems = reciprocal_elems;
+	ret->elements_used = 0;
 	if (elements_fit_in_base(ret) && !(flags & __GFP_ZERO))
 		memset(&ret->parts[0], FLEX_ARRAY_FREE,
 						FLEX_ARRAY_BASE_BYTES_LEFT);
@@ -116,6 +117,53 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
 }
 EXPORT_SYMBOL(flex_array_alloc);
 
+static int flex_array_last_element_index(struct flex_array *fa)
+{
+	struct flex_array_part *part;
+	int part_nr;
+	int i,j;
+
+	if (elements_fit_in_base(fa)) {
+		part = (struct flex_array_part *)&fa->parts[0];
+		for (i = fa->elems_per_part; i >= 0; i--)
+			if (part->elements[i] != FLEX_ARRAY_FREE)
+				return i;
+	}
+
+	i = fa->total_nr_elements;
+	for (part_nr = 0; part_nr < FLEX_ARRAY_NR_BASE_PTRS; part_nr++) {
+		part = fa->parts[part_nr];
+		if (!part) {
+			i -= fa->elems_per_part;
+			continue;
+		}
+		for (j = fa->elems_per_part; j >= 0; j--, i--)
+			if (part->elements[j] != FLEX_ARRAY_FREE)
+				goto out;
+	}
+out:
+	return i;
+}
+
+struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags)
+{
+	if (total >= fa->total_nr_elements) {
+		/* Grow case */
+		if (total > max_size)
+			return ERR_PTR(-ETOOBIG);
+		fa->total_nr_elements = total;
+	} else {
+		/* Shrink case */
+		/* Drop any pages we don't need*/
+		flex_array_shrink(fa);
+		if (flex_array_last_element_index(fa) >= total)
+			return ERR_PTR(-ESIZE);
+		fa->total_nr_elements = total;
+	}
+	return fa;
+}
+EXPORT_SYMBOL(flex_array_resize);
+
 static int fa_element_to_part_nr(struct flex_array *fa,
 					unsigned int element_nr)
 {

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Xin Long]

On Thu, Nov 29, 2018 at 11:39 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > Now when using stream reconfig to add out streams, stream->out
> > will get re-allocated, and all old streams' information will
> > be copied to the new ones and the old ones will be freed.
> >
> > So without stream->out_curr updated, next time when trying to
> > send from stream->out_curr stream, a panic would be caused.
> >
> > This patch is to check and update stream->out_curr when
> > allocating stream_out.
> >
> > v1->v2:
> >   - define fa_index() to get elem index from stream->out_curr.
> >
> > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > Reported-by: Ying Xu <yinxu@redhat.com>
> > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/sctp/stream.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > index 3892e76..30e7809 100644
> > --- a/net/sctp/stream.c
> > +++ b/net/sctp/stream.c
> > @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
> >       }
> >  }
> >
> > +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> > +{
> > +     size_t index = 0;
> > +
> > +     while (count--) {
> > +             if (elem == flex_array_get(fa, index))
> > +                     break;
> > +             index++;
> > +     }
> > +
> > +     return index;
> > +}
> > +
> >  /* Migrates chunks from stream queues to new stream queues if needed,
> >   * but not across associations. Also, removes those chunks to streams
> >   * higher than the new max.
> > @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> >
> >       if (stream->out) {
> >               fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> > +             if (stream->out_curr) {
> > +                     size_t index = fa_index(stream->out, stream->out_curr,
> > +                                             stream->outcnt);
> > +
> > +                     BUG_ON(index == stream->outcnt);
> > +                     stream->out_curr = flex_array_get(out, index);
> > +             }
> >               fa_free(stream->out);
> >       }
> >
> > --
> > 2.1.0
> >
> >
>
> This is the sort of thing I'm talking about. Its a little more code, but if you
> augment the flex_array api like this, you can preform a resize operation on your
> existing flex array, and you can avoid all the copying, and need to update
> pointers maintained outside the array.  Note this code isn't tested at all, but
> its close to what I think should work.
>
>
> diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
> index b94fa61b51fb..7fa1f27a91b5 100644
> --- a/include/linux/flex_array.h
> +++ b/include/linux/flex_array.h
> @@ -73,6 +73,8 @@ struct flex_array {
>  struct flex_array *flex_array_alloc(int element_size, unsigned int total,
>                 gfp_t flags);
>
> +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags);
> +
>  /**
>   * flex_array_prealloc() - Ensures that memory for the elements indexed in the
>   * range defined by start and nr_elements has been allocated.
> diff --git a/lib/flex_array.c b/lib/flex_array.c
> index 2eed22fa507c..f8d54af3891b 100644
> --- a/lib/flex_array.c
> +++ b/lib/flex_array.c
> @@ -109,6 +109,7 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
>         ret->total_nr_elements = total;
>         ret->elems_per_part = elems_per_part;
>         ret->reciprocal_elems = reciprocal_elems;
> +       ret->elements_used = 0;
>         if (elements_fit_in_base(ret) && !(flags & __GFP_ZERO))
>                 memset(&ret->parts[0], FLEX_ARRAY_FREE,
>                                                 FLEX_ARRAY_BASE_BYTES_LEFT);
> @@ -116,6 +117,53 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
>  }
>  EXPORT_SYMBOL(flex_array_alloc);
>
> +static int flex_array_last_element_index(struct flex_array *fa)
> +{
> +       struct flex_array_part *part;
> +       int part_nr;
> +       int i,j;
> +
> +       if (elements_fit_in_base(fa)) {
> +               part = (struct flex_array_part *)&fa->parts[0];
> +               for (i = fa->elems_per_part; i >= 0; i--)
> +                       if (part->elements[i] != FLEX_ARRAY_FREE)
> +                               return i;
> +       }
> +
> +       i = fa->total_nr_elements;
> +       for (part_nr = 0; part_nr < FLEX_ARRAY_NR_BASE_PTRS; part_nr++) {
> +               part = fa->parts[part_nr];
> +               if (!part) {
> +                       i -= fa->elems_per_part;
> +                       continue;
> +               }
> +               for (j = fa->elems_per_part; j >= 0; j--, i--)
> +                       if (part->elements[j] != FLEX_ARRAY_FREE)
> +                               goto out;
> +       }
> +out:
> +       return i;
> +}
> +
> +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags)
> +{
> +       if (total >= fa->total_nr_elements) {
> +               /* Grow case */
> +               if (total > max_size)
> +                       return ERR_PTR(-ETOOBIG);
> +               fa->total_nr_elements = total;
> +       } else {
> +               /* Shrink case */
> +               /* Drop any pages we don't need*/
> +               flex_array_shrink(fa);
> +               if (flex_array_last_element_index(fa) >= total)
> +                       return ERR_PTR(-ESIZE);
> +               fa->total_nr_elements = total;
> +       }
> +       return fa;
> +}
> +EXPORT_SYMBOL(flex_array_resize);
> +
>  static int fa_element_to_part_nr(struct flex_array *fa,
>                                         unsigned int element_nr)
>  {
I have a question about how it checks one part is free in
flex_array_shrink():
  part_is_free() is doing it by checking:
  if (part->elements[all] == FLEX_ARRAY_FREE) ...

What if the data in the array that users put is FLEX_ARRAY_FREE,
it will be treated as free, but in fact it's still in use?

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Xin Long]

On Thu, Nov 29, 2018 at 11:39 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > Now when using stream reconfig to add out streams, stream->out
> > will get re-allocated, and all old streams' information will
> > be copied to the new ones and the old ones will be freed.
> >
> > So without stream->out_curr updated, next time when trying to
> > send from stream->out_curr stream, a panic would be caused.
> >
> > This patch is to check and update stream->out_curr when
> > allocating stream_out.
> >
> > v1->v2:
> >   - define fa_index() to get elem index from stream->out_curr.
> >
> > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > Reported-by: Ying Xu <yinxu@redhat.com>
> > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/sctp/stream.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > index 3892e76..30e7809 100644
> > --- a/net/sctp/stream.c
> > +++ b/net/sctp/stream.c
> > @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
> >       }
> >  }
> >
> > +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> > +{
> > +     size_t index = 0;
> > +
> > +     while (count--) {
> > +             if (elem = flex_array_get(fa, index))
> > +                     break;
> > +             index++;
> > +     }
> > +
> > +     return index;
> > +}
> > +
> >  /* Migrates chunks from stream queues to new stream queues if needed,
> >   * but not across associations. Also, removes those chunks to streams
> >   * higher than the new max.
> > @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> >
> >       if (stream->out) {
> >               fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> > +             if (stream->out_curr) {
> > +                     size_t index = fa_index(stream->out, stream->out_curr,
> > +                                             stream->outcnt);
> > +
> > +                     BUG_ON(index = stream->outcnt);
> > +                     stream->out_curr = flex_array_get(out, index);
> > +             }
> >               fa_free(stream->out);
> >       }
> >
> > --
> > 2.1.0
> >
> >
>
> This is the sort of thing I'm talking about. Its a little more code, but if you
> augment the flex_array api like this, you can preform a resize operation on your
> existing flex array, and you can avoid all the copying, and need to update
> pointers maintained outside the array.  Note this code isn't tested at all, but
> its close to what I think should work.
>
>
> diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
> index b94fa61b51fb..7fa1f27a91b5 100644
> --- a/include/linux/flex_array.h
> +++ b/include/linux/flex_array.h
> @@ -73,6 +73,8 @@ struct flex_array {
>  struct flex_array *flex_array_alloc(int element_size, unsigned int total,
>                 gfp_t flags);
>
> +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags);
> +
>  /**
>   * flex_array_prealloc() - Ensures that memory for the elements indexed in the
>   * range defined by start and nr_elements has been allocated.
> diff --git a/lib/flex_array.c b/lib/flex_array.c
> index 2eed22fa507c..f8d54af3891b 100644
> --- a/lib/flex_array.c
> +++ b/lib/flex_array.c
> @@ -109,6 +109,7 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
>         ret->total_nr_elements = total;
>         ret->elems_per_part = elems_per_part;
>         ret->reciprocal_elems = reciprocal_elems;
> +       ret->elements_used = 0;
>         if (elements_fit_in_base(ret) && !(flags & __GFP_ZERO))
>                 memset(&ret->parts[0], FLEX_ARRAY_FREE,
>                                                 FLEX_ARRAY_BASE_BYTES_LEFT);
> @@ -116,6 +117,53 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
>  }
>  EXPORT_SYMBOL(flex_array_alloc);
>
> +static int flex_array_last_element_index(struct flex_array *fa)
> +{
> +       struct flex_array_part *part;
> +       int part_nr;
> +       int i,j;
> +
> +       if (elements_fit_in_base(fa)) {
> +               part = (struct flex_array_part *)&fa->parts[0];
> +               for (i = fa->elems_per_part; i >= 0; i--)
> +                       if (part->elements[i] != FLEX_ARRAY_FREE)
> +                               return i;
> +       }
> +
> +       i = fa->total_nr_elements;
> +       for (part_nr = 0; part_nr < FLEX_ARRAY_NR_BASE_PTRS; part_nr++) {
> +               part = fa->parts[part_nr];
> +               if (!part) {
> +                       i -= fa->elems_per_part;
> +                       continue;
> +               }
> +               for (j = fa->elems_per_part; j >= 0; j--, i--)
> +                       if (part->elements[j] != FLEX_ARRAY_FREE)
> +                               goto out;
> +       }
> +out:
> +       return i;
> +}
> +
> +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags)
> +{
> +       if (total >= fa->total_nr_elements) {
> +               /* Grow case */
> +               if (total > max_size)
> +                       return ERR_PTR(-ETOOBIG);
> +               fa->total_nr_elements = total;
> +       } else {
> +               /* Shrink case */
> +               /* Drop any pages we don't need*/
> +               flex_array_shrink(fa);
> +               if (flex_array_last_element_index(fa) >= total)
> +                       return ERR_PTR(-ESIZE);
> +               fa->total_nr_elements = total;
> +       }
> +       return fa;
> +}
> +EXPORT_SYMBOL(flex_array_resize);
> +
>  static int fa_element_to_part_nr(struct flex_array *fa,
>                                         unsigned int element_nr)
>  {
I have a question about how it checks one part is free in
flex_array_shrink():
  part_is_free() is doing it by checking:
  if (part->elements[all] = FLEX_ARRAY_FREE) ...

What if the data in the array that users put is FLEX_ARRAY_FREE,
it will be treated as free, but in fact it's still in use?

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Neil Horman]

On Fri, Nov 30, 2018 at 03:22:39PM +0900, Xin Long wrote:
> On Thu, Nov 29, 2018 at 11:39 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > > Now when using stream reconfig to add out streams, stream->out
> > > will get re-allocated, and all old streams' information will
> > > be copied to the new ones and the old ones will be freed.
> > >
> > > So without stream->out_curr updated, next time when trying to
> > > send from stream->out_curr stream, a panic would be caused.
> > >
> > > This patch is to check and update stream->out_curr when
> > > allocating stream_out.
> > >
> > > v1->v2:
> > >   - define fa_index() to get elem index from stream->out_curr.
> > >
> > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > > Reported-by: Ying Xu <yinxu@redhat.com>
> > > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > >  net/sctp/stream.c | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > >
> > > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > > index 3892e76..30e7809 100644
> > > --- a/net/sctp/stream.c
> > > +++ b/net/sctp/stream.c
> > > @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
> > >       }
> > >  }
> > >
> > > +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> > > +{
> > > +     size_t index = 0;
> > > +
> > > +     while (count--) {
> > > +             if (elem == flex_array_get(fa, index))
> > > +                     break;
> > > +             index++;
> > > +     }
> > > +
> > > +     return index;
> > > +}
> > > +
> > >  /* Migrates chunks from stream queues to new stream queues if needed,
> > >   * but not across associations. Also, removes those chunks to streams
> > >   * higher than the new max.
> > > @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> > >
> > >       if (stream->out) {
> > >               fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> > > +             if (stream->out_curr) {
> > > +                     size_t index = fa_index(stream->out, stream->out_curr,
> > > +                                             stream->outcnt);
> > > +
> > > +                     BUG_ON(index == stream->outcnt);
> > > +                     stream->out_curr = flex_array_get(out, index);
> > > +             }
> > >               fa_free(stream->out);
> > >       }
> > >
> > > --
> > > 2.1.0
> > >
> > >
> >
> > This is the sort of thing I'm talking about. Its a little more code, but if you
> > augment the flex_array api like this, you can preform a resize operation on your
> > existing flex array, and you can avoid all the copying, and need to update
> > pointers maintained outside the array.  Note this code isn't tested at all, but
> > its close to what I think should work.
> >
> >
> > diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
> > index b94fa61b51fb..7fa1f27a91b5 100644
> > --- a/include/linux/flex_array.h
> > +++ b/include/linux/flex_array.h
> > @@ -73,6 +73,8 @@ struct flex_array {
> >  struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> >                 gfp_t flags);
> >
> > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags);
> > +
> >  /**
> >   * flex_array_prealloc() - Ensures that memory for the elements indexed in the
> >   * range defined by start and nr_elements has been allocated.
> > diff --git a/lib/flex_array.c b/lib/flex_array.c
> > index 2eed22fa507c..f8d54af3891b 100644
> > --- a/lib/flex_array.c
> > +++ b/lib/flex_array.c
> > @@ -109,6 +109,7 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> >         ret->total_nr_elements = total;
> >         ret->elems_per_part = elems_per_part;
> >         ret->reciprocal_elems = reciprocal_elems;
> > +       ret->elements_used = 0;
> >         if (elements_fit_in_base(ret) && !(flags & __GFP_ZERO))
> >                 memset(&ret->parts[0], FLEX_ARRAY_FREE,
> >                                                 FLEX_ARRAY_BASE_BYTES_LEFT);
> > @@ -116,6 +117,53 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> >  }
> >  EXPORT_SYMBOL(flex_array_alloc);
> >
> > +static int flex_array_last_element_index(struct flex_array *fa)
> > +{
> > +       struct flex_array_part *part;
> > +       int part_nr;
> > +       int i,j;
> > +
> > +       if (elements_fit_in_base(fa)) {
> > +               part = (struct flex_array_part *)&fa->parts[0];
> > +               for (i = fa->elems_per_part; i >= 0; i--)
> > +                       if (part->elements[i] != FLEX_ARRAY_FREE)
> > +                               return i;
> > +       }
> > +
> > +       i = fa->total_nr_elements;
> > +       for (part_nr = 0; part_nr < FLEX_ARRAY_NR_BASE_PTRS; part_nr++) {
> > +               part = fa->parts[part_nr];
> > +               if (!part) {
> > +                       i -= fa->elems_per_part;
> > +                       continue;
> > +               }
> > +               for (j = fa->elems_per_part; j >= 0; j--, i--)
> > +                       if (part->elements[j] != FLEX_ARRAY_FREE)
> > +                               goto out;
> > +       }
> > +out:
> > +       return i;
> > +}
> > +
> > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags)
> > +{
> > +       if (total >= fa->total_nr_elements) {
> > +               /* Grow case */
> > +               if (total > max_size)
> > +                       return ERR_PTR(-ETOOBIG);
> > +               fa->total_nr_elements = total;
> > +       } else {
> > +               /* Shrink case */
> > +               /* Drop any pages we don't need*/
> > +               flex_array_shrink(fa);
> > +               if (flex_array_last_element_index(fa) >= total)
> > +                       return ERR_PTR(-ESIZE);
> > +               fa->total_nr_elements = total;
> > +       }
> > +       return fa;
> > +}
> > +EXPORT_SYMBOL(flex_array_resize);
> > +
> >  static int fa_element_to_part_nr(struct flex_array *fa,
> >                                         unsigned int element_nr)
> >  {
> I have a question about how it checks one part is free in
> flex_array_shrink():
>   part_is_free() is doing it by checking:
>   if (part->elements[all] == FLEX_ARRAY_FREE) ...
> 
> What if the data in the array that users put is FLEX_ARRAY_FREE,
> it will be treated as free, but in fact it's still in use?
> 
I noticed that too, I think they're taking an unsafe shortcut honestly, assuming
that the FLEX_ARRAY_FREE poison value in the first byte is indicative of the
entire part being free.  For this use case its not likely a problem, because the
first element of the stream is a pointer, and so its unlikely to start with the
FLEX_ARRAY_FREE value, but it could suffer from aliasing in other cases.  In the
current design they should really be doing a memory comparison of the entire
object length to a buffer filled with the poison value to determine free status. 

I would suggest not worrying about it for the purposes of this patch, but I
think I'll write a fix for the flex_array code that adds a byte to the head of
whatever object is bing stored to be reserved for free / in-use indicators.
Neil

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Neil Horman]

On Fri, Nov 30, 2018 at 03:22:39PM +0900, Xin Long wrote:
> On Thu, Nov 29, 2018 at 11:39 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > > Now when using stream reconfig to add out streams, stream->out
> > > will get re-allocated, and all old streams' information will
> > > be copied to the new ones and the old ones will be freed.
> > >
> > > So without stream->out_curr updated, next time when trying to
> > > send from stream->out_curr stream, a panic would be caused.
> > >
> > > This patch is to check and update stream->out_curr when
> > > allocating stream_out.
> > >
> > > v1->v2:
> > >   - define fa_index() to get elem index from stream->out_curr.
> > >
> > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > > Reported-by: Ying Xu <yinxu@redhat.com>
> > > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > >  net/sctp/stream.c | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > >
> > > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > > index 3892e76..30e7809 100644
> > > --- a/net/sctp/stream.c
> > > +++ b/net/sctp/stream.c
> > > @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
> > >       }
> > >  }
> > >
> > > +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> > > +{
> > > +     size_t index = 0;
> > > +
> > > +     while (count--) {
> > > +             if (elem = flex_array_get(fa, index))
> > > +                     break;
> > > +             index++;
> > > +     }
> > > +
> > > +     return index;
> > > +}
> > > +
> > >  /* Migrates chunks from stream queues to new stream queues if needed,
> > >   * but not across associations. Also, removes those chunks to streams
> > >   * higher than the new max.
> > > @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> > >
> > >       if (stream->out) {
> > >               fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> > > +             if (stream->out_curr) {
> > > +                     size_t index = fa_index(stream->out, stream->out_curr,
> > > +                                             stream->outcnt);
> > > +
> > > +                     BUG_ON(index = stream->outcnt);
> > > +                     stream->out_curr = flex_array_get(out, index);
> > > +             }
> > >               fa_free(stream->out);
> > >       }
> > >
> > > --
> > > 2.1.0
> > >
> > >
> >
> > This is the sort of thing I'm talking about. Its a little more code, but if you
> > augment the flex_array api like this, you can preform a resize operation on your
> > existing flex array, and you can avoid all the copying, and need to update
> > pointers maintained outside the array.  Note this code isn't tested at all, but
> > its close to what I think should work.
> >
> >
> > diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
> > index b94fa61b51fb..7fa1f27a91b5 100644
> > --- a/include/linux/flex_array.h
> > +++ b/include/linux/flex_array.h
> > @@ -73,6 +73,8 @@ struct flex_array {
> >  struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> >                 gfp_t flags);
> >
> > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags);
> > +
> >  /**
> >   * flex_array_prealloc() - Ensures that memory for the elements indexed in the
> >   * range defined by start and nr_elements has been allocated.
> > diff --git a/lib/flex_array.c b/lib/flex_array.c
> > index 2eed22fa507c..f8d54af3891b 100644
> > --- a/lib/flex_array.c
> > +++ b/lib/flex_array.c
> > @@ -109,6 +109,7 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> >         ret->total_nr_elements = total;
> >         ret->elems_per_part = elems_per_part;
> >         ret->reciprocal_elems = reciprocal_elems;
> > +       ret->elements_used = 0;
> >         if (elements_fit_in_base(ret) && !(flags & __GFP_ZERO))
> >                 memset(&ret->parts[0], FLEX_ARRAY_FREE,
> >                                                 FLEX_ARRAY_BASE_BYTES_LEFT);
> > @@ -116,6 +117,53 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> >  }
> >  EXPORT_SYMBOL(flex_array_alloc);
> >
> > +static int flex_array_last_element_index(struct flex_array *fa)
> > +{
> > +       struct flex_array_part *part;
> > +       int part_nr;
> > +       int i,j;
> > +
> > +       if (elements_fit_in_base(fa)) {
> > +               part = (struct flex_array_part *)&fa->parts[0];
> > +               for (i = fa->elems_per_part; i >= 0; i--)
> > +                       if (part->elements[i] != FLEX_ARRAY_FREE)
> > +                               return i;
> > +       }
> > +
> > +       i = fa->total_nr_elements;
> > +       for (part_nr = 0; part_nr < FLEX_ARRAY_NR_BASE_PTRS; part_nr++) {
> > +               part = fa->parts[part_nr];
> > +               if (!part) {
> > +                       i -= fa->elems_per_part;
> > +                       continue;
> > +               }
> > +               for (j = fa->elems_per_part; j >= 0; j--, i--)
> > +                       if (part->elements[j] != FLEX_ARRAY_FREE)
> > +                               goto out;
> > +       }
> > +out:
> > +       return i;
> > +}
> > +
> > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags)
> > +{
> > +       if (total >= fa->total_nr_elements) {
> > +               /* Grow case */
> > +               if (total > max_size)
> > +                       return ERR_PTR(-ETOOBIG);
> > +               fa->total_nr_elements = total;
> > +       } else {
> > +               /* Shrink case */
> > +               /* Drop any pages we don't need*/
> > +               flex_array_shrink(fa);
> > +               if (flex_array_last_element_index(fa) >= total)
> > +                       return ERR_PTR(-ESIZE);
> > +               fa->total_nr_elements = total;
> > +       }
> > +       return fa;
> > +}
> > +EXPORT_SYMBOL(flex_array_resize);
> > +
> >  static int fa_element_to_part_nr(struct flex_array *fa,
> >                                         unsigned int element_nr)
> >  {
> I have a question about how it checks one part is free in
> flex_array_shrink():
>   part_is_free() is doing it by checking:
>   if (part->elements[all] = FLEX_ARRAY_FREE) ...
> 
> What if the data in the array that users put is FLEX_ARRAY_FREE,
> it will be treated as free, but in fact it's still in use?
> 
I noticed that too, I think they're taking an unsafe shortcut honestly, assuming
that the FLEX_ARRAY_FREE poison value in the first byte is indicative of the
entire part being free.  For this use case its not likely a problem, because the
first element of the stream is a pointer, and so its unlikely to start with the
FLEX_ARRAY_FREE value, but it could suffer from aliasing in other cases.  In the
current design they should really be doing a memory comparison of the entire
object length to a buffer filled with the poison value to determine free status. 

I would suggest not worrying about it for the purposes of this patch, but I
think I'll write a fix for the flex_array code that adds a byte to the head of
whatever object is bing stored to be reserved for free / in-use indicators.
Neil

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Xin Long]

On Fri, Nov 30, 2018 at 9:21 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Fri, Nov 30, 2018 at 03:22:39PM +0900, Xin Long wrote:
> > On Thu, Nov 29, 2018 at 11:39 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > > > Now when using stream reconfig to add out streams, stream->out
> > > > will get re-allocated, and all old streams' information will
> > > > be copied to the new ones and the old ones will be freed.
> > > >
> > > > So without stream->out_curr updated, next time when trying to
> > > > send from stream->out_curr stream, a panic would be caused.
> > > >
> > > > This patch is to check and update stream->out_curr when
> > > > allocating stream_out.
> > > >
> > > > v1->v2:
> > > >   - define fa_index() to get elem index from stream->out_curr.
> > > >
> > > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > > > Reported-by: Ying Xu <yinxu@redhat.com>
> > > > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > ---
> > > >  net/sctp/stream.c | 20 ++++++++++++++++++++
> > > >  1 file changed, 20 insertions(+)
> > > >
> > > > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > > > index 3892e76..30e7809 100644
> > > > --- a/net/sctp/stream.c
> > > > +++ b/net/sctp/stream.c
> > > > @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
> > > >       }
> > > >  }
> > > >
> > > > +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> > > > +{
> > > > +     size_t index = 0;
> > > > +
> > > > +     while (count--) {
> > > > +             if (elem == flex_array_get(fa, index))
> > > > +                     break;
> > > > +             index++;
> > > > +     }
> > > > +
> > > > +     return index;
> > > > +}
> > > > +
> > > >  /* Migrates chunks from stream queues to new stream queues if needed,
> > > >   * but not across associations. Also, removes those chunks to streams
> > > >   * higher than the new max.
> > > > @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> > > >
> > > >       if (stream->out) {
> > > >               fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> > > > +             if (stream->out_curr) {
> > > > +                     size_t index = fa_index(stream->out, stream->out_curr,
> > > > +                                             stream->outcnt);
> > > > +
> > > > +                     BUG_ON(index == stream->outcnt);
> > > > +                     stream->out_curr = flex_array_get(out, index);
> > > > +             }
> > > >               fa_free(stream->out);
> > > >       }
> > > >
> > > > --
> > > > 2.1.0
> > > >
> > > >
> > >
> > > This is the sort of thing I'm talking about. Its a little more code, but if you
> > > augment the flex_array api like this, you can preform a resize operation on your
> > > existing flex array, and you can avoid all the copying, and need to update
> > > pointers maintained outside the array.  Note this code isn't tested at all, but
> > > its close to what I think should work.
> > >
> > >
> > > diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
> > > index b94fa61b51fb..7fa1f27a91b5 100644
> > > --- a/include/linux/flex_array.h
> > > +++ b/include/linux/flex_array.h
> > > @@ -73,6 +73,8 @@ struct flex_array {
> > >  struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > >                 gfp_t flags);
> > >
> > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags);
> > > +
> > >  /**
> > >   * flex_array_prealloc() - Ensures that memory for the elements indexed in the
> > >   * range defined by start and nr_elements has been allocated.
> > > diff --git a/lib/flex_array.c b/lib/flex_array.c
> > > index 2eed22fa507c..f8d54af3891b 100644
> > > --- a/lib/flex_array.c
> > > +++ b/lib/flex_array.c
> > > @@ -109,6 +109,7 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > >         ret->total_nr_elements = total;
> > >         ret->elems_per_part = elems_per_part;
> > >         ret->reciprocal_elems = reciprocal_elems;
> > > +       ret->elements_used = 0;
> > >         if (elements_fit_in_base(ret) && !(flags & __GFP_ZERO))
> > >                 memset(&ret->parts[0], FLEX_ARRAY_FREE,
> > >                                                 FLEX_ARRAY_BASE_BYTES_LEFT);
> > > @@ -116,6 +117,53 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > >  }
> > >  EXPORT_SYMBOL(flex_array_alloc);
> > >
> > > +static int flex_array_last_element_index(struct flex_array *fa)
> > > +{
> > > +       struct flex_array_part *part;
> > > +       int part_nr;
> > > +       int i,j;
> > > +
> > > +       if (elements_fit_in_base(fa)) {
> > > +               part = (struct flex_array_part *)&fa->parts[0];
> > > +               for (i = fa->elems_per_part; i >= 0; i--)
> > > +                       if (part->elements[i] != FLEX_ARRAY_FREE)
> > > +                               return i;
> > > +       }
> > > +
> > > +       i = fa->total_nr_elements;
> > > +       for (part_nr = 0; part_nr < FLEX_ARRAY_NR_BASE_PTRS; part_nr++) {
> > > +               part = fa->parts[part_nr];
> > > +               if (!part) {
> > > +                       i -= fa->elems_per_part;
> > > +                       continue;
> > > +               }
> > > +               for (j = fa->elems_per_part; j >= 0; j--, i--)
> > > +                       if (part->elements[j] != FLEX_ARRAY_FREE)
> > > +                               goto out;
> > > +       }
> > > +out:
> > > +       return i;
> > > +}
> > > +
> > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags)
> > > +{
> > > +       if (total >= fa->total_nr_elements) {
> > > +               /* Grow case */
> > > +               if (total > max_size)
> > > +                       return ERR_PTR(-ETOOBIG);
> > > +               fa->total_nr_elements = total;
> > > +       } else {
> > > +               /* Shrink case */
> > > +               /* Drop any pages we don't need*/
> > > +               flex_array_shrink(fa);
> > > +               if (flex_array_last_element_index(fa) >= total)
> > > +                       return ERR_PTR(-ESIZE);
> > > +               fa->total_nr_elements = total;
> > > +       }
> > > +       return fa;
> > > +}
> > > +EXPORT_SYMBOL(flex_array_resize);
> > > +
> > >  static int fa_element_to_part_nr(struct flex_array *fa,
> > >                                         unsigned int element_nr)
> > >  {
> > I have a question about how it checks one part is free in
> > flex_array_shrink():
> >   part_is_free() is doing it by checking:
> >   if (part->elements[all] == FLEX_ARRAY_FREE) ...
> >
> > What if the data in the array that users put is FLEX_ARRAY_FREE,
> > it will be treated as free, but in fact it's still in use?
> >
> I noticed that too, I think they're taking an unsafe shortcut honestly, assuming
> that the FLEX_ARRAY_FREE poison value in the first byte is indicative of the
> entire part being free.  For this use case its not likely a problem, because the
> first element of the stream is a pointer, and so its unlikely to start with the
> FLEX_ARRAY_FREE value, but it could suffer from aliasing in other cases.  In the
> current design they should really be doing a memory comparison of the entire
> object length to a buffer filled with the poison value to determine free status.
Here's still another problem, I'm not sure if you have noticed that:
When (elements_fit_in_base(fa)) or data_size <= FLEX_ARRAY_BASE_BYTES_LEFT
  part_0 = (struct flex_array_part *)&fa->parts[0]
otherwise,
  part_0 = fa->parts[0].

That means when the new out streams' size is greater than
FLEX_ARRAY_BASE_BYTES_LEFT, resize is not that easy to just set
fa->total_nr_elements. We have to do something like what flex_array_alloc
does, and the old elements memory will have to be lost.
With that, this issue won't be fixed.

After checking the flex_array code, I think flex_array is not yet working
for dynamical resize, which is not its goal either.

>
> I would suggest not worrying about it for the purposes of this patch, but I
> think I'll write a fix for the flex_array code that adds a byte to the head of
> whatever object is bing stored to be reserved for free / in-use indicators.
> Neil
>

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Xin Long]

On Fri, Nov 30, 2018 at 9:21 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Fri, Nov 30, 2018 at 03:22:39PM +0900, Xin Long wrote:
> > On Thu, Nov 29, 2018 at 11:39 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > > > Now when using stream reconfig to add out streams, stream->out
> > > > will get re-allocated, and all old streams' information will
> > > > be copied to the new ones and the old ones will be freed.
> > > >
> > > > So without stream->out_curr updated, next time when trying to
> > > > send from stream->out_curr stream, a panic would be caused.
> > > >
> > > > This patch is to check and update stream->out_curr when
> > > > allocating stream_out.
> > > >
> > > > v1->v2:
> > > >   - define fa_index() to get elem index from stream->out_curr.
> > > >
> > > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > > > Reported-by: Ying Xu <yinxu@redhat.com>
> > > > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > ---
> > > >  net/sctp/stream.c | 20 ++++++++++++++++++++
> > > >  1 file changed, 20 insertions(+)
> > > >
> > > > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > > > index 3892e76..30e7809 100644
> > > > --- a/net/sctp/stream.c
> > > > +++ b/net/sctp/stream.c
> > > > @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
> > > >       }
> > > >  }
> > > >
> > > > +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> > > > +{
> > > > +     size_t index = 0;
> > > > +
> > > > +     while (count--) {
> > > > +             if (elem = flex_array_get(fa, index))
> > > > +                     break;
> > > > +             index++;
> > > > +     }
> > > > +
> > > > +     return index;
> > > > +}
> > > > +
> > > >  /* Migrates chunks from stream queues to new stream queues if needed,
> > > >   * but not across associations. Also, removes those chunks to streams
> > > >   * higher than the new max.
> > > > @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> > > >
> > > >       if (stream->out) {
> > > >               fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> > > > +             if (stream->out_curr) {
> > > > +                     size_t index = fa_index(stream->out, stream->out_curr,
> > > > +                                             stream->outcnt);
> > > > +
> > > > +                     BUG_ON(index = stream->outcnt);
> > > > +                     stream->out_curr = flex_array_get(out, index);
> > > > +             }
> > > >               fa_free(stream->out);
> > > >       }
> > > >
> > > > --
> > > > 2.1.0
> > > >
> > > >
> > >
> > > This is the sort of thing I'm talking about. Its a little more code, but if you
> > > augment the flex_array api like this, you can preform a resize operation on your
> > > existing flex array, and you can avoid all the copying, and need to update
> > > pointers maintained outside the array.  Note this code isn't tested at all, but
> > > its close to what I think should work.
> > >
> > >
> > > diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
> > > index b94fa61b51fb..7fa1f27a91b5 100644
> > > --- a/include/linux/flex_array.h
> > > +++ b/include/linux/flex_array.h
> > > @@ -73,6 +73,8 @@ struct flex_array {
> > >  struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > >                 gfp_t flags);
> > >
> > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags);
> > > +
> > >  /**
> > >   * flex_array_prealloc() - Ensures that memory for the elements indexed in the
> > >   * range defined by start and nr_elements has been allocated.
> > > diff --git a/lib/flex_array.c b/lib/flex_array.c
> > > index 2eed22fa507c..f8d54af3891b 100644
> > > --- a/lib/flex_array.c
> > > +++ b/lib/flex_array.c
> > > @@ -109,6 +109,7 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > >         ret->total_nr_elements = total;
> > >         ret->elems_per_part = elems_per_part;
> > >         ret->reciprocal_elems = reciprocal_elems;
> > > +       ret->elements_used = 0;
> > >         if (elements_fit_in_base(ret) && !(flags & __GFP_ZERO))
> > >                 memset(&ret->parts[0], FLEX_ARRAY_FREE,
> > >                                                 FLEX_ARRAY_BASE_BYTES_LEFT);
> > > @@ -116,6 +117,53 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > >  }
> > >  EXPORT_SYMBOL(flex_array_alloc);
> > >
> > > +static int flex_array_last_element_index(struct flex_array *fa)
> > > +{
> > > +       struct flex_array_part *part;
> > > +       int part_nr;
> > > +       int i,j;
> > > +
> > > +       if (elements_fit_in_base(fa)) {
> > > +               part = (struct flex_array_part *)&fa->parts[0];
> > > +               for (i = fa->elems_per_part; i >= 0; i--)
> > > +                       if (part->elements[i] != FLEX_ARRAY_FREE)
> > > +                               return i;
> > > +       }
> > > +
> > > +       i = fa->total_nr_elements;
> > > +       for (part_nr = 0; part_nr < FLEX_ARRAY_NR_BASE_PTRS; part_nr++) {
> > > +               part = fa->parts[part_nr];
> > > +               if (!part) {
> > > +                       i -= fa->elems_per_part;
> > > +                       continue;
> > > +               }
> > > +               for (j = fa->elems_per_part; j >= 0; j--, i--)
> > > +                       if (part->elements[j] != FLEX_ARRAY_FREE)
> > > +                               goto out;
> > > +       }
> > > +out:
> > > +       return i;
> > > +}
> > > +
> > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags)
> > > +{
> > > +       if (total >= fa->total_nr_elements) {
> > > +               /* Grow case */
> > > +               if (total > max_size)
> > > +                       return ERR_PTR(-ETOOBIG);
> > > +               fa->total_nr_elements = total;
> > > +       } else {
> > > +               /* Shrink case */
> > > +               /* Drop any pages we don't need*/
> > > +               flex_array_shrink(fa);
> > > +               if (flex_array_last_element_index(fa) >= total)
> > > +                       return ERR_PTR(-ESIZE);
> > > +               fa->total_nr_elements = total;
> > > +       }
> > > +       return fa;
> > > +}
> > > +EXPORT_SYMBOL(flex_array_resize);
> > > +
> > >  static int fa_element_to_part_nr(struct flex_array *fa,
> > >                                         unsigned int element_nr)
> > >  {
> > I have a question about how it checks one part is free in
> > flex_array_shrink():
> >   part_is_free() is doing it by checking:
> >   if (part->elements[all] = FLEX_ARRAY_FREE) ...
> >
> > What if the data in the array that users put is FLEX_ARRAY_FREE,
> > it will be treated as free, but in fact it's still in use?
> >
> I noticed that too, I think they're taking an unsafe shortcut honestly, assuming
> that the FLEX_ARRAY_FREE poison value in the first byte is indicative of the
> entire part being free.  For this use case its not likely a problem, because the
> first element of the stream is a pointer, and so its unlikely to start with the
> FLEX_ARRAY_FREE value, but it could suffer from aliasing in other cases.  In the
> current design they should really be doing a memory comparison of the entire
> object length to a buffer filled with the poison value to determine free status.
Here's still another problem, I'm not sure if you have noticed that:
When (elements_fit_in_base(fa)) or data_size <= FLEX_ARRAY_BASE_BYTES_LEFT
  part_0 = (struct flex_array_part *)&fa->parts[0]
otherwise,
  part_0 = fa->parts[0].

That means when the new out streams' size is greater than
FLEX_ARRAY_BASE_BYTES_LEFT, resize is not that easy to just set
fa->total_nr_elements. We have to do something like what flex_array_alloc
does, and the old elements memory will have to be lost.
With that, this issue won't be fixed.

After checking the flex_array code, I think flex_array is not yet working
for dynamical resize, which is not its goal either.

>
> I would suggest not worrying about it for the purposes of this patch, but I
> think I'll write a fix for the flex_array code that adds a byte to the head of
> whatever object is bing stored to be reserved for free / in-use indicators.
> Neil
>

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Neil Horman]

On Fri, Nov 30, 2018 at 10:48:10PM +0900, Xin Long wrote:
> On Fri, Nov 30, 2018 at 9:21 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Fri, Nov 30, 2018 at 03:22:39PM +0900, Xin Long wrote:
> > > On Thu, Nov 29, 2018 at 11:39 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > >
> > > > On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > > > > Now when using stream reconfig to add out streams, stream->out
> > > > > will get re-allocated, and all old streams' information will
> > > > > be copied to the new ones and the old ones will be freed.
> > > > >
> > > > > So without stream->out_curr updated, next time when trying to
> > > > > send from stream->out_curr stream, a panic would be caused.
> > > > >
> > > > > This patch is to check and update stream->out_curr when
> > > > > allocating stream_out.
> > > > >
> > > > > v1->v2:
> > > > >   - define fa_index() to get elem index from stream->out_curr.
> > > > >
> > > > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > > > > Reported-by: Ying Xu <yinxu@redhat.com>
> > > > > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > ---
> > > > >  net/sctp/stream.c | 20 ++++++++++++++++++++
> > > > >  1 file changed, 20 insertions(+)
> > > > >
> > > > > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > > > > index 3892e76..30e7809 100644
> > > > > --- a/net/sctp/stream.c
> > > > > +++ b/net/sctp/stream.c
> > > > > @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
> > > > >       }
> > > > >  }
> > > > >
> > > > > +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> > > > > +{
> > > > > +     size_t index = 0;
> > > > > +
> > > > > +     while (count--) {
> > > > > +             if (elem == flex_array_get(fa, index))
> > > > > +                     break;
> > > > > +             index++;
> > > > > +     }
> > > > > +
> > > > > +     return index;
> > > > > +}
> > > > > +
> > > > >  /* Migrates chunks from stream queues to new stream queues if needed,
> > > > >   * but not across associations. Also, removes those chunks to streams
> > > > >   * higher than the new max.
> > > > > @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> > > > >
> > > > >       if (stream->out) {
> > > > >               fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> > > > > +             if (stream->out_curr) {
> > > > > +                     size_t index = fa_index(stream->out, stream->out_curr,
> > > > > +                                             stream->outcnt);
> > > > > +
> > > > > +                     BUG_ON(index == stream->outcnt);
> > > > > +                     stream->out_curr = flex_array_get(out, index);
> > > > > +             }
> > > > >               fa_free(stream->out);
> > > > >       }
> > > > >
> > > > > --
> > > > > 2.1.0
> > > > >
> > > > >
> > > >
> > > > This is the sort of thing I'm talking about. Its a little more code, but if you
> > > > augment the flex_array api like this, you can preform a resize operation on your
> > > > existing flex array, and you can avoid all the copying, and need to update
> > > > pointers maintained outside the array.  Note this code isn't tested at all, but
> > > > its close to what I think should work.
> > > >
> > > >
> > > > diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
> > > > index b94fa61b51fb..7fa1f27a91b5 100644
> > > > --- a/include/linux/flex_array.h
> > > > +++ b/include/linux/flex_array.h
> > > > @@ -73,6 +73,8 @@ struct flex_array {
> > > >  struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > >                 gfp_t flags);
> > > >
> > > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags);
> > > > +
> > > >  /**
> > > >   * flex_array_prealloc() - Ensures that memory for the elements indexed in the
> > > >   * range defined by start and nr_elements has been allocated.
> > > > diff --git a/lib/flex_array.c b/lib/flex_array.c
> > > > index 2eed22fa507c..f8d54af3891b 100644
> > > > --- a/lib/flex_array.c
> > > > +++ b/lib/flex_array.c
> > > > @@ -109,6 +109,7 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > >         ret->total_nr_elements = total;
> > > >         ret->elems_per_part = elems_per_part;
> > > >         ret->reciprocal_elems = reciprocal_elems;
> > > > +       ret->elements_used = 0;
> > > >         if (elements_fit_in_base(ret) && !(flags & __GFP_ZERO))
> > > >                 memset(&ret->parts[0], FLEX_ARRAY_FREE,
> > > >                                                 FLEX_ARRAY_BASE_BYTES_LEFT);
> > > > @@ -116,6 +117,53 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > >  }
> > > >  EXPORT_SYMBOL(flex_array_alloc);
> > > >
> > > > +static int flex_array_last_element_index(struct flex_array *fa)
> > > > +{
> > > > +       struct flex_array_part *part;
> > > > +       int part_nr;
> > > > +       int i,j;
> > > > +
> > > > +       if (elements_fit_in_base(fa)) {
> > > > +               part = (struct flex_array_part *)&fa->parts[0];
> > > > +               for (i = fa->elems_per_part; i >= 0; i--)
> > > > +                       if (part->elements[i] != FLEX_ARRAY_FREE)
> > > > +                               return i;
> > > > +       }
> > > > +
> > > > +       i = fa->total_nr_elements;
> > > > +       for (part_nr = 0; part_nr < FLEX_ARRAY_NR_BASE_PTRS; part_nr++) {
> > > > +               part = fa->parts[part_nr];
> > > > +               if (!part) {
> > > > +                       i -= fa->elems_per_part;
> > > > +                       continue;
> > > > +               }
> > > > +               for (j = fa->elems_per_part; j >= 0; j--, i--)
> > > > +                       if (part->elements[j] != FLEX_ARRAY_FREE)
> > > > +                               goto out;
> > > > +       }
> > > > +out:
> > > > +       return i;
> > > > +}
> > > > +
> > > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags)
> > > > +{
> > > > +       if (total >= fa->total_nr_elements) {
> > > > +               /* Grow case */
> > > > +               if (total > max_size)
> > > > +                       return ERR_PTR(-ETOOBIG);
> > > > +               fa->total_nr_elements = total;
> > > > +       } else {
> > > > +               /* Shrink case */
> > > > +               /* Drop any pages we don't need*/
> > > > +               flex_array_shrink(fa);
> > > > +               if (flex_array_last_element_index(fa) >= total)
> > > > +                       return ERR_PTR(-ESIZE);
> > > > +               fa->total_nr_elements = total;
> > > > +       }
> > > > +       return fa;
> > > > +}
> > > > +EXPORT_SYMBOL(flex_array_resize);
> > > > +
> > > >  static int fa_element_to_part_nr(struct flex_array *fa,
> > > >                                         unsigned int element_nr)
> > > >  {
> > > I have a question about how it checks one part is free in
> > > flex_array_shrink():
> > >   part_is_free() is doing it by checking:
> > >   if (part->elements[all] == FLEX_ARRAY_FREE) ...
> > >
> > > What if the data in the array that users put is FLEX_ARRAY_FREE,
> > > it will be treated as free, but in fact it's still in use?
> > >
> > I noticed that too, I think they're taking an unsafe shortcut honestly, assuming
> > that the FLEX_ARRAY_FREE poison value in the first byte is indicative of the
> > entire part being free.  For this use case its not likely a problem, because the
> > first element of the stream is a pointer, and so its unlikely to start with the
> > FLEX_ARRAY_FREE value, but it could suffer from aliasing in other cases.  In the
> > current design they should really be doing a memory comparison of the entire
> > object length to a buffer filled with the poison value to determine free status.
> Here's still another problem, I'm not sure if you have noticed that:
> When (elements_fit_in_base(fa)) or data_size <= FLEX_ARRAY_BASE_BYTES_LEFT
I don't see this as an or.  The only place I see a comparison being made is in
elements_fit_in_base:

static inline int elements_fit_in_base(struct flex_array *fa)
{
        int data_size = fa->element_size * fa->total_nr_elements;
        if (data_size <= FLEX_ARRAY_BASE_BYTES_LEFT)
                return 1;
        return 0;
}

I understand your point in that in the growth case, its possible that we might
cross the border in which elements which did fit in the base now no longer do.
I would think that the proper approach here is to:

1) Split the parts array out to its own page independent of the flex_array
structure

2) Adjust FLEX_ARRAY_BASE_BYTES_LEFT to not include the offsetof adjustment
(i.e. just be defined as FLEX_ARRAY_BASE_SIZE

If we were to do that, then in the growth case, if we pass the fit_in_base
threshold, we can just reallocate the parts page, and demote the old parts page
to be pointed to by parts[0].  This would still maintain the pointer values for
the elements and adjust the lookup process accordingly

likewise in the shrink case, we can then, if we now fit into the base, we can
promote parts[0] to be the parts array.

Neil

>   part_0 = (struct flex_array_part *)&fa->parts[0]
> otherwise,
>   part_0 = fa->parts[0].
> 
> That means when the new out streams' size is greater than
> FLEX_ARRAY_BASE_BYTES_LEFT, resize is not that easy to just set
> fa->total_nr_elements. We have to do something like what flex_array_alloc
> does, and the old elements memory will have to be lost.
> With that, this issue won't be fixed.
> 
> After checking the flex_array code, I think flex_array is not yet working
> for dynamical resize, which is not its goal either.
> 
> >
> > I would suggest not worrying about it for the purposes of this patch, but I
> > think I'll write a fix for the flex_array code that adds a byte to the head of
> > whatever object is bing stored to be reserved for free / in-use indicators.
> > Neil
> >
> 

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Neil Horman]

On Fri, Nov 30, 2018 at 10:48:10PM +0900, Xin Long wrote:
> On Fri, Nov 30, 2018 at 9:21 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Fri, Nov 30, 2018 at 03:22:39PM +0900, Xin Long wrote:
> > > On Thu, Nov 29, 2018 at 11:39 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > >
> > > > On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > > > > Now when using stream reconfig to add out streams, stream->out
> > > > > will get re-allocated, and all old streams' information will
> > > > > be copied to the new ones and the old ones will be freed.
> > > > >
> > > > > So without stream->out_curr updated, next time when trying to
> > > > > send from stream->out_curr stream, a panic would be caused.
> > > > >
> > > > > This patch is to check and update stream->out_curr when
> > > > > allocating stream_out.
> > > > >
> > > > > v1->v2:
> > > > >   - define fa_index() to get elem index from stream->out_curr.
> > > > >
> > > > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > > > > Reported-by: Ying Xu <yinxu@redhat.com>
> > > > > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > ---
> > > > >  net/sctp/stream.c | 20 ++++++++++++++++++++
> > > > >  1 file changed, 20 insertions(+)
> > > > >
> > > > > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > > > > index 3892e76..30e7809 100644
> > > > > --- a/net/sctp/stream.c
> > > > > +++ b/net/sctp/stream.c
> > > > > @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
> > > > >       }
> > > > >  }
> > > > >
> > > > > +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> > > > > +{
> > > > > +     size_t index = 0;
> > > > > +
> > > > > +     while (count--) {
> > > > > +             if (elem = flex_array_get(fa, index))
> > > > > +                     break;
> > > > > +             index++;
> > > > > +     }
> > > > > +
> > > > > +     return index;
> > > > > +}
> > > > > +
> > > > >  /* Migrates chunks from stream queues to new stream queues if needed,
> > > > >   * but not across associations. Also, removes those chunks to streams
> > > > >   * higher than the new max.
> > > > > @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> > > > >
> > > > >       if (stream->out) {
> > > > >               fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> > > > > +             if (stream->out_curr) {
> > > > > +                     size_t index = fa_index(stream->out, stream->out_curr,
> > > > > +                                             stream->outcnt);
> > > > > +
> > > > > +                     BUG_ON(index = stream->outcnt);
> > > > > +                     stream->out_curr = flex_array_get(out, index);
> > > > > +             }
> > > > >               fa_free(stream->out);
> > > > >       }
> > > > >
> > > > > --
> > > > > 2.1.0
> > > > >
> > > > >
> > > >
> > > > This is the sort of thing I'm talking about. Its a little more code, but if you
> > > > augment the flex_array api like this, you can preform a resize operation on your
> > > > existing flex array, and you can avoid all the copying, and need to update
> > > > pointers maintained outside the array.  Note this code isn't tested at all, but
> > > > its close to what I think should work.
> > > >
> > > >
> > > > diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
> > > > index b94fa61b51fb..7fa1f27a91b5 100644
> > > > --- a/include/linux/flex_array.h
> > > > +++ b/include/linux/flex_array.h
> > > > @@ -73,6 +73,8 @@ struct flex_array {
> > > >  struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > >                 gfp_t flags);
> > > >
> > > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags);
> > > > +
> > > >  /**
> > > >   * flex_array_prealloc() - Ensures that memory for the elements indexed in the
> > > >   * range defined by start and nr_elements has been allocated.
> > > > diff --git a/lib/flex_array.c b/lib/flex_array.c
> > > > index 2eed22fa507c..f8d54af3891b 100644
> > > > --- a/lib/flex_array.c
> > > > +++ b/lib/flex_array.c
> > > > @@ -109,6 +109,7 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > >         ret->total_nr_elements = total;
> > > >         ret->elems_per_part = elems_per_part;
> > > >         ret->reciprocal_elems = reciprocal_elems;
> > > > +       ret->elements_used = 0;
> > > >         if (elements_fit_in_base(ret) && !(flags & __GFP_ZERO))
> > > >                 memset(&ret->parts[0], FLEX_ARRAY_FREE,
> > > >                                                 FLEX_ARRAY_BASE_BYTES_LEFT);
> > > > @@ -116,6 +117,53 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > >  }
> > > >  EXPORT_SYMBOL(flex_array_alloc);
> > > >
> > > > +static int flex_array_last_element_index(struct flex_array *fa)
> > > > +{
> > > > +       struct flex_array_part *part;
> > > > +       int part_nr;
> > > > +       int i,j;
> > > > +
> > > > +       if (elements_fit_in_base(fa)) {
> > > > +               part = (struct flex_array_part *)&fa->parts[0];
> > > > +               for (i = fa->elems_per_part; i >= 0; i--)
> > > > +                       if (part->elements[i] != FLEX_ARRAY_FREE)
> > > > +                               return i;
> > > > +       }
> > > > +
> > > > +       i = fa->total_nr_elements;
> > > > +       for (part_nr = 0; part_nr < FLEX_ARRAY_NR_BASE_PTRS; part_nr++) {
> > > > +               part = fa->parts[part_nr];
> > > > +               if (!part) {
> > > > +                       i -= fa->elems_per_part;
> > > > +                       continue;
> > > > +               }
> > > > +               for (j = fa->elems_per_part; j >= 0; j--, i--)
> > > > +                       if (part->elements[j] != FLEX_ARRAY_FREE)
> > > > +                               goto out;
> > > > +       }
> > > > +out:
> > > > +       return i;
> > > > +}
> > > > +
> > > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags)
> > > > +{
> > > > +       if (total >= fa->total_nr_elements) {
> > > > +               /* Grow case */
> > > > +               if (total > max_size)
> > > > +                       return ERR_PTR(-ETOOBIG);
> > > > +               fa->total_nr_elements = total;
> > > > +       } else {
> > > > +               /* Shrink case */
> > > > +               /* Drop any pages we don't need*/
> > > > +               flex_array_shrink(fa);
> > > > +               if (flex_array_last_element_index(fa) >= total)
> > > > +                       return ERR_PTR(-ESIZE);
> > > > +               fa->total_nr_elements = total;
> > > > +       }
> > > > +       return fa;
> > > > +}
> > > > +EXPORT_SYMBOL(flex_array_resize);
> > > > +
> > > >  static int fa_element_to_part_nr(struct flex_array *fa,
> > > >                                         unsigned int element_nr)
> > > >  {
> > > I have a question about how it checks one part is free in
> > > flex_array_shrink():
> > >   part_is_free() is doing it by checking:
> > >   if (part->elements[all] = FLEX_ARRAY_FREE) ...
> > >
> > > What if the data in the array that users put is FLEX_ARRAY_FREE,
> > > it will be treated as free, but in fact it's still in use?
> > >
> > I noticed that too, I think they're taking an unsafe shortcut honestly, assuming
> > that the FLEX_ARRAY_FREE poison value in the first byte is indicative of the
> > entire part being free.  For this use case its not likely a problem, because the
> > first element of the stream is a pointer, and so its unlikely to start with the
> > FLEX_ARRAY_FREE value, but it could suffer from aliasing in other cases.  In the
> > current design they should really be doing a memory comparison of the entire
> > object length to a buffer filled with the poison value to determine free status.
> Here's still another problem, I'm not sure if you have noticed that:
> When (elements_fit_in_base(fa)) or data_size <= FLEX_ARRAY_BASE_BYTES_LEFT
I don't see this as an or.  The only place I see a comparison being made is in
elements_fit_in_base:

static inline int elements_fit_in_base(struct flex_array *fa)
{
        int data_size = fa->element_size * fa->total_nr_elements;
        if (data_size <= FLEX_ARRAY_BASE_BYTES_LEFT)
                return 1;
        return 0;
}

I understand your point in that in the growth case, its possible that we might
cross the border in which elements which did fit in the base now no longer do.
I would think that the proper approach here is to:

1) Split the parts array out to its own page independent of the flex_array
structure

2) Adjust FLEX_ARRAY_BASE_BYTES_LEFT to not include the offsetof adjustment
(i.e. just be defined as FLEX_ARRAY_BASE_SIZE

If we were to do that, then in the growth case, if we pass the fit_in_base
threshold, we can just reallocate the parts page, and demote the old parts page
to be pointed to by parts[0].  This would still maintain the pointer values for
the elements and adjust the lookup process accordingly

likewise in the shrink case, we can then, if we now fit into the base, we can
promote parts[0] to be the parts array.

Neil

>   part_0 = (struct flex_array_part *)&fa->parts[0]
> otherwise,
>   part_0 = fa->parts[0].
> 
> That means when the new out streams' size is greater than
> FLEX_ARRAY_BASE_BYTES_LEFT, resize is not that easy to just set
> fa->total_nr_elements. We have to do something like what flex_array_alloc
> does, and the old elements memory will have to be lost.
> With that, this issue won't be fixed.
> 
> After checking the flex_array code, I think flex_array is not yet working
> for dynamical resize, which is not its goal either.
> 
> >
> > I would suggest not worrying about it for the purposes of this patch, but I
> > think I'll write a fix for the flex_array code that adds a byte to the head of
> > whatever object is bing stored to be reserved for free / in-use indicators.
> > Neil
> >
> 

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Xin Long]

On Sat, Dec 1, 2018 at 12:23 AM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Fri, Nov 30, 2018 at 10:48:10PM +0900, Xin Long wrote:
> > On Fri, Nov 30, 2018 at 9:21 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > On Fri, Nov 30, 2018 at 03:22:39PM +0900, Xin Long wrote:
> > > > On Thu, Nov 29, 2018 at 11:39 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > >
> > > > > On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > > > > > Now when using stream reconfig to add out streams, stream->out
> > > > > > will get re-allocated, and all old streams' information will
> > > > > > be copied to the new ones and the old ones will be freed.
> > > > > >
> > > > > > So without stream->out_curr updated, next time when trying to
> > > > > > send from stream->out_curr stream, a panic would be caused.
> > > > > >
> > > > > > This patch is to check and update stream->out_curr when
> > > > > > allocating stream_out.
> > > > > >
> > > > > > v1->v2:
> > > > > >   - define fa_index() to get elem index from stream->out_curr.
> > > > > >
> > > > > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > > > > > Reported-by: Ying Xu <yinxu@redhat.com>
> > > > > > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > > ---
> > > > > >  net/sctp/stream.c | 20 ++++++++++++++++++++
> > > > > >  1 file changed, 20 insertions(+)
> > > > > >
> > > > > > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > > > > > index 3892e76..30e7809 100644
> > > > > > --- a/net/sctp/stream.c
> > > > > > +++ b/net/sctp/stream.c
> > > > > > @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
> > > > > >       }
> > > > > >  }
> > > > > >
> > > > > > +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> > > > > > +{
> > > > > > +     size_t index = 0;
> > > > > > +
> > > > > > +     while (count--) {
> > > > > > +             if (elem == flex_array_get(fa, index))
> > > > > > +                     break;
> > > > > > +             index++;
> > > > > > +     }
> > > > > > +
> > > > > > +     return index;
> > > > > > +}
> > > > > > +
> > > > > >  /* Migrates chunks from stream queues to new stream queues if needed,
> > > > > >   * but not across associations. Also, removes those chunks to streams
> > > > > >   * higher than the new max.
> > > > > > @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> > > > > >
> > > > > >       if (stream->out) {
> > > > > >               fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> > > > > > +             if (stream->out_curr) {
> > > > > > +                     size_t index = fa_index(stream->out, stream->out_curr,
> > > > > > +                                             stream->outcnt);
> > > > > > +
> > > > > > +                     BUG_ON(index == stream->outcnt);
> > > > > > +                     stream->out_curr = flex_array_get(out, index);
> > > > > > +             }
> > > > > >               fa_free(stream->out);
> > > > > >       }
> > > > > >
> > > > > > --
> > > > > > 2.1.0
> > > > > >
> > > > > >
> > > > >
> > > > > This is the sort of thing I'm talking about. Its a little more code, but if you
> > > > > augment the flex_array api like this, you can preform a resize operation on your
> > > > > existing flex array, and you can avoid all the copying, and need to update
> > > > > pointers maintained outside the array.  Note this code isn't tested at all, but
> > > > > its close to what I think should work.
> > > > >
> > > > >
> > > > > diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
> > > > > index b94fa61b51fb..7fa1f27a91b5 100644
> > > > > --- a/include/linux/flex_array.h
> > > > > +++ b/include/linux/flex_array.h
> > > > > @@ -73,6 +73,8 @@ struct flex_array {
> > > > >  struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > >                 gfp_t flags);
> > > > >
> > > > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags);
> > > > > +
> > > > >  /**
> > > > >   * flex_array_prealloc() - Ensures that memory for the elements indexed in the
> > > > >   * range defined by start and nr_elements has been allocated.
> > > > > diff --git a/lib/flex_array.c b/lib/flex_array.c
> > > > > index 2eed22fa507c..f8d54af3891b 100644
> > > > > --- a/lib/flex_array.c
> > > > > +++ b/lib/flex_array.c
> > > > > @@ -109,6 +109,7 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > >         ret->total_nr_elements = total;
> > > > >         ret->elems_per_part = elems_per_part;
> > > > >         ret->reciprocal_elems = reciprocal_elems;
> > > > > +       ret->elements_used = 0;
> > > > >         if (elements_fit_in_base(ret) && !(flags & __GFP_ZERO))
> > > > >                 memset(&ret->parts[0], FLEX_ARRAY_FREE,
> > > > >                                                 FLEX_ARRAY_BASE_BYTES_LEFT);
> > > > > @@ -116,6 +117,53 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > >  }
> > > > >  EXPORT_SYMBOL(flex_array_alloc);
> > > > >
> > > > > +static int flex_array_last_element_index(struct flex_array *fa)
> > > > > +{
> > > > > +       struct flex_array_part *part;
> > > > > +       int part_nr;
> > > > > +       int i,j;
> > > > > +
> > > > > +       if (elements_fit_in_base(fa)) {
> > > > > +               part = (struct flex_array_part *)&fa->parts[0];
> > > > > +               for (i = fa->elems_per_part; i >= 0; i--)
> > > > > +                       if (part->elements[i] != FLEX_ARRAY_FREE)
> > > > > +                               return i;
> > > > > +       }
> > > > > +
> > > > > +       i = fa->total_nr_elements;
> > > > > +       for (part_nr = 0; part_nr < FLEX_ARRAY_NR_BASE_PTRS; part_nr++) {
> > > > > +               part = fa->parts[part_nr];
> > > > > +               if (!part) {
> > > > > +                       i -= fa->elems_per_part;
> > > > > +                       continue;
> > > > > +               }
> > > > > +               for (j = fa->elems_per_part; j >= 0; j--, i--)
> > > > > +                       if (part->elements[j] != FLEX_ARRAY_FREE)
> > > > > +                               goto out;
> > > > > +       }
> > > > > +out:
> > > > > +       return i;
> > > > > +}
> > > > > +
> > > > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags)
> > > > > +{
> > > > > +       if (total >= fa->total_nr_elements) {
> > > > > +               /* Grow case */
> > > > > +               if (total > max_size)
> > > > > +                       return ERR_PTR(-ETOOBIG);
> > > > > +               fa->total_nr_elements = total;
> > > > > +       } else {
> > > > > +               /* Shrink case */
> > > > > +               /* Drop any pages we don't need*/
> > > > > +               flex_array_shrink(fa);
> > > > > +               if (flex_array_last_element_index(fa) >= total)
> > > > > +                       return ERR_PTR(-ESIZE);
> > > > > +               fa->total_nr_elements = total;
> > > > > +       }
> > > > > +       return fa;
> > > > > +}
> > > > > +EXPORT_SYMBOL(flex_array_resize);
> > > > > +
> > > > >  static int fa_element_to_part_nr(struct flex_array *fa,
> > > > >                                         unsigned int element_nr)
> > > > >  {
> > > > I have a question about how it checks one part is free in
> > > > flex_array_shrink():
> > > >   part_is_free() is doing it by checking:
> > > >   if (part->elements[all] == FLEX_ARRAY_FREE) ...
> > > >
> > > > What if the data in the array that users put is FLEX_ARRAY_FREE,
> > > > it will be treated as free, but in fact it's still in use?
> > > >
> > > I noticed that too, I think they're taking an unsafe shortcut honestly, assuming
> > > that the FLEX_ARRAY_FREE poison value in the first byte is indicative of the
> > > entire part being free.  For this use case its not likely a problem, because the
> > > first element of the stream is a pointer, and so its unlikely to start with the
> > > FLEX_ARRAY_FREE value, but it could suffer from aliasing in other cases.  In the
> > > current design they should really be doing a memory comparison of the entire
> > > object length to a buffer filled with the poison value to determine free status.
> > Here's still another problem, I'm not sure if you have noticed that:
> > When (elements_fit_in_base(fa)) or data_size <= FLEX_ARRAY_BASE_BYTES_LEFT
> I don't see this as an or.  The only place I see a comparison being made is in
> elements_fit_in_base:
>
> static inline int elements_fit_in_base(struct flex_array *fa)
> {
>         int data_size = fa->element_size * fa->total_nr_elements;
>         if (data_size <= FLEX_ARRAY_BASE_BYTES_LEFT)
>                 return 1;
>         return 0;
> }
>
> I understand your point in that in the growth case, its possible that we might
> cross the border in which elements which did fit in the base now no longer do.
> I would think that the proper approach here is to:
>
> 1) Split the parts array out to its own page independent of the flex_array
> structure
>
> 2) Adjust FLEX_ARRAY_BASE_BYTES_LEFT to not include the offsetof adjustment
> (i.e. just be defined as FLEX_ARRAY_BASE_SIZE
>
> If we were to do that, then in the growth case, if we pass the fit_in_base
> threshold, we can just reallocate the parts page, and demote the old parts page
> to be pointed to by parts[0].  This would still maintain the pointer values for
> the elements and adjust the lookup process accordingly
>
> likewise in the shrink case, we can then, if we now fit into the base, we can
> promote parts[0] to be the parts array.
got a draft patch, seems to work well so far.

https://paste.fedoraproject.org/paste/n1GSNLVutxOd2HNvTOzyPQ#

I'm thinking if we should do the shrink in sctp like in the patch,
or put it into flex_array_resize(),  considering FLEX_ARRAY_FREE
may not work for other cases.

>
> Neil
>
> >   part_0 = (struct flex_array_part *)&fa->parts[0]
> > otherwise,
> >   part_0 = fa->parts[0].
> >
> > That means when the new out streams' size is greater than
> > FLEX_ARRAY_BASE_BYTES_LEFT, resize is not that easy to just set
> > fa->total_nr_elements. We have to do something like what flex_array_alloc
> > does, and the old elements memory will have to be lost.
> > With that, this issue won't be fixed.
> >
> > After checking the flex_array code, I think flex_array is not yet working
> > for dynamical resize, which is not its goal either.
> >
> > >
> > > I would suggest not worrying about it for the purposes of this patch, but I
> > > think I'll write a fix for the flex_array code that adds a byte to the head of
> > > whatever object is bing stored to be reserved for free / in-use indicators.
> > > Neil
> > >
> >

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Xin Long]

On Sat, Dec 1, 2018 at 12:23 AM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Fri, Nov 30, 2018 at 10:48:10PM +0900, Xin Long wrote:
> > On Fri, Nov 30, 2018 at 9:21 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > On Fri, Nov 30, 2018 at 03:22:39PM +0900, Xin Long wrote:
> > > > On Thu, Nov 29, 2018 at 11:39 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > >
> > > > > On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > > > > > Now when using stream reconfig to add out streams, stream->out
> > > > > > will get re-allocated, and all old streams' information will
> > > > > > be copied to the new ones and the old ones will be freed.
> > > > > >
> > > > > > So without stream->out_curr updated, next time when trying to
> > > > > > send from stream->out_curr stream, a panic would be caused.
> > > > > >
> > > > > > This patch is to check and update stream->out_curr when
> > > > > > allocating stream_out.
> > > > > >
> > > > > > v1->v2:
> > > > > >   - define fa_index() to get elem index from stream->out_curr.
> > > > > >
> > > > > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > > > > > Reported-by: Ying Xu <yinxu@redhat.com>
> > > > > > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > > ---
> > > > > >  net/sctp/stream.c | 20 ++++++++++++++++++++
> > > > > >  1 file changed, 20 insertions(+)
> > > > > >
> > > > > > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > > > > > index 3892e76..30e7809 100644
> > > > > > --- a/net/sctp/stream.c
> > > > > > +++ b/net/sctp/stream.c
> > > > > > @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
> > > > > >       }
> > > > > >  }
> > > > > >
> > > > > > +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> > > > > > +{
> > > > > > +     size_t index = 0;
> > > > > > +
> > > > > > +     while (count--) {
> > > > > > +             if (elem = flex_array_get(fa, index))
> > > > > > +                     break;
> > > > > > +             index++;
> > > > > > +     }
> > > > > > +
> > > > > > +     return index;
> > > > > > +}
> > > > > > +
> > > > > >  /* Migrates chunks from stream queues to new stream queues if needed,
> > > > > >   * but not across associations. Also, removes those chunks to streams
> > > > > >   * higher than the new max.
> > > > > > @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> > > > > >
> > > > > >       if (stream->out) {
> > > > > >               fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> > > > > > +             if (stream->out_curr) {
> > > > > > +                     size_t index = fa_index(stream->out, stream->out_curr,
> > > > > > +                                             stream->outcnt);
> > > > > > +
> > > > > > +                     BUG_ON(index = stream->outcnt);
> > > > > > +                     stream->out_curr = flex_array_get(out, index);
> > > > > > +             }
> > > > > >               fa_free(stream->out);
> > > > > >       }
> > > > > >
> > > > > > --
> > > > > > 2.1.0
> > > > > >
> > > > > >
> > > > >
> > > > > This is the sort of thing I'm talking about. Its a little more code, but if you
> > > > > augment the flex_array api like this, you can preform a resize operation on your
> > > > > existing flex array, and you can avoid all the copying, and need to update
> > > > > pointers maintained outside the array.  Note this code isn't tested at all, but
> > > > > its close to what I think should work.
> > > > >
> > > > >
> > > > > diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
> > > > > index b94fa61b51fb..7fa1f27a91b5 100644
> > > > > --- a/include/linux/flex_array.h
> > > > > +++ b/include/linux/flex_array.h
> > > > > @@ -73,6 +73,8 @@ struct flex_array {
> > > > >  struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > >                 gfp_t flags);
> > > > >
> > > > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags);
> > > > > +
> > > > >  /**
> > > > >   * flex_array_prealloc() - Ensures that memory for the elements indexed in the
> > > > >   * range defined by start and nr_elements has been allocated.
> > > > > diff --git a/lib/flex_array.c b/lib/flex_array.c
> > > > > index 2eed22fa507c..f8d54af3891b 100644
> > > > > --- a/lib/flex_array.c
> > > > > +++ b/lib/flex_array.c
> > > > > @@ -109,6 +109,7 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > >         ret->total_nr_elements = total;
> > > > >         ret->elems_per_part = elems_per_part;
> > > > >         ret->reciprocal_elems = reciprocal_elems;
> > > > > +       ret->elements_used = 0;
> > > > >         if (elements_fit_in_base(ret) && !(flags & __GFP_ZERO))
> > > > >                 memset(&ret->parts[0], FLEX_ARRAY_FREE,
> > > > >                                                 FLEX_ARRAY_BASE_BYTES_LEFT);
> > > > > @@ -116,6 +117,53 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > >  }
> > > > >  EXPORT_SYMBOL(flex_array_alloc);
> > > > >
> > > > > +static int flex_array_last_element_index(struct flex_array *fa)
> > > > > +{
> > > > > +       struct flex_array_part *part;
> > > > > +       int part_nr;
> > > > > +       int i,j;
> > > > > +
> > > > > +       if (elements_fit_in_base(fa)) {
> > > > > +               part = (struct flex_array_part *)&fa->parts[0];
> > > > > +               for (i = fa->elems_per_part; i >= 0; i--)
> > > > > +                       if (part->elements[i] != FLEX_ARRAY_FREE)
> > > > > +                               return i;
> > > > > +       }
> > > > > +
> > > > > +       i = fa->total_nr_elements;
> > > > > +       for (part_nr = 0; part_nr < FLEX_ARRAY_NR_BASE_PTRS; part_nr++) {
> > > > > +               part = fa->parts[part_nr];
> > > > > +               if (!part) {
> > > > > +                       i -= fa->elems_per_part;
> > > > > +                       continue;
> > > > > +               }
> > > > > +               for (j = fa->elems_per_part; j >= 0; j--, i--)
> > > > > +                       if (part->elements[j] != FLEX_ARRAY_FREE)
> > > > > +                               goto out;
> > > > > +       }
> > > > > +out:
> > > > > +       return i;
> > > > > +}
> > > > > +
> > > > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags)
> > > > > +{
> > > > > +       if (total >= fa->total_nr_elements) {
> > > > > +               /* Grow case */
> > > > > +               if (total > max_size)
> > > > > +                       return ERR_PTR(-ETOOBIG);
> > > > > +               fa->total_nr_elements = total;
> > > > > +       } else {
> > > > > +               /* Shrink case */
> > > > > +               /* Drop any pages we don't need*/
> > > > > +               flex_array_shrink(fa);
> > > > > +               if (flex_array_last_element_index(fa) >= total)
> > > > > +                       return ERR_PTR(-ESIZE);
> > > > > +               fa->total_nr_elements = total;
> > > > > +       }
> > > > > +       return fa;
> > > > > +}
> > > > > +EXPORT_SYMBOL(flex_array_resize);
> > > > > +
> > > > >  static int fa_element_to_part_nr(struct flex_array *fa,
> > > > >                                         unsigned int element_nr)
> > > > >  {
> > > > I have a question about how it checks one part is free in
> > > > flex_array_shrink():
> > > >   part_is_free() is doing it by checking:
> > > >   if (part->elements[all] = FLEX_ARRAY_FREE) ...
> > > >
> > > > What if the data in the array that users put is FLEX_ARRAY_FREE,
> > > > it will be treated as free, but in fact it's still in use?
> > > >
> > > I noticed that too, I think they're taking an unsafe shortcut honestly, assuming
> > > that the FLEX_ARRAY_FREE poison value in the first byte is indicative of the
> > > entire part being free.  For this use case its not likely a problem, because the
> > > first element of the stream is a pointer, and so its unlikely to start with the
> > > FLEX_ARRAY_FREE value, but it could suffer from aliasing in other cases.  In the
> > > current design they should really be doing a memory comparison of the entire
> > > object length to a buffer filled with the poison value to determine free status.
> > Here's still another problem, I'm not sure if you have noticed that:
> > When (elements_fit_in_base(fa)) or data_size <= FLEX_ARRAY_BASE_BYTES_LEFT
> I don't see this as an or.  The only place I see a comparison being made is in
> elements_fit_in_base:
>
> static inline int elements_fit_in_base(struct flex_array *fa)
> {
>         int data_size = fa->element_size * fa->total_nr_elements;
>         if (data_size <= FLEX_ARRAY_BASE_BYTES_LEFT)
>                 return 1;
>         return 0;
> }
>
> I understand your point in that in the growth case, its possible that we might
> cross the border in which elements which did fit in the base now no longer do.
> I would think that the proper approach here is to:
>
> 1) Split the parts array out to its own page independent of the flex_array
> structure
>
> 2) Adjust FLEX_ARRAY_BASE_BYTES_LEFT to not include the offsetof adjustment
> (i.e. just be defined as FLEX_ARRAY_BASE_SIZE
>
> If we were to do that, then in the growth case, if we pass the fit_in_base
> threshold, we can just reallocate the parts page, and demote the old parts page
> to be pointed to by parts[0].  This would still maintain the pointer values for
> the elements and adjust the lookup process accordingly
>
> likewise in the shrink case, we can then, if we now fit into the base, we can
> promote parts[0] to be the parts array.
got a draft patch, seems to work well so far.

https://paste.fedoraproject.org/paste/n1GSNLVutxOd2HNvTOzyPQ#

I'm thinking if we should do the shrink in sctp like in the patch,
or put it into flex_array_resize(),  considering FLEX_ARRAY_FREE
may not work for other cases.

>
> Neil
>
> >   part_0 = (struct flex_array_part *)&fa->parts[0]
> > otherwise,
> >   part_0 = fa->parts[0].
> >
> > That means when the new out streams' size is greater than
> > FLEX_ARRAY_BASE_BYTES_LEFT, resize is not that easy to just set
> > fa->total_nr_elements. We have to do something like what flex_array_alloc
> > does, and the old elements memory will have to be lost.
> > With that, this issue won't be fixed.
> >
> > After checking the flex_array code, I think flex_array is not yet working
> > for dynamical resize, which is not its goal either.
> >
> > >
> > > I would suggest not worrying about it for the purposes of this patch, but I
> > > think I'll write a fix for the flex_array code that adds a byte to the head of
> > > whatever object is bing stored to be reserved for free / in-use indicators.
> > > Neil
> > >
> >

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Neil Horman]

On Sat, Dec 01, 2018 at 03:53:26AM +0900, Xin Long wrote:
> On Sat, Dec 1, 2018 at 12:23 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Fri, Nov 30, 2018 at 10:48:10PM +0900, Xin Long wrote:
> > > On Fri, Nov 30, 2018 at 9:21 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > >
> > > > On Fri, Nov 30, 2018 at 03:22:39PM +0900, Xin Long wrote:
> > > > > On Thu, Nov 29, 2018 at 11:39 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > > >
> > > > > > On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > > > > > > Now when using stream reconfig to add out streams, stream->out
> > > > > > > will get re-allocated, and all old streams' information will
> > > > > > > be copied to the new ones and the old ones will be freed.
> > > > > > >
> > > > > > > So without stream->out_curr updated, next time when trying to
> > > > > > > send from stream->out_curr stream, a panic would be caused.
> > > > > > >
> > > > > > > This patch is to check and update stream->out_curr when
> > > > > > > allocating stream_out.
> > > > > > >
> > > > > > > v1->v2:
> > > > > > >   - define fa_index() to get elem index from stream->out_curr.
> > > > > > >
> > > > > > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > > > > > > Reported-by: Ying Xu <yinxu@redhat.com>
> > > > > > > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > > > ---
> > > > > > >  net/sctp/stream.c | 20 ++++++++++++++++++++
> > > > > > >  1 file changed, 20 insertions(+)
> > > > > > >
> > > > > > > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > > > > > > index 3892e76..30e7809 100644
> > > > > > > --- a/net/sctp/stream.c
> > > > > > > +++ b/net/sctp/stream.c
> > > > > > > @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
> > > > > > >       }
> > > > > > >  }
> > > > > > >
> > > > > > > +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> > > > > > > +{
> > > > > > > +     size_t index = 0;
> > > > > > > +
> > > > > > > +     while (count--) {
> > > > > > > +             if (elem == flex_array_get(fa, index))
> > > > > > > +                     break;
> > > > > > > +             index++;
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     return index;
> > > > > > > +}
> > > > > > > +
> > > > > > >  /* Migrates chunks from stream queues to new stream queues if needed,
> > > > > > >   * but not across associations. Also, removes those chunks to streams
> > > > > > >   * higher than the new max.
> > > > > > > @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> > > > > > >
> > > > > > >       if (stream->out) {
> > > > > > >               fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> > > > > > > +             if (stream->out_curr) {
> > > > > > > +                     size_t index = fa_index(stream->out, stream->out_curr,
> > > > > > > +                                             stream->outcnt);
> > > > > > > +
> > > > > > > +                     BUG_ON(index == stream->outcnt);
> > > > > > > +                     stream->out_curr = flex_array_get(out, index);
> > > > > > > +             }
> > > > > > >               fa_free(stream->out);
> > > > > > >       }
> > > > > > >
> > > > > > > --
> > > > > > > 2.1.0
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > This is the sort of thing I'm talking about. Its a little more code, but if you
> > > > > > augment the flex_array api like this, you can preform a resize operation on your
> > > > > > existing flex array, and you can avoid all the copying, and need to update
> > > > > > pointers maintained outside the array.  Note this code isn't tested at all, but
> > > > > > its close to what I think should work.
> > > > > >
> > > > > >
> > > > > > diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
> > > > > > index b94fa61b51fb..7fa1f27a91b5 100644
> > > > > > --- a/include/linux/flex_array.h
> > > > > > +++ b/include/linux/flex_array.h
> > > > > > @@ -73,6 +73,8 @@ struct flex_array {
> > > > > >  struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > > >                 gfp_t flags);
> > > > > >
> > > > > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags);
> > > > > > +
> > > > > >  /**
> > > > > >   * flex_array_prealloc() - Ensures that memory for the elements indexed in the
> > > > > >   * range defined by start and nr_elements has been allocated.
> > > > > > diff --git a/lib/flex_array.c b/lib/flex_array.c
> > > > > > index 2eed22fa507c..f8d54af3891b 100644
> > > > > > --- a/lib/flex_array.c
> > > > > > +++ b/lib/flex_array.c
> > > > > > @@ -109,6 +109,7 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > > >         ret->total_nr_elements = total;
> > > > > >         ret->elems_per_part = elems_per_part;
> > > > > >         ret->reciprocal_elems = reciprocal_elems;
> > > > > > +       ret->elements_used = 0;
> > > > > >         if (elements_fit_in_base(ret) && !(flags & __GFP_ZERO))
> > > > > >                 memset(&ret->parts[0], FLEX_ARRAY_FREE,
> > > > > >                                                 FLEX_ARRAY_BASE_BYTES_LEFT);
> > > > > > @@ -116,6 +117,53 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > > >  }
> > > > > >  EXPORT_SYMBOL(flex_array_alloc);
> > > > > >
> > > > > > +static int flex_array_last_element_index(struct flex_array *fa)
> > > > > > +{
> > > > > > +       struct flex_array_part *part;
> > > > > > +       int part_nr;
> > > > > > +       int i,j;
> > > > > > +
> > > > > > +       if (elements_fit_in_base(fa)) {
> > > > > > +               part = (struct flex_array_part *)&fa->parts[0];
> > > > > > +               for (i = fa->elems_per_part; i >= 0; i--)
> > > > > > +                       if (part->elements[i] != FLEX_ARRAY_FREE)
> > > > > > +                               return i;
> > > > > > +       }
> > > > > > +
> > > > > > +       i = fa->total_nr_elements;
> > > > > > +       for (part_nr = 0; part_nr < FLEX_ARRAY_NR_BASE_PTRS; part_nr++) {
> > > > > > +               part = fa->parts[part_nr];
> > > > > > +               if (!part) {
> > > > > > +                       i -= fa->elems_per_part;
> > > > > > +                       continue;
> > > > > > +               }
> > > > > > +               for (j = fa->elems_per_part; j >= 0; j--, i--)
> > > > > > +                       if (part->elements[j] != FLEX_ARRAY_FREE)
> > > > > > +                               goto out;
> > > > > > +       }
> > > > > > +out:
> > > > > > +       return i;
> > > > > > +}
> > > > > > +
> > > > > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags)
> > > > > > +{
> > > > > > +       if (total >= fa->total_nr_elements) {
> > > > > > +               /* Grow case */
> > > > > > +               if (total > max_size)
> > > > > > +                       return ERR_PTR(-ETOOBIG);
> > > > > > +               fa->total_nr_elements = total;
> > > > > > +       } else {
> > > > > > +               /* Shrink case */
> > > > > > +               /* Drop any pages we don't need*/
> > > > > > +               flex_array_shrink(fa);
> > > > > > +               if (flex_array_last_element_index(fa) >= total)
> > > > > > +                       return ERR_PTR(-ESIZE);
> > > > > > +               fa->total_nr_elements = total;
> > > > > > +       }
> > > > > > +       return fa;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(flex_array_resize);
> > > > > > +
> > > > > >  static int fa_element_to_part_nr(struct flex_array *fa,
> > > > > >                                         unsigned int element_nr)
> > > > > >  {
> > > > > I have a question about how it checks one part is free in
> > > > > flex_array_shrink():
> > > > >   part_is_free() is doing it by checking:
> > > > >   if (part->elements[all] == FLEX_ARRAY_FREE) ...
> > > > >
> > > > > What if the data in the array that users put is FLEX_ARRAY_FREE,
> > > > > it will be treated as free, but in fact it's still in use?
> > > > >
> > > > I noticed that too, I think they're taking an unsafe shortcut honestly, assuming
> > > > that the FLEX_ARRAY_FREE poison value in the first byte is indicative of the
> > > > entire part being free.  For this use case its not likely a problem, because the
> > > > first element of the stream is a pointer, and so its unlikely to start with the
> > > > FLEX_ARRAY_FREE value, but it could suffer from aliasing in other cases.  In the
> > > > current design they should really be doing a memory comparison of the entire
> > > > object length to a buffer filled with the poison value to determine free status.
> > > Here's still another problem, I'm not sure if you have noticed that:
> > > When (elements_fit_in_base(fa)) or data_size <= FLEX_ARRAY_BASE_BYTES_LEFT
> > I don't see this as an or.  The only place I see a comparison being made is in
> > elements_fit_in_base:
> >
> > static inline int elements_fit_in_base(struct flex_array *fa)
> > {
> >         int data_size = fa->element_size * fa->total_nr_elements;
> >         if (data_size <= FLEX_ARRAY_BASE_BYTES_LEFT)
> >                 return 1;
> >         return 0;
> > }
> >
> > I understand your point in that in the growth case, its possible that we might
> > cross the border in which elements which did fit in the base now no longer do.
> > I would think that the proper approach here is to:
> >
> > 1) Split the parts array out to its own page independent of the flex_array
> > structure
> >
> > 2) Adjust FLEX_ARRAY_BASE_BYTES_LEFT to not include the offsetof adjustment
> > (i.e. just be defined as FLEX_ARRAY_BASE_SIZE
> >
> > If we were to do that, then in the growth case, if we pass the fit_in_base
> > threshold, we can just reallocate the parts page, and demote the old parts page
> > to be pointed to by parts[0].  This would still maintain the pointer values for
> > the elements and adjust the lookup process accordingly
> >
> > likewise in the shrink case, we can then, if we now fit into the base, we can
> > promote parts[0] to be the parts array.
> got a draft patch, seems to work well so far.
> 
> https://paste.fedoraproject.org/paste/n1GSNLVutxOd2HNvTOzyPQ#
> 
> I'm thinking if we should do the shrink in sctp like in the patch,
> or put it into flex_array_resize(),  considering FLEX_ARRAY_FREE
> may not work for other cases.
> 
I appreciate you working on this.  I think the growth path looks good for both
cases (fit in base and not fit in base).  However, I think you need to do some
extra checking for the case where a resize shrinks the array.  That is to say,
you should not allow a flex array to be resized from A elements to B elements
(where B < A), if the origional array has element A-1 populated (where A-1 > B).
In that event you have to deny the resize request.

Neil

> >
> > Neil
> >
> > >   part_0 = (struct flex_array_part *)&fa->parts[0]
> > > otherwise,
> > >   part_0 = fa->parts[0].
> > >
> > > That means when the new out streams' size is greater than
> > > FLEX_ARRAY_BASE_BYTES_LEFT, resize is not that easy to just set
> > > fa->total_nr_elements. We have to do something like what flex_array_alloc
> > > does, and the old elements memory will have to be lost.
> > > With that, this issue won't be fixed.
> > >
> > > After checking the flex_array code, I think flex_array is not yet working
> > > for dynamical resize, which is not its goal either.
> > >
> > > >
> > > > I would suggest not worrying about it for the purposes of this patch, but I
> > > > think I'll write a fix for the flex_array code that adds a byte to the head of
> > > > whatever object is bing stored to be reserved for free / in-use indicators.
> > > > Neil
> > > >
> > >
> 

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Neil Horman]

On Sat, Dec 01, 2018 at 03:53:26AM +0900, Xin Long wrote:
> On Sat, Dec 1, 2018 at 12:23 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Fri, Nov 30, 2018 at 10:48:10PM +0900, Xin Long wrote:
> > > On Fri, Nov 30, 2018 at 9:21 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > >
> > > > On Fri, Nov 30, 2018 at 03:22:39PM +0900, Xin Long wrote:
> > > > > On Thu, Nov 29, 2018 at 11:39 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > > >
> > > > > > On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > > > > > > Now when using stream reconfig to add out streams, stream->out
> > > > > > > will get re-allocated, and all old streams' information will
> > > > > > > be copied to the new ones and the old ones will be freed.
> > > > > > >
> > > > > > > So without stream->out_curr updated, next time when trying to
> > > > > > > send from stream->out_curr stream, a panic would be caused.
> > > > > > >
> > > > > > > This patch is to check and update stream->out_curr when
> > > > > > > allocating stream_out.
> > > > > > >
> > > > > > > v1->v2:
> > > > > > >   - define fa_index() to get elem index from stream->out_curr.
> > > > > > >
> > > > > > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > > > > > > Reported-by: Ying Xu <yinxu@redhat.com>
> > > > > > > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > > > ---
> > > > > > >  net/sctp/stream.c | 20 ++++++++++++++++++++
> > > > > > >  1 file changed, 20 insertions(+)
> > > > > > >
> > > > > > > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > > > > > > index 3892e76..30e7809 100644
> > > > > > > --- a/net/sctp/stream.c
> > > > > > > +++ b/net/sctp/stream.c
> > > > > > > @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
> > > > > > >       }
> > > > > > >  }
> > > > > > >
> > > > > > > +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> > > > > > > +{
> > > > > > > +     size_t index = 0;
> > > > > > > +
> > > > > > > +     while (count--) {
> > > > > > > +             if (elem = flex_array_get(fa, index))
> > > > > > > +                     break;
> > > > > > > +             index++;
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     return index;
> > > > > > > +}
> > > > > > > +
> > > > > > >  /* Migrates chunks from stream queues to new stream queues if needed,
> > > > > > >   * but not across associations. Also, removes those chunks to streams
> > > > > > >   * higher than the new max.
> > > > > > > @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> > > > > > >
> > > > > > >       if (stream->out) {
> > > > > > >               fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> > > > > > > +             if (stream->out_curr) {
> > > > > > > +                     size_t index = fa_index(stream->out, stream->out_curr,
> > > > > > > +                                             stream->outcnt);
> > > > > > > +
> > > > > > > +                     BUG_ON(index = stream->outcnt);
> > > > > > > +                     stream->out_curr = flex_array_get(out, index);
> > > > > > > +             }
> > > > > > >               fa_free(stream->out);
> > > > > > >       }
> > > > > > >
> > > > > > > --
> > > > > > > 2.1.0
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > This is the sort of thing I'm talking about. Its a little more code, but if you
> > > > > > augment the flex_array api like this, you can preform a resize operation on your
> > > > > > existing flex array, and you can avoid all the copying, and need to update
> > > > > > pointers maintained outside the array.  Note this code isn't tested at all, but
> > > > > > its close to what I think should work.
> > > > > >
> > > > > >
> > > > > > diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
> > > > > > index b94fa61b51fb..7fa1f27a91b5 100644
> > > > > > --- a/include/linux/flex_array.h
> > > > > > +++ b/include/linux/flex_array.h
> > > > > > @@ -73,6 +73,8 @@ struct flex_array {
> > > > > >  struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > > >                 gfp_t flags);
> > > > > >
> > > > > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags);
> > > > > > +
> > > > > >  /**
> > > > > >   * flex_array_prealloc() - Ensures that memory for the elements indexed in the
> > > > > >   * range defined by start and nr_elements has been allocated.
> > > > > > diff --git a/lib/flex_array.c b/lib/flex_array.c
> > > > > > index 2eed22fa507c..f8d54af3891b 100644
> > > > > > --- a/lib/flex_array.c
> > > > > > +++ b/lib/flex_array.c
> > > > > > @@ -109,6 +109,7 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > > >         ret->total_nr_elements = total;
> > > > > >         ret->elems_per_part = elems_per_part;
> > > > > >         ret->reciprocal_elems = reciprocal_elems;
> > > > > > +       ret->elements_used = 0;
> > > > > >         if (elements_fit_in_base(ret) && !(flags & __GFP_ZERO))
> > > > > >                 memset(&ret->parts[0], FLEX_ARRAY_FREE,
> > > > > >                                                 FLEX_ARRAY_BASE_BYTES_LEFT);
> > > > > > @@ -116,6 +117,53 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > > >  }
> > > > > >  EXPORT_SYMBOL(flex_array_alloc);
> > > > > >
> > > > > > +static int flex_array_last_element_index(struct flex_array *fa)
> > > > > > +{
> > > > > > +       struct flex_array_part *part;
> > > > > > +       int part_nr;
> > > > > > +       int i,j;
> > > > > > +
> > > > > > +       if (elements_fit_in_base(fa)) {
> > > > > > +               part = (struct flex_array_part *)&fa->parts[0];
> > > > > > +               for (i = fa->elems_per_part; i >= 0; i--)
> > > > > > +                       if (part->elements[i] != FLEX_ARRAY_FREE)
> > > > > > +                               return i;
> > > > > > +       }
> > > > > > +
> > > > > > +       i = fa->total_nr_elements;
> > > > > > +       for (part_nr = 0; part_nr < FLEX_ARRAY_NR_BASE_PTRS; part_nr++) {
> > > > > > +               part = fa->parts[part_nr];
> > > > > > +               if (!part) {
> > > > > > +                       i -= fa->elems_per_part;
> > > > > > +                       continue;
> > > > > > +               }
> > > > > > +               for (j = fa->elems_per_part; j >= 0; j--, i--)
> > > > > > +                       if (part->elements[j] != FLEX_ARRAY_FREE)
> > > > > > +                               goto out;
> > > > > > +       }
> > > > > > +out:
> > > > > > +       return i;
> > > > > > +}
> > > > > > +
> > > > > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags)
> > > > > > +{
> > > > > > +       if (total >= fa->total_nr_elements) {
> > > > > > +               /* Grow case */
> > > > > > +               if (total > max_size)
> > > > > > +                       return ERR_PTR(-ETOOBIG);
> > > > > > +               fa->total_nr_elements = total;
> > > > > > +       } else {
> > > > > > +               /* Shrink case */
> > > > > > +               /* Drop any pages we don't need*/
> > > > > > +               flex_array_shrink(fa);
> > > > > > +               if (flex_array_last_element_index(fa) >= total)
> > > > > > +                       return ERR_PTR(-ESIZE);
> > > > > > +               fa->total_nr_elements = total;
> > > > > > +       }
> > > > > > +       return fa;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(flex_array_resize);
> > > > > > +
> > > > > >  static int fa_element_to_part_nr(struct flex_array *fa,
> > > > > >                                         unsigned int element_nr)
> > > > > >  {
> > > > > I have a question about how it checks one part is free in
> > > > > flex_array_shrink():
> > > > >   part_is_free() is doing it by checking:
> > > > >   if (part->elements[all] = FLEX_ARRAY_FREE) ...
> > > > >
> > > > > What if the data in the array that users put is FLEX_ARRAY_FREE,
> > > > > it will be treated as free, but in fact it's still in use?
> > > > >
> > > > I noticed that too, I think they're taking an unsafe shortcut honestly, assuming
> > > > that the FLEX_ARRAY_FREE poison value in the first byte is indicative of the
> > > > entire part being free.  For this use case its not likely a problem, because the
> > > > first element of the stream is a pointer, and so its unlikely to start with the
> > > > FLEX_ARRAY_FREE value, but it could suffer from aliasing in other cases.  In the
> > > > current design they should really be doing a memory comparison of the entire
> > > > object length to a buffer filled with the poison value to determine free status.
> > > Here's still another problem, I'm not sure if you have noticed that:
> > > When (elements_fit_in_base(fa)) or data_size <= FLEX_ARRAY_BASE_BYTES_LEFT
> > I don't see this as an or.  The only place I see a comparison being made is in
> > elements_fit_in_base:
> >
> > static inline int elements_fit_in_base(struct flex_array *fa)
> > {
> >         int data_size = fa->element_size * fa->total_nr_elements;
> >         if (data_size <= FLEX_ARRAY_BASE_BYTES_LEFT)
> >                 return 1;
> >         return 0;
> > }
> >
> > I understand your point in that in the growth case, its possible that we might
> > cross the border in which elements which did fit in the base now no longer do.
> > I would think that the proper approach here is to:
> >
> > 1) Split the parts array out to its own page independent of the flex_array
> > structure
> >
> > 2) Adjust FLEX_ARRAY_BASE_BYTES_LEFT to not include the offsetof adjustment
> > (i.e. just be defined as FLEX_ARRAY_BASE_SIZE
> >
> > If we were to do that, then in the growth case, if we pass the fit_in_base
> > threshold, we can just reallocate the parts page, and demote the old parts page
> > to be pointed to by parts[0].  This would still maintain the pointer values for
> > the elements and adjust the lookup process accordingly
> >
> > likewise in the shrink case, we can then, if we now fit into the base, we can
> > promote parts[0] to be the parts array.
> got a draft patch, seems to work well so far.
> 
> https://paste.fedoraproject.org/paste/n1GSNLVutxOd2HNvTOzyPQ#
> 
> I'm thinking if we should do the shrink in sctp like in the patch,
> or put it into flex_array_resize(),  considering FLEX_ARRAY_FREE
> may not work for other cases.
> 
I appreciate you working on this.  I think the growth path looks good for both
cases (fit in base and not fit in base).  However, I think you need to do some
extra checking for the case where a resize shrinks the array.  That is to say,
you should not allow a flex array to be resized from A elements to B elements
(where B < A), if the origional array has element A-1 populated (where A-1 > B).
In that event you have to deny the resize request.

Neil

> >
> > Neil
> >
> > >   part_0 = (struct flex_array_part *)&fa->parts[0]
> > > otherwise,
> > >   part_0 = fa->parts[0].
> > >
> > > That means when the new out streams' size is greater than
> > > FLEX_ARRAY_BASE_BYTES_LEFT, resize is not that easy to just set
> > > fa->total_nr_elements. We have to do something like what flex_array_alloc
> > > does, and the old elements memory will have to be lost.
> > > With that, this issue won't be fixed.
> > >
> > > After checking the flex_array code, I think flex_array is not yet working
> > > for dynamical resize, which is not its goal either.
> > >
> > > >
> > > > I would suggest not worrying about it for the purposes of this patch, but I
> > > > think I'll write a fix for the flex_array code that adds a byte to the head of
> > > > whatever object is bing stored to be reserved for free / in-use indicators.
> > > > Neil
> > > >
> > >
> 

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Neil Horman]

On Sat, Dec 01, 2018 at 03:53:26AM +0900, Xin Long wrote:
> On Sat, Dec 1, 2018 at 12:23 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Fri, Nov 30, 2018 at 10:48:10PM +0900, Xin Long wrote:
> > > On Fri, Nov 30, 2018 at 9:21 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > >
> > > > On Fri, Nov 30, 2018 at 03:22:39PM +0900, Xin Long wrote:
> > > > > On Thu, Nov 29, 2018 at 11:39 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > > >
> > > > > > On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > > > > > > Now when using stream reconfig to add out streams, stream->out
> > > > > > > will get re-allocated, and all old streams' information will
> > > > > > > be copied to the new ones and the old ones will be freed.
> > > > > > >
> > > > > > > So without stream->out_curr updated, next time when trying to
> > > > > > > send from stream->out_curr stream, a panic would be caused.
> > > > > > >
> > > > > > > This patch is to check and update stream->out_curr when
> > > > > > > allocating stream_out.
> > > > > > >
> > > > > > > v1->v2:
> > > > > > >   - define fa_index() to get elem index from stream->out_curr.
> > > > > > >
> > > > > > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > > > > > > Reported-by: Ying Xu <yinxu@redhat.com>
> > > > > > > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > > > ---
> > > > > > >  net/sctp/stream.c | 20 ++++++++++++++++++++
> > > > > > >  1 file changed, 20 insertions(+)
> > > > > > >
> > > > > > > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > > > > > > index 3892e76..30e7809 100644
> > > > > > > --- a/net/sctp/stream.c
> > > > > > > +++ b/net/sctp/stream.c
> > > > > > > @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
> > > > > > >       }
> > > > > > >  }
> > > > > > >
> > > > > > > +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> > > > > > > +{
> > > > > > > +     size_t index = 0;
> > > > > > > +
> > > > > > > +     while (count--) {
> > > > > > > +             if (elem == flex_array_get(fa, index))
> > > > > > > +                     break;
> > > > > > > +             index++;
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     return index;
> > > > > > > +}
> > > > > > > +
> > > > > > >  /* Migrates chunks from stream queues to new stream queues if needed,
> > > > > > >   * but not across associations. Also, removes those chunks to streams
> > > > > > >   * higher than the new max.
> > > > > > > @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> > > > > > >
> > > > > > >       if (stream->out) {
> > > > > > >               fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> > > > > > > +             if (stream->out_curr) {
> > > > > > > +                     size_t index = fa_index(stream->out, stream->out_curr,
> > > > > > > +                                             stream->outcnt);
> > > > > > > +
> > > > > > > +                     BUG_ON(index == stream->outcnt);
> > > > > > > +                     stream->out_curr = flex_array_get(out, index);
> > > > > > > +             }
> > > > > > >               fa_free(stream->out);
> > > > > > >       }
> > > > > > >
> > > > > > > --
> > > > > > > 2.1.0
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > This is the sort of thing I'm talking about. Its a little more code, but if you
> > > > > > augment the flex_array api like this, you can preform a resize operation on your
> > > > > > existing flex array, and you can avoid all the copying, and need to update
> > > > > > pointers maintained outside the array.  Note this code isn't tested at all, but
> > > > > > its close to what I think should work.
> > > > > >
> > > > > >
> > > > > > diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
> > > > > > index b94fa61b51fb..7fa1f27a91b5 100644
> > > > > > --- a/include/linux/flex_array.h
> > > > > > +++ b/include/linux/flex_array.h
> > > > > > @@ -73,6 +73,8 @@ struct flex_array {
> > > > > >  struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > > >                 gfp_t flags);
> > > > > >
> > > > > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags);
> > > > > > +
> > > > > >  /**
> > > > > >   * flex_array_prealloc() - Ensures that memory for the elements indexed in the
> > > > > >   * range defined by start and nr_elements has been allocated.
> > > > > > diff --git a/lib/flex_array.c b/lib/flex_array.c
> > > > > > index 2eed22fa507c..f8d54af3891b 100644
> > > > > > --- a/lib/flex_array.c
> > > > > > +++ b/lib/flex_array.c
> > > > > > @@ -109,6 +109,7 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > > >         ret->total_nr_elements = total;
> > > > > >         ret->elems_per_part = elems_per_part;
> > > > > >         ret->reciprocal_elems = reciprocal_elems;
> > > > > > +       ret->elements_used = 0;
> > > > > >         if (elements_fit_in_base(ret) && !(flags & __GFP_ZERO))
> > > > > >                 memset(&ret->parts[0], FLEX_ARRAY_FREE,
> > > > > >                                                 FLEX_ARRAY_BASE_BYTES_LEFT);
> > > > > > @@ -116,6 +117,53 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > > >  }
> > > > > >  EXPORT_SYMBOL(flex_array_alloc);
> > > > > >
> > > > > > +static int flex_array_last_element_index(struct flex_array *fa)
> > > > > > +{
> > > > > > +       struct flex_array_part *part;
> > > > > > +       int part_nr;
> > > > > > +       int i,j;
> > > > > > +
> > > > > > +       if (elements_fit_in_base(fa)) {
> > > > > > +               part = (struct flex_array_part *)&fa->parts[0];
> > > > > > +               for (i = fa->elems_per_part; i >= 0; i--)
> > > > > > +                       if (part->elements[i] != FLEX_ARRAY_FREE)
> > > > > > +                               return i;
> > > > > > +       }
> > > > > > +
> > > > > > +       i = fa->total_nr_elements;
> > > > > > +       for (part_nr = 0; part_nr < FLEX_ARRAY_NR_BASE_PTRS; part_nr++) {
> > > > > > +               part = fa->parts[part_nr];
> > > > > > +               if (!part) {
> > > > > > +                       i -= fa->elems_per_part;
> > > > > > +                       continue;
> > > > > > +               }
> > > > > > +               for (j = fa->elems_per_part; j >= 0; j--, i--)
> > > > > > +                       if (part->elements[j] != FLEX_ARRAY_FREE)
> > > > > > +                               goto out;
> > > > > > +       }
> > > > > > +out:
> > > > > > +       return i;
> > > > > > +}
> > > > > > +
> > > > > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags)
> > > > > > +{
> > > > > > +       if (total >= fa->total_nr_elements) {
> > > > > > +               /* Grow case */
> > > > > > +               if (total > max_size)
> > > > > > +                       return ERR_PTR(-ETOOBIG);
> > > > > > +               fa->total_nr_elements = total;
> > > > > > +       } else {
> > > > > > +               /* Shrink case */
> > > > > > +               /* Drop any pages we don't need*/
> > > > > > +               flex_array_shrink(fa);
> > > > > > +               if (flex_array_last_element_index(fa) >= total)
> > > > > > +                       return ERR_PTR(-ESIZE);
> > > > > > +               fa->total_nr_elements = total;
> > > > > > +       }
> > > > > > +       return fa;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(flex_array_resize);
> > > > > > +
> > > > > >  static int fa_element_to_part_nr(struct flex_array *fa,
> > > > > >                                         unsigned int element_nr)
> > > > > >  {
> > > > > I have a question about how it checks one part is free in
> > > > > flex_array_shrink():
> > > > >   part_is_free() is doing it by checking:
> > > > >   if (part->elements[all] == FLEX_ARRAY_FREE) ...
> > > > >
> > > > > What if the data in the array that users put is FLEX_ARRAY_FREE,
> > > > > it will be treated as free, but in fact it's still in use?
> > > > >
> > > > I noticed that too, I think they're taking an unsafe shortcut honestly, assuming
> > > > that the FLEX_ARRAY_FREE poison value in the first byte is indicative of the
> > > > entire part being free.  For this use case its not likely a problem, because the
> > > > first element of the stream is a pointer, and so its unlikely to start with the
> > > > FLEX_ARRAY_FREE value, but it could suffer from aliasing in other cases.  In the
> > > > current design they should really be doing a memory comparison of the entire
> > > > object length to a buffer filled with the poison value to determine free status.
> > > Here's still another problem, I'm not sure if you have noticed that:
> > > When (elements_fit_in_base(fa)) or data_size <= FLEX_ARRAY_BASE_BYTES_LEFT
> > I don't see this as an or.  The only place I see a comparison being made is in
> > elements_fit_in_base:
> >
> > static inline int elements_fit_in_base(struct flex_array *fa)
> > {
> >         int data_size = fa->element_size * fa->total_nr_elements;
> >         if (data_size <= FLEX_ARRAY_BASE_BYTES_LEFT)
> >                 return 1;
> >         return 0;
> > }
> >
> > I understand your point in that in the growth case, its possible that we might
> > cross the border in which elements which did fit in the base now no longer do.
> > I would think that the proper approach here is to:
> >
> > 1) Split the parts array out to its own page independent of the flex_array
> > structure
> >
> > 2) Adjust FLEX_ARRAY_BASE_BYTES_LEFT to not include the offsetof adjustment
> > (i.e. just be defined as FLEX_ARRAY_BASE_SIZE
> >
> > If we were to do that, then in the growth case, if we pass the fit_in_base
> > threshold, we can just reallocate the parts page, and demote the old parts page
> > to be pointed to by parts[0].  This would still maintain the pointer values for
> > the elements and adjust the lookup process accordingly
> >
> > likewise in the shrink case, we can then, if we now fit into the base, we can
> > promote parts[0] to be the parts array.
> got a draft patch, seems to work well so far.
> 
> https://paste.fedoraproject.org/paste/n1GSNLVutxOd2HNvTOzyPQ#
> 
> I'm thinking if we should do the shrink in sctp like in the patch,
> or put it into flex_array_resize(),  considering FLEX_ARRAY_FREE
> may not work for other cases.
> 
Sorry, didn't answer this in my last email. It think it makes sense to do the
shrink inside the resize function, just to ensure that memory won't go unsued.

Neil

> >
> > Neil
> >
> > >   part_0 = (struct flex_array_part *)&fa->parts[0]
> > > otherwise,
> > >   part_0 = fa->parts[0].
> > >
> > > That means when the new out streams' size is greater than
> > > FLEX_ARRAY_BASE_BYTES_LEFT, resize is not that easy to just set
> > > fa->total_nr_elements. We have to do something like what flex_array_alloc
> > > does, and the old elements memory will have to be lost.
> > > With that, this issue won't be fixed.
> > >
> > > After checking the flex_array code, I think flex_array is not yet working
> > > for dynamical resize, which is not its goal either.
> > >
> > > >
> > > > I would suggest not worrying about it for the purposes of this patch, but I
> > > > think I'll write a fix for the flex_array code that adds a byte to the head of
> > > > whatever object is bing stored to be reserved for free / in-use indicators.
> > > > Neil
> > > >
> > >
> 

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Neil Horman]

On Sat, Dec 01, 2018 at 03:53:26AM +0900, Xin Long wrote:
> On Sat, Dec 1, 2018 at 12:23 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Fri, Nov 30, 2018 at 10:48:10PM +0900, Xin Long wrote:
> > > On Fri, Nov 30, 2018 at 9:21 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > >
> > > > On Fri, Nov 30, 2018 at 03:22:39PM +0900, Xin Long wrote:
> > > > > On Thu, Nov 29, 2018 at 11:39 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > > >
> > > > > > On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > > > > > > Now when using stream reconfig to add out streams, stream->out
> > > > > > > will get re-allocated, and all old streams' information will
> > > > > > > be copied to the new ones and the old ones will be freed.
> > > > > > >
> > > > > > > So without stream->out_curr updated, next time when trying to
> > > > > > > send from stream->out_curr stream, a panic would be caused.
> > > > > > >
> > > > > > > This patch is to check and update stream->out_curr when
> > > > > > > allocating stream_out.
> > > > > > >
> > > > > > > v1->v2:
> > > > > > >   - define fa_index() to get elem index from stream->out_curr.
> > > > > > >
> > > > > > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > > > > > > Reported-by: Ying Xu <yinxu@redhat.com>
> > > > > > > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > > > ---
> > > > > > >  net/sctp/stream.c | 20 ++++++++++++++++++++
> > > > > > >  1 file changed, 20 insertions(+)
> > > > > > >
> > > > > > > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > > > > > > index 3892e76..30e7809 100644
> > > > > > > --- a/net/sctp/stream.c
> > > > > > > +++ b/net/sctp/stream.c
> > > > > > > @@ -84,6 +84,19 @@ static void fa_zero(struct flex_array *fa, size_t index, size_t count)
> > > > > > >       }
> > > > > > >  }
> > > > > > >
> > > > > > > +static size_t fa_index(struct flex_array *fa, void *elem, size_t count)
> > > > > > > +{
> > > > > > > +     size_t index = 0;
> > > > > > > +
> > > > > > > +     while (count--) {
> > > > > > > +             if (elem = flex_array_get(fa, index))
> > > > > > > +                     break;
> > > > > > > +             index++;
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     return index;
> > > > > > > +}
> > > > > > > +
> > > > > > >  /* Migrates chunks from stream queues to new stream queues if needed,
> > > > > > >   * but not across associations. Also, removes those chunks to streams
> > > > > > >   * higher than the new max.
> > > > > > > @@ -147,6 +160,13 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> > > > > > >
> > > > > > >       if (stream->out) {
> > > > > > >               fa_copy(out, stream->out, 0, min(outcnt, stream->outcnt));
> > > > > > > +             if (stream->out_curr) {
> > > > > > > +                     size_t index = fa_index(stream->out, stream->out_curr,
> > > > > > > +                                             stream->outcnt);
> > > > > > > +
> > > > > > > +                     BUG_ON(index = stream->outcnt);
> > > > > > > +                     stream->out_curr = flex_array_get(out, index);
> > > > > > > +             }
> > > > > > >               fa_free(stream->out);
> > > > > > >       }
> > > > > > >
> > > > > > > --
> > > > > > > 2.1.0
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > This is the sort of thing I'm talking about. Its a little more code, but if you
> > > > > > augment the flex_array api like this, you can preform a resize operation on your
> > > > > > existing flex array, and you can avoid all the copying, and need to update
> > > > > > pointers maintained outside the array.  Note this code isn't tested at all, but
> > > > > > its close to what I think should work.
> > > > > >
> > > > > >
> > > > > > diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
> > > > > > index b94fa61b51fb..7fa1f27a91b5 100644
> > > > > > --- a/include/linux/flex_array.h
> > > > > > +++ b/include/linux/flex_array.h
> > > > > > @@ -73,6 +73,8 @@ struct flex_array {
> > > > > >  struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > > >                 gfp_t flags);
> > > > > >
> > > > > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags);
> > > > > > +
> > > > > >  /**
> > > > > >   * flex_array_prealloc() - Ensures that memory for the elements indexed in the
> > > > > >   * range defined by start and nr_elements has been allocated.
> > > > > > diff --git a/lib/flex_array.c b/lib/flex_array.c
> > > > > > index 2eed22fa507c..f8d54af3891b 100644
> > > > > > --- a/lib/flex_array.c
> > > > > > +++ b/lib/flex_array.c
> > > > > > @@ -109,6 +109,7 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > > >         ret->total_nr_elements = total;
> > > > > >         ret->elems_per_part = elems_per_part;
> > > > > >         ret->reciprocal_elems = reciprocal_elems;
> > > > > > +       ret->elements_used = 0;
> > > > > >         if (elements_fit_in_base(ret) && !(flags & __GFP_ZERO))
> > > > > >                 memset(&ret->parts[0], FLEX_ARRAY_FREE,
> > > > > >                                                 FLEX_ARRAY_BASE_BYTES_LEFT);
> > > > > > @@ -116,6 +117,53 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
> > > > > >  }
> > > > > >  EXPORT_SYMBOL(flex_array_alloc);
> > > > > >
> > > > > > +static int flex_array_last_element_index(struct flex_array *fa)
> > > > > > +{
> > > > > > +       struct flex_array_part *part;
> > > > > > +       int part_nr;
> > > > > > +       int i,j;
> > > > > > +
> > > > > > +       if (elements_fit_in_base(fa)) {
> > > > > > +               part = (struct flex_array_part *)&fa->parts[0];
> > > > > > +               for (i = fa->elems_per_part; i >= 0; i--)
> > > > > > +                       if (part->elements[i] != FLEX_ARRAY_FREE)
> > > > > > +                               return i;
> > > > > > +       }
> > > > > > +
> > > > > > +       i = fa->total_nr_elements;
> > > > > > +       for (part_nr = 0; part_nr < FLEX_ARRAY_NR_BASE_PTRS; part_nr++) {
> > > > > > +               part = fa->parts[part_nr];
> > > > > > +               if (!part) {
> > > > > > +                       i -= fa->elems_per_part;
> > > > > > +                       continue;
> > > > > > +               }
> > > > > > +               for (j = fa->elems_per_part; j >= 0; j--, i--)
> > > > > > +                       if (part->elements[j] != FLEX_ARRAY_FREE)
> > > > > > +                               goto out;
> > > > > > +       }
> > > > > > +out:
> > > > > > +       return i;
> > > > > > +}
> > > > > > +
> > > > > > +struct flex_array *flex_array_resize(struct flex_array *fa, unsigned int total, gfp_t flags)
> > > > > > +{
> > > > > > +       if (total >= fa->total_nr_elements) {
> > > > > > +               /* Grow case */
> > > > > > +               if (total > max_size)
> > > > > > +                       return ERR_PTR(-ETOOBIG);
> > > > > > +               fa->total_nr_elements = total;
> > > > > > +       } else {
> > > > > > +               /* Shrink case */
> > > > > > +               /* Drop any pages we don't need*/
> > > > > > +               flex_array_shrink(fa);
> > > > > > +               if (flex_array_last_element_index(fa) >= total)
> > > > > > +                       return ERR_PTR(-ESIZE);
> > > > > > +               fa->total_nr_elements = total;
> > > > > > +       }
> > > > > > +       return fa;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(flex_array_resize);
> > > > > > +
> > > > > >  static int fa_element_to_part_nr(struct flex_array *fa,
> > > > > >                                         unsigned int element_nr)
> > > > > >  {
> > > > > I have a question about how it checks one part is free in
> > > > > flex_array_shrink():
> > > > >   part_is_free() is doing it by checking:
> > > > >   if (part->elements[all] = FLEX_ARRAY_FREE) ...
> > > > >
> > > > > What if the data in the array that users put is FLEX_ARRAY_FREE,
> > > > > it will be treated as free, but in fact it's still in use?
> > > > >
> > > > I noticed that too, I think they're taking an unsafe shortcut honestly, assuming
> > > > that the FLEX_ARRAY_FREE poison value in the first byte is indicative of the
> > > > entire part being free.  For this use case its not likely a problem, because the
> > > > first element of the stream is a pointer, and so its unlikely to start with the
> > > > FLEX_ARRAY_FREE value, but it could suffer from aliasing in other cases.  In the
> > > > current design they should really be doing a memory comparison of the entire
> > > > object length to a buffer filled with the poison value to determine free status.
> > > Here's still another problem, I'm not sure if you have noticed that:
> > > When (elements_fit_in_base(fa)) or data_size <= FLEX_ARRAY_BASE_BYTES_LEFT
> > I don't see this as an or.  The only place I see a comparison being made is in
> > elements_fit_in_base:
> >
> > static inline int elements_fit_in_base(struct flex_array *fa)
> > {
> >         int data_size = fa->element_size * fa->total_nr_elements;
> >         if (data_size <= FLEX_ARRAY_BASE_BYTES_LEFT)
> >                 return 1;
> >         return 0;
> > }
> >
> > I understand your point in that in the growth case, its possible that we might
> > cross the border in which elements which did fit in the base now no longer do.
> > I would think that the proper approach here is to:
> >
> > 1) Split the parts array out to its own page independent of the flex_array
> > structure
> >
> > 2) Adjust FLEX_ARRAY_BASE_BYTES_LEFT to not include the offsetof adjustment
> > (i.e. just be defined as FLEX_ARRAY_BASE_SIZE
> >
> > If we were to do that, then in the growth case, if we pass the fit_in_base
> > threshold, we can just reallocate the parts page, and demote the old parts page
> > to be pointed to by parts[0].  This would still maintain the pointer values for
> > the elements and adjust the lookup process accordingly
> >
> > likewise in the shrink case, we can then, if we now fit into the base, we can
> > promote parts[0] to be the parts array.
> got a draft patch, seems to work well so far.
> 
> https://paste.fedoraproject.org/paste/n1GSNLVutxOd2HNvTOzyPQ#
> 
> I'm thinking if we should do the shrink in sctp like in the patch,
> or put it into flex_array_resize(),  considering FLEX_ARRAY_FREE
> may not work for other cases.
> 
Sorry, didn't answer this in my last email. It think it makes sense to do the
shrink inside the resize function, just to ensure that memory won't go unsued.

Neil

> >
> > Neil
> >
> > >   part_0 = (struct flex_array_part *)&fa->parts[0]
> > > otherwise,
> > >   part_0 = fa->parts[0].
> > >
> > > That means when the new out streams' size is greater than
> > > FLEX_ARRAY_BASE_BYTES_LEFT, resize is not that easy to just set
> > > fa->total_nr_elements. We have to do something like what flex_array_alloc
> > > does, and the old elements memory will have to be lost.
> > > With that, this issue won't be fixed.
> > >
> > > After checking the flex_array code, I think flex_array is not yet working
> > > for dynamical resize, which is not its goal either.
> > >
> > > >
> > > > I would suggest not worrying about it for the purposes of this patch, but I
> > > > think I'll write a fix for the flex_array code that adds a byte to the head of
> > > > whatever object is bing stored to be reserved for free / in-use indicators.
> > > > Neil
> > > >
> > >
> 

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Marcelo Ricardo Leitner]

On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> Now when using stream reconfig to add out streams, stream->out
> will get re-allocated, and all old streams' information will
> be copied to the new ones and the old ones will be freed.
> 
> So without stream->out_curr updated, next time when trying to
> send from stream->out_curr stream, a panic would be caused.
> 
> This patch is to check and update stream->out_curr when
> allocating stream_out.
> 
> v1->v2:
>   - define fa_index() to get elem index from stream->out_curr.
> 
> Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> Reported-by: Ying Xu <yinxu@redhat.com>
> Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

We are sort of mixing things up here. We have a bug on SCTP stack that
triggers panics. As good practices recommends, the code should be as
generic as possible and the SCTP-only was dropped in favor of a more
generic one, fixing rhashtables instead. Okay. But then we discovered
rhashtables are going away and we are now waiting on a restructing
to fix the panic. That's not good, especially because it cannot and
should not be backported into -stable trees.

That said, we should not wait for the restructuring to _implicitly_
fix the bug. We should pursuit both fixes here:
- Apply this patch, to fix SCTP stack and allow it to be easily
  backportable.
- Apply the generic fix, which is the restructuring, whenever it
  actually lands.

Thoughts?

Thanks,
Marcelo

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Marcelo Ricardo Leitner]

On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> Now when using stream reconfig to add out streams, stream->out
> will get re-allocated, and all old streams' information will
> be copied to the new ones and the old ones will be freed.
> 
> So without stream->out_curr updated, next time when trying to
> send from stream->out_curr stream, a panic would be caused.
> 
> This patch is to check and update stream->out_curr when
> allocating stream_out.
> 
> v1->v2:
>   - define fa_index() to get elem index from stream->out_curr.
> 
> Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> Reported-by: Ying Xu <yinxu@redhat.com>
> Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

We are sort of mixing things up here. We have a bug on SCTP stack that
triggers panics. As good practices recommends, the code should be as
generic as possible and the SCTP-only was dropped in favor of a more
generic one, fixing rhashtables instead. Okay. But then we discovered
rhashtables are going away and we are now waiting on a restructing
to fix the panic. That's not good, especially because it cannot and
should not be backported into -stable trees.

That said, we should not wait for the restructuring to _implicitly_
fix the bug. We should pursuit both fixes here:
- Apply this patch, to fix SCTP stack and allow it to be easily
  backportable.
- Apply the generic fix, which is the restructuring, whenever it
  actually lands.

Thoughts?

Thanks,
Marcelo

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Tuxdriver]

I was initially under the impression that with Kent's repost, the radixtree 
(which is what I think you meant by rhashtables) updates would be merged 
imminently, but that doesn't seem to be the case.  I'd really like to know 
what the hold up there is, as that patch seems to have been stalled for 
months.  I hate the notion of breaking the radixtree patch, but if it's 
status is indeterminate, then, yes, we probably need to go with xins patch 
for the short term, and let Kent fix it up in due course.

Neil

On January 29, 2019 1:06:33 PM Marcelo Ricardo Leitner 
<marcelo.leitner@gmail.com> wrote:

> On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
>> Now when using stream reconfig to add out streams, stream->out
>> will get re-allocated, and all old streams' information will
>> be copied to the new ones and the old ones will be freed.
>>
>> So without stream->out_curr updated, next time when trying to
>> send from stream->out_curr stream, a panic would be caused.
>>
>> This patch is to check and update stream->out_curr when
>> allocating stream_out.
>>
>> v1->v2:
>>   - define fa_index() to get elem index from stream->out_curr.
>>
>> Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
>> Reported-by: Ying Xu <yinxu@redhat.com>
>> Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>
> We are sort of mixing things up here. We have a bug on SCTP stack that
> triggers panics. As good practices recommends, the code should be as
> generic as possible and the SCTP-only was dropped in favor of a more
> generic one, fixing rhashtables instead. Okay. But then we discovered
> rhashtables are going away and we are now waiting on a restructing
> to fix the panic. That's not good, especially because it cannot and
> should not be backported into -stable trees.
>
> That said, we should not wait for the restructuring to _implicitly_
> fix the bug. We should pursuit both fixes here:
> - Apply this patch, to fix SCTP stack and allow it to be easily
>  backportable.
> - Apply the generic fix, which is the restructuring, whenever it
>  actually lands.
>
> Thoughts?
>
> Thanks,
> Marcelo


Sent with AquaMail for Android
https://www.mobisystems.com/aqua-mail

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Tuxdriver]

I was initially under the impression that with Kent's repost, the radixtree 
(which is what I think you meant by rhashtables) updates would be merged 
imminently, but that doesn't seem to be the case.  I'd really like to know 
what the hold up there is, as that patch seems to have been stalled for 
months.  I hate the notion of breaking the radixtree patch, but if it's 
status is indeterminate, then, yes, we probably need to go with xins patch 
for the short term, and let Kent fix it up in due course.

Neil

On January 29, 2019 1:06:33 PM Marcelo Ricardo Leitner 
<marcelo.leitner@gmail.com> wrote:

> On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
>> Now when using stream reconfig to add out streams, stream->out
>> will get re-allocated, and all old streams' information will
>> be copied to the new ones and the old ones will be freed.
>>
>> So without stream->out_curr updated, next time when trying to
>> send from stream->out_curr stream, a panic would be caused.
>>
>> This patch is to check and update stream->out_curr when
>> allocating stream_out.
>>
>> v1->v2:
>>   - define fa_index() to get elem index from stream->out_curr.
>>
>> Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
>> Reported-by: Ying Xu <yinxu@redhat.com>
>> Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>
> We are sort of mixing things up here. We have a bug on SCTP stack that
> triggers panics. As good practices recommends, the code should be as
> generic as possible and the SCTP-only was dropped in favor of a more
> generic one, fixing rhashtables instead. Okay. But then we discovered
> rhashtables are going away and we are now waiting on a restructing
> to fix the panic. That's not good, especially because it cannot and
> should not be backported into -stable trees.
>
> That said, we should not wait for the restructuring to _implicitly_
> fix the bug. We should pursuit both fixes here:
> - Apply this patch, to fix SCTP stack and allow it to be easily
>  backportable.
> - Apply the generic fix, which is the restructuring, whenever it
>  actually lands.
>
> Thoughts?
>
> Thanks,
> Marcelo


Sent with AquaMail for Android
https://www.mobisystems.com/aqua-mail

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Marcelo Ricardo Leitner]

On Tue, Jan 29, 2019 at 07:58:07PM +0100, Tuxdriver wrote:
> I was initially under the impression that with Kent's repost, the radixtree
> (which is what I think you meant by rhashtables) updates would be merged

Oops! Yep.. I had meant flex_arrays actually.

> imminently, but that doesn't seem to be the case.  I'd really like to know
> what the hold up there is, as that patch seems to have been stalled for
> months.  I hate the notion of breaking the radixtree patch, but if it's
> status is indeterminate, then, yes, we probably need to go with xins patch
> for the short term, and let Kent fix it up in due course.

Dave, can you please consider applying this patch? The conflict
resolution will be easy: just ignore the changes introduced by this
patch.

This is the radixtree converstion:
https://lwn.net/ml/linux-kernel/20181217131929.11727-1-kent.overstreet@gmail.com/
Seems that went to a limbo after
https://lwn.net/ml/linux-kernel/20181217210021.GA7144@kmo-pixel/
Maybe Kent should have reposted, but he didn't reply either.

My reasoning is below. Just please also notice that this is
triggerable by users and remotely, as remote peers may request to add
'in' streams and that implies in adding 'out' streams on local peer.
(https://tools.ietf.org/html/rfc6525#section-5.2.6)

> 
> Neil
> 
> On January 29, 2019 1:06:33 PM Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> 
> > On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > > Now when using stream reconfig to add out streams, stream->out
> > > will get re-allocated, and all old streams' information will
> > > be copied to the new ones and the old ones will be freed.
> > > 
> > > So without stream->out_curr updated, next time when trying to
> > > send from stream->out_curr stream, a panic would be caused.
> > > 
> > > This patch is to check and update stream->out_curr when
> > > allocating stream_out.
> > > 
> > > v1->v2:
> > >   - define fa_index() to get elem index from stream->out_curr.
> > > 
> > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > > Reported-by: Ying Xu <yinxu@redhat.com>
> > > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > 
> > We are sort of mixing things up here. We have a bug on SCTP stack that
> > triggers panics. As good practices recommends, the code should be as
> > generic as possible and the SCTP-only was dropped in favor of a more
> > generic one, fixing rhashtables instead. Okay. But then we discovered
> > rhashtables are going away and we are now waiting on a restructing
> > to fix the panic. That's not good, especially because it cannot and
> > should not be backported into -stable trees.
> > 
> > That said, we should not wait for the restructuring to _implicitly_
> > fix the bug. We should pursuit both fixes here:
> > - Apply this patch, to fix SCTP stack and allow it to be easily
> >  backportable.
> > - Apply the generic fix, which is the restructuring, whenever it
> >  actually lands.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Marcelo
> 
> 
> Sent with AquaMail for Android
> https://www.mobisystems.com/aqua-mail
> 
> 

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Marcelo Ricardo Leitner]

On Tue, Jan 29, 2019 at 07:58:07PM +0100, Tuxdriver wrote:
> I was initially under the impression that with Kent's repost, the radixtree
> (which is what I think you meant by rhashtables) updates would be merged

Oops! Yep.. I had meant flex_arrays actually.

> imminently, but that doesn't seem to be the case.  I'd really like to know
> what the hold up there is, as that patch seems to have been stalled for
> months.  I hate the notion of breaking the radixtree patch, but if it's
> status is indeterminate, then, yes, we probably need to go with xins patch
> for the short term, and let Kent fix it up in due course.

Dave, can you please consider applying this patch? The conflict
resolution will be easy: just ignore the changes introduced by this
patch.

This is the radixtree converstion:
https://lwn.net/ml/linux-kernel/20181217131929.11727-1-kent.overstreet@gmail.com/
Seems that went to a limbo after
https://lwn.net/ml/linux-kernel/20181217210021.GA7144@kmo-pixel/
Maybe Kent should have reposted, but he didn't reply either.

My reasoning is below. Just please also notice that this is
triggerable by users and remotely, as remote peers may request to add
'in' streams and that implies in adding 'out' streams on local peer.
(https://tools.ietf.org/html/rfc6525#section-5.2.6)

> 
> Neil
> 
> On January 29, 2019 1:06:33 PM Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> 
> > On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > > Now when using stream reconfig to add out streams, stream->out
> > > will get re-allocated, and all old streams' information will
> > > be copied to the new ones and the old ones will be freed.
> > > 
> > > So without stream->out_curr updated, next time when trying to
> > > send from stream->out_curr stream, a panic would be caused.
> > > 
> > > This patch is to check and update stream->out_curr when
> > > allocating stream_out.
> > > 
> > > v1->v2:
> > >   - define fa_index() to get elem index from stream->out_curr.
> > > 
> > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > > Reported-by: Ying Xu <yinxu@redhat.com>
> > > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > 
> > We are sort of mixing things up here. We have a bug on SCTP stack that
> > triggers panics. As good practices recommends, the code should be as
> > generic as possible and the SCTP-only was dropped in favor of a more
> > generic one, fixing rhashtables instead. Okay. But then we discovered
> > rhashtables are going away and we are now waiting on a restructing
> > to fix the panic. That's not good, especially because it cannot and
> > should not be backported into -stable trees.
> > 
> > That said, we should not wait for the restructuring to _implicitly_
> > fix the bug. We should pursuit both fixes here:
> > - Apply this patch, to fix SCTP stack and allow it to be easily
> >  backportable.
> > - Apply the generic fix, which is the restructuring, whenever it
> >  actually lands.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Marcelo
> 
> 
> Sent with AquaMail for Android
> https://www.mobisystems.com/aqua-mail
> 
> 

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Neil Horman]

On Thu, Jan 31, 2019 at 10:39:41PM -0200, Marcelo Ricardo Leitner wrote:
> On Tue, Jan 29, 2019 at 07:58:07PM +0100, Tuxdriver wrote:
> > I was initially under the impression that with Kent's repost, the radixtree
> > (which is what I think you meant by rhashtables) updates would be merged
> 
> Oops! Yep.. I had meant flex_arrays actually.
> 
> > imminently, but that doesn't seem to be the case.  I'd really like to know
> > what the hold up there is, as that patch seems to have been stalled for
> > months.  I hate the notion of breaking the radixtree patch, but if it's
> > status is indeterminate, then, yes, we probably need to go with xins patch
> > for the short term, and let Kent fix it up in due course.
> 
> Dave, can you please consider applying this patch? The conflict
> resolution will be easy: just ignore the changes introduced by this
> patch.
> 
Dave I concur with Marcelo here.  Kent was very active in getting sctp fixed up
to use radixtrees, but now he seems to have gone to ground, and for whatever
reason, no one seems interested in incorporating his patch.  Its been languising
for months, so I think we need to take action to secure sctp now until such time
as his genradix changes finally move forward.

Neil

> This is the radixtree converstion:
> https://lwn.net/ml/linux-kernel/20181217131929.11727-1-kent.overstreet@gmail.com/
> Seems that went to a limbo after
> https://lwn.net/ml/linux-kernel/20181217210021.GA7144@kmo-pixel/
> Maybe Kent should have reposted, but he didn't reply either.
> 
> My reasoning is below. Just please also notice that this is
> triggerable by users and remotely, as remote peers may request to add
> 'in' streams and that implies in adding 'out' streams on local peer.
> (https://tools.ietf.org/html/rfc6525#section-5.2.6)
> 
> > 
> > Neil
> > 
> > On January 29, 2019 1:06:33 PM Marcelo Ricardo Leitner
> > <marcelo.leitner@gmail.com> wrote:
> > 
> > > On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > > > Now when using stream reconfig to add out streams, stream->out
> > > > will get re-allocated, and all old streams' information will
> > > > be copied to the new ones and the old ones will be freed.
> > > > 
> > > > So without stream->out_curr updated, next time when trying to
> > > > send from stream->out_curr stream, a panic would be caused.
> > > > 
> > > > This patch is to check and update stream->out_curr when
> > > > allocating stream_out.
> > > > 
> > > > v1->v2:
> > > >   - define fa_index() to get elem index from stream->out_curr.
> > > > 
> > > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > > > Reported-by: Ying Xu <yinxu@redhat.com>
> > > > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > 
> > > We are sort of mixing things up here. We have a bug on SCTP stack that
> > > triggers panics. As good practices recommends, the code should be as
> > > generic as possible and the SCTP-only was dropped in favor of a more
> > > generic one, fixing rhashtables instead. Okay. But then we discovered
> > > rhashtables are going away and we are now waiting on a restructing
> > > to fix the panic. That's not good, especially because it cannot and
> > > should not be backported into -stable trees.
> > > 
> > > That said, we should not wait for the restructuring to _implicitly_
> > > fix the bug. We should pursuit both fixes here:
> > > - Apply this patch, to fix SCTP stack and allow it to be easily
> > >  backportable.
> > > - Apply the generic fix, which is the restructuring, whenever it
> > >  actually lands.
> > > 
> > > Thoughts?
> > > 
> > > Thanks,
> > > Marcelo
> > 
> > 
> > Sent with AquaMail for Android
> > https://www.mobisystems.com/aqua-mail
> > 
> > 
> 

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Neil Horman]

On Thu, Jan 31, 2019 at 10:39:41PM -0200, Marcelo Ricardo Leitner wrote:
> On Tue, Jan 29, 2019 at 07:58:07PM +0100, Tuxdriver wrote:
> > I was initially under the impression that with Kent's repost, the radixtree
> > (which is what I think you meant by rhashtables) updates would be merged
> 
> Oops! Yep.. I had meant flex_arrays actually.
> 
> > imminently, but that doesn't seem to be the case.  I'd really like to know
> > what the hold up there is, as that patch seems to have been stalled for
> > months.  I hate the notion of breaking the radixtree patch, but if it's
> > status is indeterminate, then, yes, we probably need to go with xins patch
> > for the short term, and let Kent fix it up in due course.
> 
> Dave, can you please consider applying this patch? The conflict
> resolution will be easy: just ignore the changes introduced by this
> patch.
> 
Dave I concur with Marcelo here.  Kent was very active in getting sctp fixed up
to use radixtrees, but now he seems to have gone to ground, and for whatever
reason, no one seems interested in incorporating his patch.  Its been languising
for months, so I think we need to take action to secure sctp now until such time
as his genradix changes finally move forward.

Neil

> This is the radixtree converstion:
> https://lwn.net/ml/linux-kernel/20181217131929.11727-1-kent.overstreet@gmail.com/
> Seems that went to a limbo after
> https://lwn.net/ml/linux-kernel/20181217210021.GA7144@kmo-pixel/
> Maybe Kent should have reposted, but he didn't reply either.
> 
> My reasoning is below. Just please also notice that this is
> triggerable by users and remotely, as remote peers may request to add
> 'in' streams and that implies in adding 'out' streams on local peer.
> (https://tools.ietf.org/html/rfc6525#section-5.2.6)
> 
> > 
> > Neil
> > 
> > On January 29, 2019 1:06:33 PM Marcelo Ricardo Leitner
> > <marcelo.leitner@gmail.com> wrote:
> > 
> > > On Thu, Nov 29, 2018 at 02:42:56PM +0800, Xin Long wrote:
> > > > Now when using stream reconfig to add out streams, stream->out
> > > > will get re-allocated, and all old streams' information will
> > > > be copied to the new ones and the old ones will be freed.
> > > > 
> > > > So without stream->out_curr updated, next time when trying to
> > > > send from stream->out_curr stream, a panic would be caused.
> > > > 
> > > > This patch is to check and update stream->out_curr when
> > > > allocating stream_out.
> > > > 
> > > > v1->v2:
> > > >   - define fa_index() to get elem index from stream->out_curr.
> > > > 
> > > > Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
> > > > Reported-by: Ying Xu <yinxu@redhat.com>
> > > > Reported-by: syzbot+e33a3a138267ca119c7d@syzkaller.appspotmail.com
> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > 
> > > We are sort of mixing things up here. We have a bug on SCTP stack that
> > > triggers panics. As good practices recommends, the code should be as
> > > generic as possible and the SCTP-only was dropped in favor of a more
> > > generic one, fixing rhashtables instead. Okay. But then we discovered
> > > rhashtables are going away and we are now waiting on a restructing
> > > to fix the panic. That's not good, especially because it cannot and
> > > should not be backported into -stable trees.
> > > 
> > > That said, we should not wait for the restructuring to _implicitly_
> > > fix the bug. We should pursuit both fixes here:
> > > - Apply this patch, to fix SCTP stack and allow it to be easily
> > >  backportable.
> > > - Apply the generic fix, which is the restructuring, whenever it
> > >  actually lands.
> > > 
> > > Thoughts?
> > > 
> > > Thanks,
> > > Marcelo
> > 
> > 
> > Sent with AquaMail for Android
> > https://www.mobisystems.com/aqua-mail
> > 
> > 
> 

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Fri, 1 Feb 2019 07:31:18 -0500

> On Thu, Jan 31, 2019 at 10:39:41PM -0200, Marcelo Ricardo Leitner wrote:
>> On Tue, Jan 29, 2019 at 07:58:07PM +0100, Tuxdriver wrote:
>> > I was initially under the impression that with Kent's repost, the radixtree
>> > (which is what I think you meant by rhashtables) updates would be merged
>> 
>> Oops! Yep.. I had meant flex_arrays actually.
>> 
>> > imminently, but that doesn't seem to be the case.  I'd really like to know
>> > what the hold up there is, as that patch seems to have been stalled for
>> > months.  I hate the notion of breaking the radixtree patch, but if it's
>> > status is indeterminate, then, yes, we probably need to go with xins patch
>> > for the short term, and let Kent fix it up in due course.
>> 
>> Dave, can you please consider applying this patch? The conflict
>> resolution will be easy: just ignore the changes introduced by this
>> patch.
>> 
> Dave I concur with Marcelo here.  Kent was very active in getting sctp fixed up
> to use radixtrees, but now he seems to have gone to ground, and for whatever
> reason, no one seems interested in incorporating his patch.  Its been languising
> for months, so I think we need to take action to secure sctp now until such time
> as his genradix changes finally move forward.

I agree, please let's get this sctp patch resubmitted and back into
patchwork and I will apply it.

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Fri, 1 Feb 2019 07:31:18 -0500

> On Thu, Jan 31, 2019 at 10:39:41PM -0200, Marcelo Ricardo Leitner wrote:
>> On Tue, Jan 29, 2019 at 07:58:07PM +0100, Tuxdriver wrote:
>> > I was initially under the impression that with Kent's repost, the radixtree
>> > (which is what I think you meant by rhashtables) updates would be merged
>> 
>> Oops! Yep.. I had meant flex_arrays actually.
>> 
>> > imminently, but that doesn't seem to be the case.  I'd really like to know
>> > what the hold up there is, as that patch seems to have been stalled for
>> > months.  I hate the notion of breaking the radixtree patch, but if it's
>> > status is indeterminate, then, yes, we probably need to go with xins patch
>> > for the short term, and let Kent fix it up in due course.
>> 
>> Dave, can you please consider applying this patch? The conflict
>> resolution will be easy: just ignore the changes introduced by this
>> patch.
>> 
> Dave I concur with Marcelo here.  Kent was very active in getting sctp fixed up
> to use radixtrees, but now he seems to have gone to ground, and for whatever
> reason, no one seems interested in incorporating his patch.  Its been languising
> for months, so I think we need to take action to secure sctp now until such time
> as his genradix changes finally move forward.

I agree, please let's get this sctp patch resubmitted and back into
patchwork and I will apply it.

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Xin Long]

On Sat, Feb 2, 2019 at 2:38 AM David Miller <davem@davemloft.net> wrote:
>
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Fri, 1 Feb 2019 07:31:18 -0500
>
> > On Thu, Jan 31, 2019 at 10:39:41PM -0200, Marcelo Ricardo Leitner wrote:
> >> On Tue, Jan 29, 2019 at 07:58:07PM +0100, Tuxdriver wrote:
> >> > I was initially under the impression that with Kent's repost, the radixtree
> >> > (which is what I think you meant by rhashtables) updates would be merged
> >>
> >> Oops! Yep.. I had meant flex_arrays actually.
> >>
> >> > imminently, but that doesn't seem to be the case.  I'd really like to know
> >> > what the hold up there is, as that patch seems to have been stalled for
> >> > months.  I hate the notion of breaking the radixtree patch, but if it's
> >> > status is indeterminate, then, yes, we probably need to go with xins patch
> >> > for the short term, and let Kent fix it up in due course.
> >>
> >> Dave, can you please consider applying this patch? The conflict
> >> resolution will be easy: just ignore the changes introduced by this
> >> patch.
> >>
> > Dave I concur with Marcelo here.  Kent was very active in getting sctp fixed up
> > to use radixtrees, but now he seems to have gone to ground, and for whatever
> > reason, no one seems interested in incorporating his patch.  Its been languising
> > for months, so I think we need to take action to secure sctp now until such time
> > as his genradix changes finally move forward.
>
> I agree, please let's get this sctp patch resubmitted and back into
> patchwork and I will apply it.
reposted.

Re: [PATCHv2 net] sctp: check and update stream->out_curr when allocating stream_out

[Xin Long]

On Sat, Feb 2, 2019 at 2:38 AM David Miller <davem@davemloft.net> wrote:
>
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Fri, 1 Feb 2019 07:31:18 -0500
>
> > On Thu, Jan 31, 2019 at 10:39:41PM -0200, Marcelo Ricardo Leitner wrote:
> >> On Tue, Jan 29, 2019 at 07:58:07PM +0100, Tuxdriver wrote:
> >> > I was initially under the impression that with Kent's repost, the radixtree
> >> > (which is what I think you meant by rhashtables) updates would be merged
> >>
> >> Oops! Yep.. I had meant flex_arrays actually.
> >>
> >> > imminently, but that doesn't seem to be the case.  I'd really like to know
> >> > what the hold up there is, as that patch seems to have been stalled for
> >> > months.  I hate the notion of breaking the radixtree patch, but if it's
> >> > status is indeterminate, then, yes, we probably need to go with xins patch
> >> > for the short term, and let Kent fix it up in due course.
> >>
> >> Dave, can you please consider applying this patch? The conflict
> >> resolution will be easy: just ignore the changes introduced by this
> >> patch.
> >>
> > Dave I concur with Marcelo here.  Kent was very active in getting sctp fixed up
> > to use radixtrees, but now he seems to have gone to ground, and for whatever
> > reason, no one seems interested in incorporating his patch.  Its been languising
> > for months, so I think we need to take action to secure sctp now until such time
> > as his genradix changes finally move forward.
>
> I agree, please let's get this sctp patch resubmitted and back into
> patchwork and I will apply it.
reposted.