diff options
author | Steve Wise <swise@opengridcomputing.com> | 2011-05-20 20:25:05 +0400 |
---|---|---|
committer | Roland Dreier <roland@purestorage.com> | 2011-05-24 20:47:38 +0400 |
commit | c337374bf23b88620bcc66a7a09f141cc640f548 (patch) | |
tree | 5867078d9b9f7e8eb44df4e94b08e460aede1616 /drivers/infiniband/hw | |
parent | 257313b2a87795e07a0bdf58d0fffbdba8b31051 (diff) | |
download | linux-c337374bf23b88620bcc66a7a09f141cc640f548.tar.xz |
RDMA/cxgb4: Use completion objects for event blocking
There exists a race condition when using wait_queue_head_t objects
that are declared on the stack. This was being done in a few places
where we are sending work requests to the FW and awaiting replies, but
we don't have an endpoint structure with an embedded c4iw_wr_wait
struct. So the code was allocating it locally on the stack. Bad
design. The race is:
1) thread on cpuX declares the wait_queue_head_t on the stack, then
posts a firmware WR with that wait object ptr as the cookie to be
returned in the WR reply. This thread will proceed to block in
wait_event_timeout() but before it does:
2) An interrupt runs on cpuY with the WR reply. fw6_msg() handles
this and calls c4iw_wake_up(). c4iw_wake_up() sets the condition
variable in the c4iw_wr_wait object to TRUE and will call
wake_up(), but before it calls wake_up():
3) The thread on cpuX calls c4iw_wait_for_reply(), which calls
wait_event_timeout(). The wait_event_timeout() macro checks the
condition variable and returns immediately since it is TRUE. So
this thread never blocks/sleeps. The function then returns
effectively deallocating the c4iw_wr_wait object that was on the
stack.
4) So at this point cpuY has a pointer to the c4iw_wr_wait object
that is no longer valid. Further its pointing to a stack frame
that might now be in use by some other context/thread. So cpuY
continues execution and calls wake_up() on a ptr to a wait object
that as been effectively deallocated.
This race, when it hits, can cause a crash in wake_up(), which I've
seen under heavy stress. It can also corrupt the referenced stack
which can cause any number of failures.
The fix:
Use struct completion, which supports on-stack declarations.
Completions use a spinlock around setting the condition to true and
the wake up so that steps 2 and 4 above are atomic and step 3 can
never happen in-between.
Signed-off-by: Steve Wise <swise@opengridcomputing.com>
Diffstat (limited to 'drivers/infiniband/hw')
-rw-r--r-- | drivers/infiniband/hw/cxgb4/iw_cxgb4.h | 18 |
1 files changed, 5 insertions, 13 deletions
diff --git a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h index 35d2a5dd9bb4..4f045375c8e2 100644 --- a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h +++ b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h @@ -35,7 +35,7 @@ #include <linux/list.h> #include <linux/spinlock.h> #include <linux/idr.h> -#include <linux/workqueue.h> +#include <linux/completion.h> #include <linux/netdevice.h> #include <linux/sched.h> #include <linux/pci.h> @@ -131,28 +131,21 @@ static inline int c4iw_num_stags(struct c4iw_rdev *rdev) #define C4IW_WR_TO (10*HZ) -enum { - REPLY_READY = 0, -}; - struct c4iw_wr_wait { - wait_queue_head_t wait; - unsigned long status; + struct completion completion; int ret; }; static inline void c4iw_init_wr_wait(struct c4iw_wr_wait *wr_waitp) { wr_waitp->ret = 0; - wr_waitp->status = 0; - init_waitqueue_head(&wr_waitp->wait); + init_completion(&wr_waitp->completion); } static inline void c4iw_wake_up(struct c4iw_wr_wait *wr_waitp, int ret) { wr_waitp->ret = ret; - set_bit(REPLY_READY, &wr_waitp->status); - wake_up(&wr_waitp->wait); + complete(&wr_waitp->completion); } static inline int c4iw_wait_for_reply(struct c4iw_rdev *rdev, @@ -164,8 +157,7 @@ static inline int c4iw_wait_for_reply(struct c4iw_rdev *rdev, int ret; do { - ret = wait_event_timeout(wr_waitp->wait, - test_and_clear_bit(REPLY_READY, &wr_waitp->status), to); + ret = wait_for_completion_timeout(&wr_waitp->completion, to); if (!ret) { printk(KERN_ERR MOD "%s - Device %s not responding - " "tid %u qpid %u\n", func, |