On Tue, Nov 27, 2018 at 7:30 PM Xin Long <lucien.xin@xxxxxxxxx> wrote:
>
> On Mon, Nov 26, 2018 at 10:59 PM Neil Horman <nhorman@xxxxxxxxxxxxx> wrote:
> >
> > On Mon, Nov 26, 2018 at 10:46:33PM +0900, Xin Long wrote:
> > > On Mon, Nov 26, 2018 at 9:54 PM Neil Horman <nhorman@xxxxxxxxxxxxx> wrote:
> > > >
> > > > On Mon, Nov 26, 2018 at 07:22:05PM +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 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 <yinxu@xxxxxxxxxx>
> > > > > Reported-by: syzbot+e33a3a138267ca119c7d@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > > > Signed-off-by: Xin Long <lucien.xin@xxxxxxxxx>
> > > > > ---
> > > > > 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;
sorry, I meant:
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 +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);
> }
>
> >
> > > > 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.
> >
> > > 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
> >
