On Tue 08 Nov 01:36 PST 2016, Mian Yousaf Kaukab wrote:

On Mon, 2016-11-07 at 12:23 -0800, Bjorn Andersson wrote:

quoted

On Mon 07 Nov 09:27 PST 2016, Mian Yousaf Kaukab wrote:

[..]

quoted

I added some debugging prints to track the stack management in
depmod_report_cycles().

remoteproc, mdt_loader, wcnss_iris and wcnss_pil are all roots and we
have dependencies like this.

�������/---(rproc)
�������|�����^
�������|�����|
(pil) -+-> (mdt)
� ^����|
� |����v
� +- (iris)

AFAICT this is not correct. Dependencies are like following:

pil -> rproc
pil -> mtd -> rproc
pil -> iris -> pil

All dependencies are:

pil -> rproc
pil -> mdt
mdt -> rproc
pil -> iris
iris -> pil

The last two forming a cyclic subgraph in a dependency graph including
all of them.

quoted

1) We pick rproc as first root, mark that as visited and see that
we're
���done.

2) We pick mdt_loader as second root, we push remoteproc to the stack
���and find that it's already visited, so we found a loop!

�����mdt_loader -> remoteproc

This is not cyclic.

Exactly my point. But the DFS will find this cycle, as we don't reset
"visited" between the iterations in the loop.

quoted

3) We pick iris as third root, we push wcnss which pushes remoteproc,
���mdt_loader and iris. All three have already been visited from root
#1
���and #2, so we find that there's a loop in:

�����iris -> wcnss -> iris
�����iris -> wcnss -> mdt
�����iris -> wcnss -> remoteproc

Only one of these cases are actually a cycle, but as we don't reset
visited between the searches we can't tell. Further more, if there
was a
dependency from iris -> remoteproc that would have shown up earlier
and
marked iris->visited and when we get to step #3 we would just have
bailed directly - completely missing the cycle.

In case we have more than one cyclic dependencies around same modules?

Well in this case we have a mixture of cyclic and non-cyclic subgraphs
that the DFS hits due to the "visited" issue above, the non-cyclic ones
are what triggers the buffer overflow.

May be we should add a testcase for such a scenario so that its easy to
understand and reproduce.

+1

Thanks for looking at this!

Regards,
Bjorn