Re: [PATCH] IPMI driver updates, part 1b

[Andrew Morton]

Corey Minyard <minyard@xxxxxxx> wrote:
>
> >diff -puN net/ipmi/af_ipmi.c~af_ipmi-locking-fix net/ipmi/af_ipmi.c
>  >--- 25/net/ipmi/af_ipmi.c~af_ipmi-locking-fix	Tue Feb 24 16:56:36 2004
>  >+++ 25-akpm/net/ipmi/af_ipmi.c	Tue Feb 24 16:57:00 2004
>  >@@ -336,6 +336,7 @@ static int ipmi_recvmsg(struct kiocb *io
>  > 		}
>  > 
>  > 		timeo = ipmi_wait_for_queue(i, timeo);
>  >+		spin_lock_irqsave(&i->lock, flags);
>  > 	}
>  > 
>  > 	rcvmsg = list_entry(i->msg_list.next, struct ipmi_recv_msg, link);
>  >
>  >
>  > which may or may not be correct.
>  >
>  Actually, I believe the code is correct, and your change will break it.  
>  This is in a "while (1)" loop, and the only way to get out of this loop 
>  is to return with the lock not held or to break out of the loop with the 
>  lock held (and later code will unlock it).  Am I correct here?

With a little more context:

+		spin_unlock_irqrestore(&i->lock, flags);
+		if (!timeo) {
+			return -EAGAIN;
+		} else if (signal_pending (current)) {
+			dbg("Signal pending: %d", 1);
+			return -EINTR;
+		}
+
+		timeo = ipmi_wait_for_queue(i, timeo);
+	}
+
+	rcvmsg = list_entry(i->msg_list.next, struct ipmi_recv_msg, link);
+	list_del(&rcvmsg->link);
+	spin_unlock_irqrestore(&i->lock, flags);

See, there's a direct code path from one spin_unlock() to the other.  And
ipmi_wait_for_queue() does not retake the lock.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/