| Message ID | 20190227.170024.1735727318522180688.davem@davemloft.net |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | SCTP: Event skb list overhaul. | expand |
On Wed, Feb 27, 2019 at 05:00:24PM -0800, David Miller wrote: > > Pass this, instead of an event. Then everything trickles down and we > always have events a non-empty list. > > Then we needs a list creating stub to place into .enqueue_event for sctp_stream_interleave_1. > > Signed-off-by: David S. Miller <davem@davemloft.net> > --- > net/sctp/stream_interleave.c | 44 +++++++++++++++++++++++++++--------- > 1 file changed, 33 insertions(+), 11 deletions(-) > > diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c > index b6b251b8b3cf..0bc3d9329d9a 100644 > --- a/net/sctp/stream_interleave.c > +++ b/net/sctp/stream_interleave.c ... > @@ -866,11 +867,15 @@ static int sctp_ulpevent_idata(struct sctp_ulpq *ulpq, More context: if (!(event->msg_flags & SCTP_DATA_UNORDERED)) { event = sctp_intl_reasm(ulpq, event); [1] if (event && event->msg_flags & MSG_EOR) { [2] skb_queue_head_init(&temp); __skb_queue_tail(&temp, sctp_event2skb(event)); event = sctp_intl_order(ulpq, event); > } > } else { > event = sctp_intl_reasm_uo(ulpq, event); > + if (event) { > + skb_queue_head_init(&temp); > + __skb_queue_tail(&temp, sctp_event2skb(event)); > + } > } > > if (event) { > event_eor = (event->msg_flags & MSG_EOR) ? 1 : 0; > - sctp_enqueue_event(ulpq, event); > + sctp_enqueue_event(ulpq, &temp); [1] can return an event without MSG_EOR (a partial delivery), which would skip the condition on [2] and cause temp to not be initialized by here. Same applies to sctp_ulpq_tail_data(). It's the only thing I noticed on the series. Will test it tomorrow. > } > > return event_eor; ...
On Wed, Feb 27, 2019 at 11:19:58PM -0300, Marcelo Ricardo Leitner wrote: > On Wed, Feb 27, 2019 at 05:00:24PM -0800, David Miller wrote: > > > > Pass this, instead of an event. Then everything trickles down and we > > always have events a non-empty list. > > > > Then we needs a list creating stub to place into .enqueue_event for sctp_stream_interleave_1. > > > > Signed-off-by: David S. Miller <davem@davemloft.net> > > --- > > net/sctp/stream_interleave.c | 44 +++++++++++++++++++++++++++--------- > > 1 file changed, 33 insertions(+), 11 deletions(-) > > > > diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c > > index b6b251b8b3cf..0bc3d9329d9a 100644 > > --- a/net/sctp/stream_interleave.c > > +++ b/net/sctp/stream_interleave.c > ... > > @@ -866,11 +867,15 @@ static int sctp_ulpevent_idata(struct sctp_ulpq *ulpq, > > More context: > if (!(event->msg_flags & SCTP_DATA_UNORDERED)) { > event = sctp_intl_reasm(ulpq, event); [1] > if (event && event->msg_flags & MSG_EOR) { [2] > skb_queue_head_init(&temp); > __skb_queue_tail(&temp, sctp_event2skb(event)); > > event = sctp_intl_order(ulpq, event); > > } > > } else { > > event = sctp_intl_reasm_uo(ulpq, event); > > + if (event) { > > + skb_queue_head_init(&temp); > > + __skb_queue_tail(&temp, sctp_event2skb(event)); > > + } > > } > > > > if (event) { > > event_eor = (event->msg_flags & MSG_EOR) ? 1 : 0; > > - sctp_enqueue_event(ulpq, event); > > + sctp_enqueue_event(ulpq, &temp); > > [1] can return an event without MSG_EOR (a partial delivery), which > would skip the condition on [2] and cause temp to not be initialized > by here. Same applies to sctp_ulpq_tail_data(). > I agree, it seems we canjust drop the msg_flags check and just key off of event being non-null, no? Neil > It's the only thing I noticed on the series. Will test it tomorrow. > > > } > > > > return event_eor; > ... >
On Thu, Feb 28, 2019 at 07:23:56AM -0500, Neil Horman wrote: > On Wed, Feb 27, 2019 at 11:19:58PM -0300, Marcelo Ricardo Leitner wrote: > > On Wed, Feb 27, 2019 at 05:00:24PM -0800, David Miller wrote: > > > > > > Pass this, instead of an event. Then everything trickles down and we > > > always have events a non-empty list. > > > > > > Then we needs a list creating stub to place into .enqueue_event for sctp_stream_interleave_1. > > > > > > Signed-off-by: David S. Miller <davem@davemloft.net> > > > --- > > > net/sctp/stream_interleave.c | 44 +++++++++++++++++++++++++++--------- > > > 1 file changed, 33 insertions(+), 11 deletions(-) > > > > > > diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c > > > index b6b251b8b3cf..0bc3d9329d9a 100644 > > > --- a/net/sctp/stream_interleave.c > > > +++ b/net/sctp/stream_interleave.c > > ... > > > @@ -866,11 +867,15 @@ static int sctp_ulpevent_idata(struct sctp_ulpq *ulpq, > > > > More context: > > if (!(event->msg_flags & SCTP_DATA_UNORDERED)) { > > event = sctp_intl_reasm(ulpq, event); [1] > > if (event && event->msg_flags & MSG_EOR) { [2] > > skb_queue_head_init(&temp); > > __skb_queue_tail(&temp, sctp_event2skb(event)); > > > > event = sctp_intl_order(ulpq, event); > > > } > > > } else { > > > event = sctp_intl_reasm_uo(ulpq, event); > > > + if (event) { > > > + skb_queue_head_init(&temp); > > > + __skb_queue_tail(&temp, sctp_event2skb(event)); > > > + } > > > } > > > > > > if (event) { > > > event_eor = (event->msg_flags & MSG_EOR) ? 1 : 0; > > > - sctp_enqueue_event(ulpq, event); > > > + sctp_enqueue_event(ulpq, &temp); > > > > [1] can return an event without MSG_EOR (a partial delivery), which > > would skip the condition on [2] and cause temp to not be initialized > > by here. Same applies to sctp_ulpq_tail_data(). > > > I agree, it seems we canjust drop the msg_flags check and just key off of event > being non-null, no? Yes. Like: if (!(event->msg_flags & SCTP_DATA_UNORDERED)) { event = sctp_intl_reasm(ulpq, event); if (event) { skb_queue_head_init(&temp); __skb_queue_tail(&temp, sctp_event2skb(event)); if (event->msg_flags & MSG_EOR) event = sctp_intl_order(ulpq, event); } Or some other clever way to handle it and reduce amount of the 'if (event)' checks that we have in there. Maybe 'if (!event) goto out;' helps. Marcelo > > Neil > > > It's the only thing I noticed on the series. Will test it tomorrow. > > > > > } > > > > > > return event_eor; > > ... > >
diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c index b6b251b8b3cf..0bc3d9329d9a 100644 --- a/net/sctp/stream_interleave.c +++ b/net/sctp/stream_interleave.c @@ -484,14 +484,15 @@ static struct sctp_ulpevent *sctp_intl_order(struct sctp_ulpq *ulpq, } static int sctp_enqueue_event(struct sctp_ulpq *ulpq, - struct sctp_ulpevent *event) + struct sk_buff_head *skb_list) { - struct sk_buff *skb = sctp_event2skb(event); struct sock *sk = ulpq->asoc->base.sk; struct sctp_sock *sp = sctp_sk(sk); - struct sk_buff_head *skb_list; + struct sctp_ulpevent *event; + struct sk_buff *skb; - skb_list = (struct sk_buff_head *)skb->prev; + skb = __skb_peek(skb_list); + event = sctp_skb2event(skb); if (sk->sk_shutdown & RCV_SHUTDOWN && (sk->sk_shutdown & SEND_SHUTDOWN || @@ -866,11 +867,15 @@ static int sctp_ulpevent_idata(struct sctp_ulpq *ulpq, } } else { event = sctp_intl_reasm_uo(ulpq, event); + if (event) { + skb_queue_head_init(&temp); + __skb_queue_tail(&temp, sctp_event2skb(event)); + } } if (event) { event_eor = (event->msg_flags & MSG_EOR) ? 1 : 0; - sctp_enqueue_event(ulpq, event); + sctp_enqueue_event(ulpq, &temp); } return event_eor; @@ -944,20 +949,27 @@ static struct sctp_ulpevent *sctp_intl_retrieve_first(struct sctp_ulpq *ulpq) static void sctp_intl_start_pd(struct sctp_ulpq *ulpq, gfp_t gfp) { struct sctp_ulpevent *event; + struct sk_buff_head temp; if (!skb_queue_empty(&ulpq->reasm)) { do { event = sctp_intl_retrieve_first(ulpq); - if (event) - sctp_enqueue_event(ulpq, event); + if (event) { + skb_queue_head_init(&temp); + __skb_queue_tail(&temp, sctp_event2skb(event)); + sctp_enqueue_event(ulpq, &temp); + } } while (event); } if (!skb_queue_empty(&ulpq->reasm_uo)) { do { event = sctp_intl_retrieve_first_uo(ulpq); - if (event) - sctp_enqueue_event(ulpq, event); + if (event) { + skb_queue_head_init(&temp); + __skb_queue_tail(&temp, sctp_event2skb(event)); + sctp_enqueue_event(ulpq, &temp); + } } while (event); } } @@ -1059,7 +1071,7 @@ static void sctp_intl_reap_ordered(struct sctp_ulpq *ulpq, __u16 sid) if (event) { sctp_intl_retrieve_ordered(ulpq, event); - sctp_enqueue_event(ulpq, event); + sctp_enqueue_event(ulpq, &temp); } } @@ -1326,6 +1338,16 @@ static struct sctp_stream_interleave sctp_stream_interleave_0 = { .handle_ftsn = sctp_handle_fwdtsn, }; +static int do_sctp_enqueue_event(struct sctp_ulpq *ulpq, + struct sctp_ulpevent *event) +{ + struct sk_buff_head temp; + + skb_queue_head_init(&temp); + __skb_queue_tail(&temp, sctp_event2skb(event)); + return sctp_enqueue_event(ulpq, &temp); +} + static struct sctp_stream_interleave sctp_stream_interleave_1 = { .data_chunk_len = sizeof(struct sctp_idata_chunk), .ftsn_chunk_len = sizeof(struct sctp_ifwdtsn_chunk), @@ -1334,7 +1356,7 @@ static struct sctp_stream_interleave sctp_stream_interleave_1 = { .assign_number = sctp_chunk_assign_mid, .validate_data = sctp_validate_idata, .ulpevent_data = sctp_ulpevent_idata, - .enqueue_event = sctp_enqueue_event, + .enqueue_event = do_sctp_enqueue_event, .renege_events = sctp_renege_events, .start_pd = sctp_intl_start_pd, .abort_pd = sctp_intl_abort_pd,
Pass this, instead of an event. Then everything trickles down and we always have events a non-empty list. Then we needs a list creating stub to place into .enqueue_event for sctp_stream_interleave_1. Signed-off-by: David S. Miller <davem@davemloft.net> --- net/sctp/stream_interleave.c | 44 +++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 11 deletions(-)