linux-kernel - Re: [RFC 26/29] dma-buf/fence: remove pointless fence_timeline

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160115180207.GD2664@joana>
Date:	Fri, 15 Jan 2016 16:02:07 -0200
From:	Gustavo Padovan <gustavo.padovan@...labora.co.uk>
To:	John Harrison <John.C.Harrison@...el.com>
Cc:	Gustavo Padovan <gustavo@...ovan.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, devel@...verdev.osuosl.org,
	dri-devel@...ts.freedesktop.org, daniels@...labora.com,
	Arve Hjønnevåg <arve@...roid.com>,
	Riley Andrews <riandrews@...roid.com>,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	Rob Clark <robdclark@...il.com>,
	Greg Hackmann <ghackmann@...gle.com>,
	Maarten Lankhorst <maarten.lankhorst@...onical.com>
Subject: Re: [RFC 26/29] dma-buf/fence: remove pointless
 fence_timeline_signal at destroy phase

2016-01-15 John Harrison <John.C.Harrison@...el.com>:

> On 15/01/2016 14:55, Gustavo Padovan wrote:
> >From: Gustavo Padovan <gustavo.padovan@...labora.co.uk>
> >
> >All changes to timeline value come through the user via
> >fence_timeline_signal() calls. When fence_timeline_destroy() is called no
> >changes on timeline->value happens hence call fence_timeline_signal() with
> >no increment is pointless.
> >
> >Signed-off-by: Gustavo Padovan <gustavo.padovan@...labora.co.uk>
> >---
> >  drivers/dma-buf/fence.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >
> >diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
> >index 7a5fc9b..26f5f0f 100644
> >--- a/drivers/dma-buf/fence.c
> >+++ b/drivers/dma-buf/fence.c
> >@@ -136,7 +136,7 @@ EXPORT_SYMBOL(fence_timeline_put);
> >   * fence_timeline_destroy - destroy a fence_timeline
> >   * @timeline	[in]	the fence_timeline to destroy
> >   *
> >- * This function destroys a timeline. It signals any active fence first.
> >+ * This function destroys a timeline.
> 
> The implementation for this was certainly broken but I would say it should
> be fixed to match the comment rather than just abandoned completely. That
> is, what happens if a timeline owner destroys their timeline while there are
> outstanding fences which other drivers are waiting on? That is presumably a
> bug in the code that called destroy prematurely, but bugs happen.
> 
> The old implementation simply leaked the fences. Doing a debugfs dump would
> show the timeline with all its outstanding fences still floating around
> forever after. Worse, anything waiting on them would never be signalled and
> is therefore potentially deadlocked.
> 
> Note that I haven't had chance to look through the entire patch series yet
> so maybe this has been fixed up elsewhere. If not, then I think it
> definitely needs looking into.
> 

Patches 27 and 28 are attempt to fix that. I assumed that if some code is
calling fence_timeline_destroy() it wants to stop everything so I
worked on a solution that stops any waiter and allows the timeline to be
destroyed.

No one is using fence_timeline_destroy() in mainline now, so it is
definately a behaviour we can discuss.

	Gustavo