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]

Message-Id: <201006021522.35784.rusty@rustcorp.com.au>
Date:	Wed, 2 Jun 2010 15:22:34 +0930
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
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>,
	Tim Abbott <tabbott@...lice.com>
Subject: Re: [PATCH 2/2] module: fix bne2 "gave up waiting for init of module libcrc32c"

On Wed, 2 Jun 2010 02:26:52 pm Linus Torvalds wrote:
> 
> On Wed, 2 Jun 2010, Rusty Russell wrote:
> > 
> > OK, this might be worthwhile.  Take the very first mail you sent:
> > 
> >    Hmm. That does seem to be buggy.  We can't just drop and re-take the lock: 
> >    that may make sense internally as far as resolve_symbol() itself is 
> >    concerned, but the caller will its own local variables, and some of those 
> >    will no longer be valid if the lock was dropped. 
> > 
> > The implication here is that that I don't know locking, and that this was
> > done without thought or care.  In fact, I did worry about it and decided it
> > was less risky than a locking reduction given my limited cycles (indeed,
> > it has been slightly).
> 
> Well, part of the context here is that the commit had been bisected as 
> being buggy. It turns out the bug was a different issue, but at the same 
> time, it very much looked like the locking was simply known a-priori to be 
> buggy. No?

Hmm, you're still missing it.  Let me try again.

You weren't the best person to make the call.  That didn't occur to you, did
it?

Subsystem maintainers aren't just patch monkeys, we collect all the metadata
which informs these decisions.  Without that, you wandered in, came to the
obvious conclusion, and were wrong.

Really grating is the arrogant lack of respect for that knowledge combined
with your multiple mistakes since then due to your lack of it.  See below.

> >    That commit also changes the return value semantics of "use_module()", 
> >    which is an exported interface where the only users seem to be 
> >    out-of-kernel (the only in-kernel use is in kernel/module.c itself). That 
> >    seems like a really really bad idea too.
> > 
> > Again with the implication that this was done without consideration.
> 
> No. The implication was that it's wrong to do and a bad idea. Anything 
> else is just you reading things into it.
> 
> It's an exported interface, and you changed it with no real reason. 
> Whether you then talked to users or not is immaterial.
> 
> You could easily have just changed the _internal_ thing, and exported the 
> unchanged interface.

I *genuinely* thought our policy with out-of-tree code was to smother
out-of-tree code with absolute apathy so they'll get sick of riding the
churn.

Still, easy to avoid in this case.

> Even your second version is just very confused. It renames it, but then 
> exports the renamed version. What does that help?

This *almost* looks like an honest "hey, I don't understand this".

But no, "very confused" already assumes you're right.  You do ask if it
helps...

> It still means that the  external module - for no good reason - needs to
> have basically a  source-level version number check.

...and go on to assume it doesn't.  I must be really confused, right?

Or maybe, I know something you don't about the code I maintain.

We have infrastructure to look up symbols dynamically: symbol_get().

Sure, they have to adapt their code, but now they can maintain their own
damn wrapper, *and* they'll discover the change as soon as they try to
build their module against the new kernel.

I'd go so far as to say it's clever...
Rusty.
--
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/