From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Ricardo Leitner Subject: Re: [PATCH net] sctp: do sanity checks before migrating the asoc Date: Thu, 4 Feb 2016 07:47:16 -0200 Message-ID: <20160204094716.GB31743@localhost.localdomain> References: <10616913996c7a4cbe8a2bb23cf4e78fcfa0a13a.1452891824.git.marcelo.leitner@gmail.com> <569E45FD.4040801@gmail.com> <569E5D44.5000103@gmail.com> <569E8280.9080701@gmail.com> <569E8F2F.5070906@gmail.com> <569E94CF.4030409@gmail.com> <569E97D0.7050109@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Vlad Yasevich , netdev , linux-sctp@vger.kernel.org, Eric Dumazet , syzkaller , Kostya Serebryany , Alexander Potapenko , Sasha Levin To: Dmitry Vyukov Return-path: Received: from mx1.redhat.com ([209.132.183.28]:58076 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932829AbcBDJrT (ORCPT ); Thu, 4 Feb 2016 04:47:19 -0500 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Wed, Feb 03, 2016 at 05:13:25PM +0100, Dmitry Vyukov wrote: > On Tue, Jan 19, 2016 at 9:08 PM, Marcelo Ricardo Leitner > wrote: > > Em 19-01-2016 17:55, Vlad Yasevich escreveu: > >> > >> On 01/19/2016 02:31 PM, Marcelo Ricardo Leitner wrote: > >>> > >>> Em 19-01-2016 16:37, Vlad Yasevich escreveu: > >>>> > >>>> On 01/19/2016 10:59 AM, Marcelo Ricardo Leitner wrote: > >>>>> > >>>>> Yes, not thrilled here either about connect-to-self. > >>>>> > >>>>> But there is a big difference on how both works. For rx we can just > >>>>> look for wanted skbs > >>>>> in rx queue, as they aren't going anywhere, but for tx I don't think we > >>>>> can easily block > >>>>> sctp_wfree() call because that may be happening on another CPU (or am I > >>>>> mistaken here? > >>>>> sctp still doesn't have RFS but even irqbalance could affect this > >>>>> AFAICT) and more than > >>>>> one skb may be in transit at a time. > >>>> > >>>> > >>>> The way it's done now, we wouldn't have to block sctp_wfree. Chunks are > >>>> released under > >>>> lock when they are acked, so we are OK here. The tx completions will > >>>> just put 1 byte back > >>>> to the socket associated with the tx'ed skb, and that should still be ok > >>>> as > >>>> sctp_packet_release_owner will call sk_free(). > >>> > >>> > >>> Please let me rephrase it. I'm actually worried about the asoc->base.sk > >>> part of the story > >>> and how it's fetched in sctp_wfree(). I think we can update that sk > >>> pointer after > >>> sock_wfree() has fetched it but not used it yet, possibly leading to > >>> accounting it twice, > >>> one during migration and one on sock_wfree. > >>> In sock_wfree() it will update some sk stats like sk->sk_wmem_alloc, > >>> among others. > >> > >> > >> sctp_wfree() is only used on skbs that were created as sctp chunks to be > >> transmitted. > >> Right now, these skbs aren't actually submitted to the IP or to nic to be > >> transmitted. > >> They are queued at the association level (either in transports or in the > >> outqueue). > >> They are only freed during ACK processing. > >> > >> The ACK processing happens under a socket lock and thus asoc->base.sk can > >> not move. > >> > >> The migration process also happens under a socket lock. As a result, > >> during migration > >> we are guaranteed the chunk queues remain consistent and that > >> asoc->base.sk linkage > >> remains consistent. In fact, if you look at the sctp_sock_migrate, we > >> lock both > >> sockets when we reassign the assoc->base.sk so we know both sockets are > >> properly locked. > >> > >> So, I am not sure that what you are worried about can happen. Please feel > >> free to > >> double-check the above of course. > > > > > > Ohh, right. That makes sense. I'll rework the patch. Thanks Vlad. > > > Hi Marcelo, > > Any updates on this? I still see the leak. Hi Dmitry, No, not yet, and I'll be out for 3 weeks starting monday. So if I don't get it by sunday, it will be a while, sorry. Marcelo From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Ricardo Leitner Date: Thu, 04 Feb 2016 09:47:16 +0000 Subject: Re: [PATCH net] sctp: do sanity checks before migrating the asoc Message-Id: <20160204094716.GB31743@localhost.localdomain> List-Id: References: <10616913996c7a4cbe8a2bb23cf4e78fcfa0a13a.1452891824.git.marcelo.leitner@gmail.com> <569E45FD.4040801@gmail.com> <569E5D44.5000103@gmail.com> <569E8280.9080701@gmail.com> <569E8F2F.5070906@gmail.com> <569E94CF.4030409@gmail.com> <569E97D0.7050109@gmail.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Dmitry Vyukov Cc: Vlad Yasevich , netdev , linux-sctp@vger.kernel.org, Eric Dumazet , syzkaller , Kostya Serebryany , Alexander Potapenko , Sasha Levin On Wed, Feb 03, 2016 at 05:13:25PM +0100, Dmitry Vyukov wrote: > On Tue, Jan 19, 2016 at 9:08 PM, Marcelo Ricardo Leitner > wrote: > > Em 19-01-2016 17:55, Vlad Yasevich escreveu: > >> > >> On 01/19/2016 02:31 PM, Marcelo Ricardo Leitner wrote: > >>> > >>> Em 19-01-2016 16:37, Vlad Yasevich escreveu: > >>>> > >>>> On 01/19/2016 10:59 AM, Marcelo Ricardo Leitner wrote: > >>>>> > >>>>> Yes, not thrilled here either about connect-to-self. > >>>>> > >>>>> But there is a big difference on how both works. For rx we can just > >>>>> look for wanted skbs > >>>>> in rx queue, as they aren't going anywhere, but for tx I don't think we > >>>>> can easily block > >>>>> sctp_wfree() call because that may be happening on another CPU (or am I > >>>>> mistaken here? > >>>>> sctp still doesn't have RFS but even irqbalance could affect this > >>>>> AFAICT) and more than > >>>>> one skb may be in transit at a time. > >>>> > >>>> > >>>> The way it's done now, we wouldn't have to block sctp_wfree. Chunks are > >>>> released under > >>>> lock when they are acked, so we are OK here. The tx completions will > >>>> just put 1 byte back > >>>> to the socket associated with the tx'ed skb, and that should still be ok > >>>> as > >>>> sctp_packet_release_owner will call sk_free(). > >>> > >>> > >>> Please let me rephrase it. I'm actually worried about the asoc->base.sk > >>> part of the story > >>> and how it's fetched in sctp_wfree(). I think we can update that sk > >>> pointer after > >>> sock_wfree() has fetched it but not used it yet, possibly leading to > >>> accounting it twice, > >>> one during migration and one on sock_wfree. > >>> In sock_wfree() it will update some sk stats like sk->sk_wmem_alloc, > >>> among others. > >> > >> > >> sctp_wfree() is only used on skbs that were created as sctp chunks to be > >> transmitted. > >> Right now, these skbs aren't actually submitted to the IP or to nic to be > >> transmitted. > >> They are queued at the association level (either in transports or in the > >> outqueue). > >> They are only freed during ACK processing. > >> > >> The ACK processing happens under a socket lock and thus asoc->base.sk can > >> not move. > >> > >> The migration process also happens under a socket lock. As a result, > >> during migration > >> we are guaranteed the chunk queues remain consistent and that > >> asoc->base.sk linkage > >> remains consistent. In fact, if you look at the sctp_sock_migrate, we > >> lock both > >> sockets when we reassign the assoc->base.sk so we know both sockets are > >> properly locked. > >> > >> So, I am not sure that what you are worried about can happen. Please feel > >> free to > >> double-check the above of course. > > > > > > Ohh, right. That makes sense. I'll rework the patch. Thanks Vlad. > > > Hi Marcelo, > > Any updates on this? I still see the leak. Hi Dmitry, No, not yet, and I'll be out for 3 weeks starting monday. So if I don't get it by sunday, it will be a while, sorry. Marcelo