Date:   Mon, 26 Nov 2018 08:58:46 -0500
From:   Neil Horman <nhorman@...driver.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        davem <davem@...emloft.net>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Subject: Re: [PATCH net] sctp: check and update stream->out_curr when
 allocating stream_out

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@...driver.com> 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@...hat.com>
> > > Reported-by: syzbot+e33a3a138267ca119c7d@...kaller.appspotmail.com
> > > Signed-off-by: Xin Long <lucien.xin@...il.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.

> > 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

Powered by blists - more mailing lists