Re: [PATCH] IPMI driver for Linux, version 6

[Arjan van de Ven <arjanv@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2679 bytes --]

On Mon, 2002-10-14 at 03:06, Corey Minyard wrote:
> Yet one more update, mostly fixes for stylistic things that Randy Dunlap 
> pointed out, and a bug fix that lets the KCS state machine get out of 
> the "hosed" state on the next message (buggy hardware can sometimes be 
> useful :-).  As usual, on my home page at  http://home.attbi.com/~minyard.
> 
> -Corey
+static int ipmi_open(struct inode *inode, struct file *file)
..
+	priv = kmalloc(sizeof(*priv), GFP_KERNEL);
..
+	MOD_INC_USE_COUNT;

hmm. Ok so you either need the MOD_INC_USE_COUNT or you don't, but if
you do it should go before the sleeping kmalloc ;)

+static int ipmi_ioctl(struct inode  *inode,
...
+		if (copy_from_user(&req, (void *) data, sizeof(req))) {
+			rv = -EFAULT;
+			break;
+		}
+
+		if (req.addr_len > sizeof(struct ipmi_addr))
+		{
+			rv = -EINVAL;
+			break;
+		}
+
+		if (copy_from_user(&addr, req.addr, req.addr_len)) {

since addr_len is a signed value, a user that passes -1 will get a
LOOOONG copy_from_user scribbling all over kernel memory...same for
data_len a few lines onwards

+static void sender(void                *send_info,
..
+	if (kcs_info->run_to_completion) {
+		/* If we are running to completion, then throw it in
+		   the list and run transactions until everything is
+		   clear.  Priority doesn't matter here. */
+		list_add_tail(&(msg->link), &(kcs_info->xmit_msgs));
+		result = kcs_event_handler(kcs_info, 0);
+		while (result != KCS_SM_IDLE) {
+			udelay(500);
+			result = kcs_event_handler(kcs_info, 500);
+		}

is that unconditional udelay with interrupts off really needed? 

+/* The locking for these for a write lock is done by two locks, first
+   the "outside" lock then the normal lock.  This way, the interfaces
+   lock can be converted to a read lock without allowing a new write
+   locker to come in.  Note that at interrupt level, this can only be
+   claimed read, so there is no reason for read lock to save
+   interrupts.  Write locks must still save interrupts because they
+   can block an interrupt. */

I get the feeling this locking is fishy. A double r/w lock smells bad.
really. Either you always take both the same way (eg both for read or
both for write) and then the inner lock could be a normal spinlock; or
you will deadlock in new and surprising ways....
 
+	spin_lock_irqsave(&interfaces_outside_lock, flags);
+	write_lock(&interfaces_lock);
+	for (i=0; i<MAX_IPMI_INTERFACES; i++) {
+		if (ipmi_interfaces[i] == NULL) {
+			new_intf = kmalloc(sizeof(*new_intf), GFP_KERNEL);

ehm ehm didn't just take TWO spinlocks 3 lines above this sleeping
function ?



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]