linux-kernel - Re: [PATCH 2/2] module: fix bne2 "gave up waiting for init of module libcrc32c"

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]

Date:	Mon, 31 May 2010 20:40:36 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Rusty Russell <rusty@...tcorp.com.au>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Brandon Philips <brandon@...p.org>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	LKML <linux-kernel@...r.kernel.org>,
	Jon Masters <jonathan@...masters.org>,
	Tejun Heo <htejun@...il.com>,
	Masami Hiramatsu <mhiramat@...hat.com>,
	Kay Sievers <kay.sievers@...y.org>
Subject: Re: [PATCH 2/2] module: fix bne2 "gave up waiting for init of module
 libcrc32c"

On Tue, 1 Jun 2010, Rusty Russell wrote:

> On Tue, 1 Jun 2010 05:45:37 am Linus Torvalds wrote:
> > 
> > basically, we should do the whole module dependency list regardless 
> > of whether we can unload modules or not 
> 
> Why?

Because the current non-CONFIG_MODULE_UNLOAD code is currently broken, and 
it was broken exactly because the code had two totally different paths and 
totally different logic. And one part simply missed the case.

We'd be much better off having as much of the logic shared as possible. 
No?

Your 2/2 actually fixed that, because it moved the broken 
wait_event_interruptible_timeout() out of the (non-shared) use_module() 
into the (shared) resolve_symbol_wait(). But even that seemed to be almost 
accidental, and seemed to be more about the fact that now the locking 
rules required it (if you wanted to wait without holding the lock), rather 
than anything else.

In contrast, wouldn't it be nice if we just made the waiting be a separate 
event entirely? And have the !MODULE_UNLOAD case also able to share the 
/proc/module format, for example? 

You do realize - although maybe you don't - that right now, even with your 
2/2 case, the !MODULE_UNLOAD case seems to get the module ref-counts 
wrong. I didn't look through it all, but it _seems_ to increment the 
ref-count for every symbol it resolves. 

What happens if the module load fails in the middle (say, because some 
symbols referred to module 'a' that was loaded successfully, and others 
refer to module 'b' that had some probelsm)? I think the !MODULE_UNLOAD 
case is _broken_ again.

Why?

Take a small guess. It's broken - even with your 2/2 - exactly because the 
!MODULE_UNLOAD case does something fundamentally different from the 
regular case. It increments the module counts for each symbol it looks up, 
exactly BECAUSE IT DOESN'T TRACK MODULE DEPENDENCIES!

Which means that it then cannot clean up properly afterwards if an error 
happens in the middle.

So I'd suggest we should just track those module dependencies, and share 
more of the code and the logic. Because it looks to me like not sharing it 
continually results in bugs.

No?

			Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/