Re: [PATCH] dma-buf: fix debugfs versus rcu and fence dumping

[Jerome Glisse]

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@xxxxxxxxxx:
> >>> From: Jérôme Glisse <jglisse@xxxxxxxxxx>
> >>>
> >>> 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 <jglisse@xxxxxxxxxx>
> >>> Cc: Christian König <christian.koenig@xxxxxxx>
> >>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> >>> Cc: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
> >>> Cc: linux-media@xxxxxxxxxxxxxxx
> >>> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> >>> Cc: linaro-mm-sig@xxxxxxxxxxxxxxxx
> >>> Cc: Stéphane Marchesin <marcheu@xxxxxxxxxxxx>
> >>> Cc: stable@xxxxxxxxxxxxxxx
> >> 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