linux-kernel - Re: [lm-sensors] [patch 1/5] hwmon: Convert fschmd to unlocked

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: <20091106143103.2fca8368@hyperion.delvare>
Date:	Fri, 6 Nov 2009 14:31:03 +0100
From:	Jean Delvare <khali@...ux-fr.org>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	LKML <linux-kernel@...r.kernel.org>,
	Alan Cox <alan@...rguk.ukuu.org.uk>, lm-sensors@...sensors.org,
	Hans de Goede <hdegoede@...hat.com>
Subject: Re: [lm-sensors] [patch 1/5] hwmon: Convert fschmd to
 unlocked_ioctl

On Fri, 6 Nov 2009 14:22:49 +0100, Arnd Bergmann wrote:
> On Friday 06 November 2009, Jean Delvare wrote:
> > I'm not sure why we need to hold the mutex here? My understanding is
> > that watchdog_data_mutex protects watchdog_data_list and each
> > watchdog's kref. And the above code doesn't touch either.
> > 
> > What I am more worried about is why ident is declared static. This
> > looks like a bug to me. Instead of abusing watchdog_data_mutex to
> > workaround this, I'd rather remove the "static". I guess that the
> > current code happens to work because neither data->revision nor
> > nowayout can change over time, but this looks needlessly fragile.
> > 
> > Hans, any comment?
> 
> The data copied into the watchdog_info data structure is completely
> static. To make that clearer, I'd suggest moving it outside of
> the ioctl function, and initializing it from fschmd_init.
> Then it becomes obvious that the mutex is not needed to protect it.

Thinking about it some more... The driver supports the case where
several chips are present on the system. I don't think we've ever seen
this, but it is possible in theory. In this case, each chip would have
its data->revision, and then the watchdog_info data structure is no
longer static. We would need one watchdog_info data structure per
fschmd_data. Whether allocating and initializing them at device probing
time makes sense, I'm not so sure... I doubt this ioctl is
performance-critical, so generating the data on-the-fly sounds quite
sane to me.

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