From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> To: Qiujun Huang <hqjagain@gmail.com> Cc: "David S. Miller" <davem@davemloft.net>, vyasevich@gmail.com, nhorman@tuxdriver.com, Jakub Kicinski <kuba@kernel.org>, linux-sctp@vger.kernel.org, netdev <netdev@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, anenbupt@gmail.com Subject: Re: [PATCH v3] sctp: fix refcount bug in sctp_wfree Date: Fri, 20 Mar 2020 22:33:17 -0300 [thread overview] Message-ID: <20200321013317.GF3756@localhost.localdomain> (raw) In-Reply-To: <CAJRQjofnZ5Oz-0PNMY6ojddg3MZc4v5UC5AJwi2CxLByDGaZhQ@mail.gmail.com> On Sat, Mar 21, 2020 at 09:23:54AM +0800, Qiujun Huang wrote: > On Sat, Mar 21, 2020 at 9:02 AM Marcelo Ricardo Leitner > <marcelo.leitner@gmail.com> wrote: > > > > On Sat, Mar 21, 2020 at 07:53:29AM +0800, Qiujun Huang wrote: > > ... > > > > > So, sctp_wfree was not called to destroy SKB) > > > > > > > > > > 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 > > > > > > > > The real fix is to fix the migration to the new socket, though the > > > > situation on which it is happening is still not clear. > > > > > > > > The 2nd sendto() call on the reproducer is sending 212992 bytes on a > > > > single call. That's usually the whole sndbuf size, and will cause > > > > fragmentation to happen. That means the datamsg will contain several > > > > skbs. But still, the sacked chunks should be freed if needed while the > > > > remaining ones will be left on the queues that they are. > > > > > > in sctp_sendmsg_to_asoc > > > datamsg holds his chunk result in that the sacked chunks can't be freed > > > > Right! Now I see it, thanks. > > In the end, it's not a locking race condition. It's just not iterating > > on the lists properly. > > > > > > > > list_for_each_entry(chunk, &datamsg->chunks, frag_list) { > > > sctp_chunk_hold(chunk); > > > sctp_set_owner_w(chunk); > > > chunk->transport = transport; > > > } > > > > > > any ideas to handle it? > > > > sctp_for_each_tx_datachunk() needs to be aware of this situation. > > Instead of iterating directly/only over the chunk list, it should > > iterate over the datamsgs instead. Something like the below (just > > compile tested). > > > > Then, the old socket will be free to die regardless of the new one. > > Otherwise, if this association gets stuck on retransmissions or so, > > the old socket would not be freed till then. > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > index fed26a1e9518..85c742310d26 100644 > > --- a/net/sctp/socket.c > > +++ b/net/sctp/socket.c > > @@ -151,9 +151,10 @@ static void sctp_for_each_tx_datachunk(struct sctp_association *asoc, > > void (*cb)(struct sctp_chunk *)) > > > > { > > + struct sctp_datamsg *msg, *prev_msg = NULL; > > struct sctp_outq *q = &asoc->outqueue; > > struct sctp_transport *t; > > - struct sctp_chunk *chunk; > > + struct sctp_chunk *chunk, *c; I missed to swap some lines here, for reverse christmass-tree style, btw. > > > > list_for_each_entry(t, &asoc->peer.transport_addr_list, transports) > > list_for_each_entry(chunk, &t->transmitted, transmitted_list) > > @@ -162,8 +163,14 @@ static void sctp_for_each_tx_datachunk(struct sctp_association *asoc, > > list_for_each_entry(chunk, &q->retransmit, transmitted_list) > > cb(chunk); > > > > - list_for_each_entry(chunk, &q->sacked, transmitted_list) > > - cb(chunk); > > + list_for_each_entry(chunk, &q->sacked, transmitted_list) { > > + msg = chunk->msg; > > + if (msg == prev_msg) > > + continue; > > + list_for_each_entry(c, &msg->chunks, frag_list) > > + cb(c); > > + prev_msg = msg; > > + } > > great. I'll trigger a syzbot test. Thanks. Mind that it may need to handled on the other lists as well. I didn't check them :] > > > > > list_for_each_entry(chunk, &q->abandoned, transmitted_list) > > cb(chunk);
WARNING: multiple messages have this Message-ID (diff)
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> To: Qiujun Huang <hqjagain@gmail.com> Cc: "David S. Miller" <davem@davemloft.net>, vyasevich@gmail.com, nhorman@tuxdriver.com, Jakub Kicinski <kuba@kernel.org>, linux-sctp@vger.kernel.org, netdev <netdev@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, anenbupt@gmail.com Subject: Re: [PATCH v3] sctp: fix refcount bug in sctp_wfree Date: Sat, 21 Mar 2020 01:33:17 +0000 [thread overview] Message-ID: <20200321013317.GF3756@localhost.localdomain> (raw) In-Reply-To: <CAJRQjofnZ5Oz-0PNMY6ojddg3MZc4v5UC5AJwi2CxLByDGaZhQ@mail.gmail.com> On Sat, Mar 21, 2020 at 09:23:54AM +0800, Qiujun Huang wrote: > On Sat, Mar 21, 2020 at 9:02 AM Marcelo Ricardo Leitner > <marcelo.leitner@gmail.com> wrote: > > > > On Sat, Mar 21, 2020 at 07:53:29AM +0800, Qiujun Huang wrote: > > ... > > > > > So, sctp_wfree was not called to destroy SKB) > > > > > > > > > > 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 > > > > > > > > The real fix is to fix the migration to the new socket, though the > > > > situation on which it is happening is still not clear. > > > > > > > > The 2nd sendto() call on the reproducer is sending 212992 bytes on a > > > > single call. That's usually the whole sndbuf size, and will cause > > > > fragmentation to happen. That means the datamsg will contain several > > > > skbs. But still, the sacked chunks should be freed if needed while the > > > > remaining ones will be left on the queues that they are. > > > > > > in sctp_sendmsg_to_asoc > > > datamsg holds his chunk result in that the sacked chunks can't be freed > > > > Right! Now I see it, thanks. > > In the end, it's not a locking race condition. It's just not iterating > > on the lists properly. > > > > > > > > list_for_each_entry(chunk, &datamsg->chunks, frag_list) { > > > sctp_chunk_hold(chunk); > > > sctp_set_owner_w(chunk); > > > chunk->transport = transport; > > > } > > > > > > any ideas to handle it? > > > > sctp_for_each_tx_datachunk() needs to be aware of this situation. > > Instead of iterating directly/only over the chunk list, it should > > iterate over the datamsgs instead. Something like the below (just > > compile tested). > > > > Then, the old socket will be free to die regardless of the new one. > > Otherwise, if this association gets stuck on retransmissions or so, > > the old socket would not be freed till then. > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > index fed26a1e9518..85c742310d26 100644 > > --- a/net/sctp/socket.c > > +++ b/net/sctp/socket.c > > @@ -151,9 +151,10 @@ static void sctp_for_each_tx_datachunk(struct sctp_association *asoc, > > void (*cb)(struct sctp_chunk *)) > > > > { > > + struct sctp_datamsg *msg, *prev_msg = NULL; > > struct sctp_outq *q = &asoc->outqueue; > > struct sctp_transport *t; > > - struct sctp_chunk *chunk; > > + struct sctp_chunk *chunk, *c; I missed to swap some lines here, for reverse christmass-tree style, btw. > > > > list_for_each_entry(t, &asoc->peer.transport_addr_list, transports) > > list_for_each_entry(chunk, &t->transmitted, transmitted_list) > > @@ -162,8 +163,14 @@ static void sctp_for_each_tx_datachunk(struct sctp_association *asoc, > > list_for_each_entry(chunk, &q->retransmit, transmitted_list) > > cb(chunk); > > > > - list_for_each_entry(chunk, &q->sacked, transmitted_list) > > - cb(chunk); > > + list_for_each_entry(chunk, &q->sacked, transmitted_list) { > > + msg = chunk->msg; > > + if (msg = prev_msg) > > + continue; > > + list_for_each_entry(c, &msg->chunks, frag_list) > > + cb(c); > > + prev_msg = msg; > > + } > > great. I'll trigger a syzbot test. Thanks. Mind that it may need to handled on the other lists as well. I didn't check them :] > > > > > list_for_each_entry(chunk, &q->abandoned, transmitted_list) > > cb(chunk);
next prev parent reply other threads:[~2020-03-21 1:33 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-03-20 11:09 [PATCH v3] sctp: fix refcount bug in sctp_wfree Qiujun Huang 2020-03-20 11:09 ` Qiujun Huang 2020-03-20 17:10 ` Eric Dumazet 2020-03-20 17:10 ` Eric Dumazet 2020-03-20 23:36 ` Qiujun Huang 2020-03-20 23:36 ` Qiujun Huang 2020-03-20 18:52 ` Marcelo Ricardo Leitner 2020-03-20 18:52 ` Marcelo Ricardo Leitner 2020-03-20 23:48 ` Qiujun Huang 2020-03-20 23:48 ` Qiujun Huang 2020-03-20 23:53 ` Qiujun Huang 2020-03-20 23:53 ` Qiujun Huang 2020-03-21 1:02 ` Marcelo Ricardo Leitner 2020-03-21 1:02 ` Marcelo Ricardo Leitner 2020-03-21 1:23 ` Qiujun Huang 2020-03-21 1:23 ` Qiujun Huang 2020-03-21 1:33 ` Marcelo Ricardo Leitner [this message] 2020-03-21 1:33 ` Marcelo Ricardo Leitner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200321013317.GF3756@localhost.localdomain \ --to=marcelo.leitner@gmail.com \ --cc=anenbupt@gmail.com \ --cc=davem@davemloft.net \ --cc=hqjagain@gmail.com \ --cc=kuba@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-sctp@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=nhorman@tuxdriver.com \ --cc=vyasevich@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.