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

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

quoted

On Sat, 2016-11-05 at 21:50 -0200, Lucas De Marchi wrote:

quoted

Hi,

On Fri, Nov 4, 2016 at 7:33 AM, Mian Yousaf Kaukab
[off-list ref] wrote:

quoted

Only print actual cyclic dependencies. Don't print count of all
the
modules as it includes other modules which have dependencies
but
not
necessarily cyclic.

Printing related modules causes buffer overflow as m->modnamesz 
is
not
included in buffer size calculations (loop == m is never true).
This buffer overflow causes kmod to crash.

Reported-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Mian Yousaf Kaukab <redacted>
---
As count of modules in cyclic dependency chain is not known at
the
start of this function, so it is not printed anymore. The
output
when cyclic dependency is detected is changed as following:

Old:
depmod: ERROR: Found 8 modules in dependency cycles!

New:
depmod: ERROR: Modules found in dependency cycles!

I think it would be good to fix this problem, but retaining the
behavior.

Would it be OK if the modules names are printed first and count is
printed afterward. Something like following: 

DEPMOD  4.9.0-rc4-default
depmod: ERROR: Cycle detected: qcom_wcnss_iris -> qcom_wcnss ->
qcom_wcnss_iris
depmod: ERROR: Found 2 modules in dependency cycles!
Makefile:1227: recipe for target '_modinst_post' failed

In this way modules names can still be printed in the loop and then
the
exact count of modules involved in cyclic dependency is printed at
the
end of the depmod_report_cycles().

quoted

My first reaction to this is "how do I reproduce the issue?".
This
number so far does tell us how many modules are involved in
loops. 

There is more info at the following link:
https://bugzilla.opensuse.org/show_bug.cgi?id=1008186

You can reproduce the issue by enabling CONFIG_QCOM_WCNSS_PIL as
module
in v4.9-rc4 for arm64. 
+CONFIG_REMOTEPROC=m
+CONFIG_QCOM_MDT_LOADER=m
-# CONFIG_QCOM_WCNSS_PIL is not set
+CONFIG_QCOM_WCNSS_IRIS=m
+CONFIG_QCOM_WCNSS_PIL=m

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

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.

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?
May be we should add a testcase for such a scenario so that its easy to
understand and reproduce.

So I think we need to reset the visited list on each run of the DFS
from
each root.

Regards,
Bjorn

BR,
Yousaf