linux-kernel - Re: [PATCH 1/2] Make the module 'usage' lists be two-way

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: <alpine.LFD.2.00.1005312041070.3637@i5.linux-foundation.org>
Date:	Mon, 31 May 2010 20:51:06 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Américo Wang <xiyou.wangcong@...il.com>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Rusty Russell <rusty@...tcorp.com.au>,
	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 1/2] Make the module 'usage' lists be two-way

On Tue, 1 Jun 2010, Américo Wang wrote:
> >+	list_for_each_entry_safe(use, tmp, &mod->target_list, target_list) {
> >+		struct module *i = use->target;
> >+		DEBUGP("%s unusing %s\n", mod->name, i->name);
> >+		module_put(i);
> >+		list_del(&use->source_list);
> >+		list_del(&use->target_list);
> >+		kfree(use);
> >+		sysfs_remove_link(i->holders_dir, mod->name);
> 
> I think it's nice to have a remove_module_usage() here, since we already
> have add_module_usage().

I agree that those five lines could easily be a function of their own. It 
would make sense and be symmetric. That said, I didn't do it because there 
was just a single use place, and it was so simple.

Also, if you do turn it into a function, it's a bit dubious what the 
proper calling convention would be. The clean interface would be to just 
pass in "use", since you can figure out both the source and target from 
there. At the same time, it's a bit sad to re-load "mod" from 
"use->source", when we had it explicitly and started from it.  But maybe 
that doesn't really matter.

One thing I react to now that I look at it again - I think we should also 
move the "module_put(i)" to the end. In fact I thought I did it when I 
moved the code around, but clearly I hadn't.

It won't really matter as-is (we hold the module_mutex, so "i" isn't going 
away), and it's what the old code used to do, but we should probably aim 
for more of a ref-counted worldview where you don't put the module until 
after it's not used again. And we clearly still use "i" after the 
module_put() above in the sysfs_remove_link thing. So that's a bit ugly 
too.

I suspect the module code could do with quite a bit of a cleanup. The 
primary target of that patch was a different issue, though.

		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/