On Wed, Dec 14, 2016 at 05:53:00PM +0000, Kieran Bingham wrote:
> Hi Sakari,
>
> On 14/12/16 12:43, Sakari Ailus wrote:
> > Hi Kieran,
> >
> > On Wed, Dec 14, 2016 at 12:27:37PM +0000, Kieran Bingham wrote:
> >> Hi Sakari,
> >>
> >> On 14/12/16 07:28, Sakari Ailus wrote:
> >>> Hi Kieran,
> >>>
> >>> On Tue, Dec 13, 2016 at 05:59:44PM +0000, Kieran Bingham wrote:
> >>>> media_entity_pipeline_stop() can be called through error paths with a
> >>>> NULL entity pipe object. In this instance, stopping is a no-op, so
> >>>> simply return without any action
> >>>
> >>> The approach of returning silently is wrong; the streaming count is
> >>> indeed a
> >>> count: you have to decrement it the exactly same number of times it's been
> >>> increased. Code that attempts to call __media_entity_pipeline_stop() on an
> >>> entity that's not streaming is simply buggy.
> >>
> >> Ok, Thanks for the heads up on where to look, as I suspected we are in
> >> section B) of my comments below :D
> >>
> >>> I've got a patch here that adds a warning to graph traversal on streaming
> >>> count being zero. I sent a pull request including that some days ago:
> >>>
> >>> <URL:http://www.spinics.net/lists/linux-media/msg108980.html>
> >>> <URL:http://www.spinics.net/lists/linux-media/msg108995.html>
> >>
> >> Excellent, thanks, I've merged your branch into mine, and I'll
> >> investigate where the error is actually coming from.
> >>
> >> --
> >> Ok - so I've merged your patches, (and dropped this patch) but they
> >> don't fire any warning before I hit my null-pointer dereference in
> >> __media_pipeline_stop(), on the WARN_ON(!pipe->streaming_count);
> >>
> >> So I think this is a case of calling stop on an invalid entity rather
> >> than an unbalanced start/stop somehow:
> >
> > Not necessarily. The pipe is set to NULL if the count goes to zero.
> >
> > This check should be dropped, it's ill-placed anyway: if streaming count is
> > zero the pipe is expected to be NULL anyway in which case you get a NULL
> > pointer exception (that you saw there). With the check removed, you should
> > get the warning instead.
>
> Ahh, yes, I'd missed the part there that was setting it to NULL.
>
> However, it will still segfault ... (though hopefully after the warning)
> as the last line of the function states:
>
> if (!--pipe->streaming_count)
> media_entity_graph_walk_cleanup(graph);
>
> So we will hit a null deref on the pipe->streaming_count there, which
> still leaves an unbalanced stop as a kernel panic.
Right, indeed. The graph is part of the pipeline, so if pipe was NULL you
don't have graph either. How about changing the check to:
if (WARN_ON(!pipe))
return;
Instead of removing it. Nothing in that function really makes sense if
there's no pipeline. The driver bug still exists. That's just to make the
framework behaving a little bit better in the presence of that sort of bug.
>
> In fact, I've just tested removing that WARN_ON(!pipe->streaming_count);
> - it doesn't even get that far - and segfaults in
>
> media_entity_graph_walk_cleanup(graph);
>
> [ 80.916558] Unable to handle kernel NULL pointer dereference at
> virtual address 00000110
> ....
> [ 81.769492] [<ffffff800853e278>] media_graph_walk_start+0x20/0xf0
> [ 81.776615] [<ffffff800853e73c>] __media_pipeline_stop+0x3c/0xd8
> [ 81.783644] [<ffffff800853e80c>] media_pipeline_stop+0x34/0x48
> [ 81.790505] [<ffffff8008567ff0>] vsp1_du_setup_lif+0x68/0x5a8
> [ 81.797279] [<ffffff80084de9d4>] rcar_du_vsp_disable+0x2c/0x38
> [ 81.804137] [<ffffff80084da710>] rcar_du_crtc_stop+0x198/0x1e8
> [ 81.810984] [<ffffff80084da780>] rcar_du_crtc_disable+0x20/0x38
>
> due to the fact that 'graph' is dereferenced through the 'pipe'.
>
> {
> /* Entity->pipe is NULL, therefore 'graph' is invalid */
> struct media_graph *graph = &entity->pipe->graph;
> struct media_pipeline *pipe = entity->pipe;
>
> media_graph_walk_start(graph, entity);
> ...
> }
>
> So I suspect we do still need a warning or check in there somewhere.
> Something to tell the developer that there is an unbalanced stop, would
> be much better than a panic/segfault...
>
> (And of course, we still need to look at the actual unbalanced stop in
> vsp1_du_setup_lif!)
Indeed.
>
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
