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

On Mon, Nov 26, 2018 at 10:59 PM Neil Horman [off-list ref] wrote:

On Mon, Nov 26, 2018 at 10:46:33PM +0900, Xin Long wrote:

quoted

On Mon, Nov 26, 2018 at 9:54 PM Neil Horman [off-list ref] wrote:

quoted

On Mon, Nov 26, 2018 at 07:22:05PM +0800, Xin Long wrote:

quoted

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 define sctp_stream_out_copy used to update the
stream->out_curr pointer to the new stream when copying the old
streams' information.

While at it, rename fa_copy to sctp_stream_in_copy.

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

diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index 3892e76..0687eeb 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c

@@ -61,18 +61,6 @@ static void fa_free(struct flex_array *fa)
              flex_array_free(fa);
 }

-static void fa_copy(struct flex_array *fa, struct flex_array *from,
-                 size_t index, size_t count)
-{
-     void *elem;
-
-     while (count--) {
-             elem = flex_array_get(from, index);
-             flex_array_put(fa, index, elem, 0);
-             index++;
-     }
-}
-
 static void fa_zero(struct flex_array *fa, size_t index, size_t count)
 {
      void *elem;

@@ -135,6 +123,36 @@ static void sctp_stream_outq_migrate(struct sctp_stream *stream,
              kfree(SCTP_SO(stream, i)->ext);
 }

+static void sctp_stream_in_copy(struct flex_array *fa,
+                             struct sctp_stream *stream, __u16 count)
+{
+     size_t index = 0;
+     void *elem;
+
+     count = min(count, stream->incnt);
+     while (count--) {
+             elem = flex_array_get(stream->in, index);
+             flex_array_put(fa, index, elem, 0);
+             index++;
+     }
+}
+
+static void sctp_stream_out_copy(struct flex_array *fa,
+                              struct sctp_stream *stream, __u16 count)
+{
+     size_t index = 0;
+     void *elem;
+
+     count = min(count, stream->outcnt);
+     while (count--) {
+             elem = flex_array_get(stream->out, index);
+             flex_array_put(fa, index, elem, 0);
+             if (stream->out_curr == elem)
+                     stream->out_curr = flex_array_get(fa, index);
+             index++;
+     }
+}
+

Seems like you are duplicating code here.  I think you would be better off
moving the fa_copy routine to the flex_array api (perhaps renaming it
flex_array_copy), and then codig sctp_stream_*_copy as static inlines that just
call the flex_array api to do the copy.  As for setting the out_curr pointer,
perhaps you should convert that to an index, so it can be looked up on demand,

changing to use index only for this  may not worth it.
there is no API from flex_array to convert element to index either
the index is also the stream_id, but we didn't save it into stream_out
either, too.

Right, I'm saying it would be valuable to augment the flex_array api to include
a copy function, as well as a pointer to index lookup element, that could have
use beyond just sctp.

What do you think of this?

It's not called in tx/rx path, so no performance problem caused.
Since only SCTP is using fa_xxx(), I will not move any to
lib/flex_array.c for now.

+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;
+
+       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 +158,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);
        }

quoted

quoted

so that it doesn't have to be updated here at all, or alternatively, just set it
back to NULL here so that the selected scheduler will be forced to do the next
lookup.

We can't set it back to NULL. Otherwise, the scheduler may go to
send other msg if the last msg (with multiple chunks) is not yet sent
out completely, which is not allowed when it's not I-Data chunk.

If setting it back to NULL isn't valid, then the above is your solution.

quoted

This is not much duplicating, and this can reduce few params.
I'm actually ok with this.

I know your ok with it, you wrote the patch :).  I however, am not really ok
with the duplication.  I would like to see it collapsed, if its not going to
create a significant performance impact.

Neil