From: Qiujun Huang <[email protected]>
[ Upstream commit 5c3e82fe159622e46e91458c1a6509c321a62820 ]
We should iterate over the datamsgs to move
all chunks(skbs) to newsk.
The following case cause the bug:
for the trouble SKB, it was in outq->transmitted list
sctp_outq_sack
sctp_check_transmitted
SKB was moved to outq->sacked list
then throw away the sack queue
SKB was deleted from outq->sacked
(but it was held by datamsg at sctp_datamsg_to_asoc
So, sctp_wfree was not called here)
then migrate happened
sctp_for_each_tx_datachunk(
sctp_clear_owner_w);
sctp_assoc_migrate();
sctp_for_each_tx_datachunk(
sctp_set_owner_w);
SKB was not in the outq, and was not changed to newsk
finally
__sctp_outq_teardown
sctp_chunk_put (for another skb)
sctp_datamsg_put
__kfree_skb(msg->frag_list)
sctp_wfree (for SKB)
SKB->sk was still oldsk (skb->sk != asoc->base.sk).
Reported-and-tested-by: [email protected]
Signed-off-by: Qiujun Huang <[email protected]>
Acked-by: Marcelo Ricardo Leitner <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
net/sctp/socket.c | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -165,29 +165,44 @@ static void sctp_clear_owner_w(struct sc
skb_orphan(chunk->skb);
}
+#define traverse_and_process() \
+do { \
+ msg = chunk->msg; \
+ if (msg == prev_msg) \
+ continue; \
+ list_for_each_entry(c, &msg->chunks, frag_list) { \
+ if ((clear && asoc->base.sk == c->skb->sk) || \
+ (!clear && asoc->base.sk != c->skb->sk)) \
+ cb(c); \
+ } \
+ prev_msg = msg; \
+} while (0)
+
static void sctp_for_each_tx_datachunk(struct sctp_association *asoc,
+ bool clear,
void (*cb)(struct sctp_chunk *))
{
+ struct sctp_datamsg *msg, *prev_msg = NULL;
struct sctp_outq *q = &asoc->outqueue;
+ struct sctp_chunk *chunk, *c;
struct sctp_transport *t;
- struct sctp_chunk *chunk;
list_for_each_entry(t, &asoc->peer.transport_addr_list, transports)
list_for_each_entry(chunk, &t->transmitted, transmitted_list)
- cb(chunk);
+ traverse_and_process();
list_for_each_entry(chunk, &q->retransmit, transmitted_list)
- cb(chunk);
+ traverse_and_process();
list_for_each_entry(chunk, &q->sacked, transmitted_list)
- cb(chunk);
+ traverse_and_process();
list_for_each_entry(chunk, &q->abandoned, transmitted_list)
- cb(chunk);
+ traverse_and_process();
list_for_each_entry(chunk, &q->out_chunk_list, list)
- cb(chunk);
+ traverse_and_process();
}
static void sctp_for_each_rx_skb(struct sctp_association *asoc, struct sock *sk,
@@ -8899,9 +8914,9 @@ static void sctp_sock_migrate(struct soc
* paths won't try to lock it and then oldsk.
*/
lock_sock_nested(newsk, SINGLE_DEPTH_NESTING);
- sctp_for_each_tx_datachunk(assoc, sctp_clear_owner_w);
+ sctp_for_each_tx_datachunk(assoc, true, sctp_clear_owner_w);
sctp_assoc_migrate(assoc, newsk);
- sctp_for_each_tx_datachunk(assoc, sctp_set_owner_w);
+ sctp_for_each_tx_datachunk(assoc, false, sctp_set_owner_w);
/* If the association on the newsk is already closed before accept()
* is called, set RCV_SHUTDOWN flag.
Hi!
> The following case cause the bug:
> for the trouble SKB, it was in outq->transmitted list
Ok... but this is one hell of "interesting" code.
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -165,29 +165,44 @@ static void sctp_clear_owner_w(struct sc
> skb_orphan(chunk->skb);
> }
>
> +#define traverse_and_process() \
> +do { \
> + msg = chunk->msg; \
> + if (msg == prev_msg) \
> + continue; \
> + list_for_each_entry(c, &msg->chunks, frag_list) { \
> + if ((clear && asoc->base.sk == c->skb->sk) || \
> + (!clear && asoc->base.sk != c->skb->sk)) \
> + cb(c); \
> + } \
> + prev_msg = msg; \
> +} while (0)
Should this be function?
Do you see how it does "continue"? But the do {} while(0) wrapper eats
the continue. "break" would be more clear here, but they are really
equivalent due to way this macro is used.
It is just very, very confusing.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sat, Apr 11, 2020 at 08:28:13PM +0200, Pavel Machek wrote:
> Hi!
>
> > The following case cause the bug:
> > for the trouble SKB, it was in outq->transmitted list
>
> Ok... but this is one hell of "interesting" code.
>
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -165,29 +165,44 @@ static void sctp_clear_owner_w(struct sc
> > skb_orphan(chunk->skb);
> > }
> >
> > +#define traverse_and_process() \
> > +do { \
> > + msg = chunk->msg; \
> > + if (msg == prev_msg) \
> > + continue; \
> > + list_for_each_entry(c, &msg->chunks, frag_list) { \
> > + if ((clear && asoc->base.sk == c->skb->sk) || \
> > + (!clear && asoc->base.sk != c->skb->sk)) \
> > + cb(c); \
> > + } \
> > + prev_msg = msg; \
> > +} while (0)
>
> Should this be function?
>
> Do you see how it does "continue"? But the do {} while(0) wrapper eats
> the continue. "break" would be more clear here, but they are really
> equivalent due to way this macro is used.
>
> It is just very, very confusing.
Agree. The 'continue' itself slipped in on a refactoring and I didn't
notice the confusing aspect of it. Initially, the code was written
without the macro, and the continue refererred to the outter
list_for_each_entry().
Marcelo