linux-kernel - Re: [HP ProLiant WatchDog driver] hpwdt HP WatchDog Patch <resend>

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: <20071104191133.GC2648@infomag.infomag.iguana.be>
Date:	Sun, 4 Nov 2007 20:11:33 +0100
From:	Wim Van Sebroeck <wim@...ana.be>
To:	thomas.mingarelli@...com
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [HP ProLiant WatchDog driver] hpwdt HP WatchDog Patch <resend>

Hi Thomas,

I reviewed yor code and have ome remarks:
* All watchdog device drivers should work with seconds. Your driver works
with minutes. Please change to seconds.
* The misc_register function (which opens your driver towards user-space)
should be the last iregister-action you take into the pci probe function.
(So just before you do the printk where you say that the driver has been
initialized).
* The open and release functions should make sure that /dev/watchdog can
only be opened once.
* /dev/watchdog is a VFS (Virtual File System) so your open function should
return with "return nonseekable_open(inode, file);".
* please put the code to start the watchdog in a seperate function (this
will make it easier to migrate your driver to the generic watchdog model
in the future).
* If you watchdog can't be stopped then you should use a timer to make sure
that when /dev/watchdog is closed, that you continue to keepalive/ping the
watchdog device. (please see mixcomwd.c (this device has the same problem)).
* please put the code to change the timeout in a seperate function.
* the default return value for the ioctl calls should be -ENOTTY (instead of
-ENOIOCTLCMD).
* the GETSTATUS call should also have a "put_user(0, (int *)arg);".
* if possible please add sparse testing code into the ioctl handling.
* Personnaly: I would rather see the generic smbios code somewhere in the
arch/x86/ directory. We will probably need that also in other (non-watchdog)
drivers in the future.

Greetings,
Wim.

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