From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerome Glisse Subject: Re: [PATCH] dma-buf: fix debugfs versus rcu and fence dumping Date: Thu, 6 Dec 2018 11:19:51 -0500 Message-ID: <20181206161950.GB3544@redhat.com> References: <20181206014103.1364-1-jglisse@redhat.com> <20181206152116.GA3544@redhat.com> <4e79bf05-864e-f3ca-194c-40c4504e472a@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <4e79bf05-864e-f3ca-194c-40c4504e472a@amd.com> Sender: linux-kernel-owner@vger.kernel.org To: "Koenig, Christian" Cc: "linux-kernel@vger.kernel.org" , Daniel Vetter , Sumit Semwal , "linux-media@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "linaro-mm-sig@lists.linaro.org" , =?iso-8859-1?Q?St=E9phane?= Marchesin , "stable@vger.kernel.org" List-Id: dri-devel@lists.freedesktop.org On Thu, Dec 06, 2018 at 04:08:12PM +0000, Koenig, Christian wrote: > Am 06.12.18 um 16:21 schrieb Jerome Glisse: > > On Thu, Dec 06, 2018 at 08:09:28AM +0000, Koenig, Christian wrote: > >> Am 06.12.18 um 02:41 schrieb jglisse@redhat.com: > >>> From: Jérôme Glisse > >>> > >>> The debugfs take reference on fence without dropping them. Also the > >>> rcu section are not well balance. Fix all that ... > >>> > >>> Signed-off-by: Jérôme Glisse > >>> Cc: Christian König > >>> Cc: Daniel Vetter > >>> Cc: Sumit Semwal > >>> Cc: linux-media@vger.kernel.org > >>> Cc: dri-devel@lists.freedesktop.org > >>> Cc: linaro-mm-sig@lists.linaro.org > >>> Cc: Stéphane Marchesin > >>> Cc: stable@vger.kernel.org > >> Well NAK, you are now taking the RCU lock twice and dropping the RCU and > >> still accessing fobj has a huge potential for accessing freed up memory. > >> > >> The only correct thing I can see here is to grab a reference to the > >> fence before printing any info on it, > >> Christian. > > Hu ? That is exactly what i am doing, take reference under rcu, > > rcu_unlock print the fence info, drop the fence reference, rcu > > lock rinse and repeat ... > > > > Note that the fobj in _existing_ code is access outside the rcu > > end that there is an rcu imbalance in that code ie a lonlely > > rcu_unlock after the for loop. > > > > So that the existing code is broken. > > No, the existing code is perfectly fine. > > Please note the break in the loop before the rcu_unlock(); > > if (!read_seqcount_retry(&robj->seq, seq)) > > break; <- HERE! > > rcu_read_unlock(); > > } > > So your patch breaks that and take the RCU read lock twice. Ok missed that, i wonder if the refcount in balance explains the crash that was reported to me ... i sent a patch just for that. Thank you for reviewing and pointing out the code i was oblivious too :) Cheers, Jérôme From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:57004 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725945AbeLFQTy (ORCPT ); Thu, 6 Dec 2018 11:19:54 -0500 Date: Thu, 6 Dec 2018 11:19:51 -0500 From: Jerome Glisse To: "Koenig, Christian" Cc: "linux-kernel@vger.kernel.org" , Daniel Vetter , Sumit Semwal , "linux-media@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "linaro-mm-sig@lists.linaro.org" , =?iso-8859-1?Q?St=E9phane?= Marchesin , "stable@vger.kernel.org" Subject: Re: [PATCH] dma-buf: fix debugfs versus rcu and fence dumping Message-ID: <20181206161950.GB3544@redhat.com> References: <20181206014103.1364-1-jglisse@redhat.com> <20181206152116.GA3544@redhat.com> <4e79bf05-864e-f3ca-194c-40c4504e472a@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4e79bf05-864e-f3ca-194c-40c4504e472a@amd.com> Sender: stable-owner@vger.kernel.org List-ID: On Thu, Dec 06, 2018 at 04:08:12PM +0000, Koenig, Christian wrote: > Am 06.12.18 um 16:21 schrieb Jerome Glisse: > > On Thu, Dec 06, 2018 at 08:09:28AM +0000, Koenig, Christian wrote: > >> Am 06.12.18 um 02:41 schrieb jglisse@redhat.com: > >>> From: J�r�me Glisse > >>> > >>> The debugfs take reference on fence without dropping them. Also the > >>> rcu section are not well balance. Fix all that ... > >>> > >>> Signed-off-by: J�r�me Glisse > >>> Cc: Christian K�nig > >>> Cc: Daniel Vetter > >>> Cc: Sumit Semwal > >>> Cc: linux-media@vger.kernel.org > >>> Cc: dri-devel@lists.freedesktop.org > >>> Cc: linaro-mm-sig@lists.linaro.org > >>> Cc: St�phane Marchesin > >>> Cc: stable@vger.kernel.org > >> Well NAK, you are now taking the RCU lock twice and dropping the RCU and > >> still accessing fobj has a huge potential for accessing freed up memory. > >> > >> The only correct thing I can see here is to grab a reference to the > >> fence before printing any info on it, > >> Christian. > > Hu ? That is exactly what i am doing, take reference under rcu, > > rcu_unlock print the fence info, drop the fence reference, rcu > > lock rinse and repeat ... > > > > Note that the fobj in _existing_ code is access outside the rcu > > end that there is an rcu imbalance in that code ie a lonlely > > rcu_unlock after the for loop. > > > > So that the existing code is broken. > > No, the existing code is perfectly fine. > > Please note the break in the loop before the rcu_unlock(); > > if (!read_seqcount_retry(&robj->seq, seq)) > > break; <- HERE! > > rcu_read_unlock(); > > } > > So your patch breaks that and take the RCU read lock twice. Ok missed that, i wonder if the refcount in balance explains the crash that was reported to me ... i sent a patch just for that. Thank you for reviewing and pointing out the code i was oblivious too :) Cheers, J�r�me