linux-kernel - Re: [PATCH] fix hwmon/i5k_amb.c sysfs attribute for lockdep

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] [day] [month] [year] [list]

Message-ID: <20100610090905.458b3fb9@hyperion.delvare>
Date:	Thu, 10 Jun 2010 09:09:05 +0200
From:	Jean Delvare <khali@...ux-fr.org>
To:	ebiederm@...ssion.com (Eric W. Biederman)
Cc:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
	lm-sensors@...sensors.org, mingo@...hat.com,
	"akpm\@linux-foundation.org" <akpm@...ux-foundation.org>,
	Jiri Kosina <jkosina@...e.cz>
Subject: Re: [PATCH] fix hwmon/i5k_amb.c sysfs attribute for lockdep

Hi Eric,

On Wed, 09 Jun 2010 12:54:58 -0700, Eric W. Biederman wrote:
> I would love to see all of the sysfs attributes be declared
> statically which is overwhelming the common practice for sysfs
> attributes.

I fear this isn't going to happen. If you look at the hwmon drivers
which create their attributes dynamically, you'll see that at least
some of them really have to: themselves receive the list of attributes
dynamically from the firmware or hardware. Switching to static
attributes would mean setting arbitrary limits and wasting a lot of
memory.

> And that 46 approaching 50 call sites just barely exceeds the number
> of call sites who have had lockdep problems fixed.
> 
> Furthermore it does make sense to have a special initialization helper
> for dynamically allocated sysfs attributes.

Sure it does make sense, we have the same mechanism for many other
structures. But when the initialization is only needed when debugging,
it becomes somewhat questionable.

> It doesn't really make sense to add sysfs_attr_init into
> device_create_file.  The vast majority of sysfs attributes are
> declared statically and thus get a very fine grained locking analysis,
> by virtue of each one having their very own lock_class_key.
> device_create_file is by and large used on those static files.  So
> we would likely reduce the quality of the lockdep coverage if we folded
> sysfs_attr_init into device_create_file.

What's wrong with calling sysfs_attr_init() automatically in
device_create_file() if and only if it has not been called explicitly
before? This would avoid all these per-driver patches. I understand
that this means that all dynamic attributes would share the same
lock_class_key, but I suspect this shouldn't be a problem in practice.
And this would avoid having to add calls to sysfs_attr_init() in many
drivers. Only drivers which need finer-grained classes AND use dynamic
attributes would have had to be touched. This might as well be 0 driver
in the end.

> Right now the burden of sysfs_attr_init is placed on drivers that do
> strange things with their sysfs files, and need to dynamically
> allocate them, which is maybe 5% of the sysfs files, that are not
> automatically generated.  Apparently the hwmon drivers seem to
> standout in that regard.  With two different drivers needing this
> treatment.

Five, actually... Three more need fixing (asc7621, abituguru and
abituguru3). Yes, hwmon drivers are a big consumer of sysfs attribute
files.

-- 
Jean Delvare
--
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/