| Message ID | 20120124.170410.829129930720684609.davem@davemloft.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Jan 24, 2012 at 5:04 PM, David Miller <davem@davemloft.net> wrote: > > Please try this patch: > > -------------------- > rds: Make rds_sock_lock BH rather than IRQ safe. > > rds_sock_info() triggers locking warnings because we try to perform a > local_bh_enable() (via sock_i_ino()) while hardware interrupts are > disabled (via taking rds_sock_lock). > > There is no reason for rds_sock_lock to be a hardware IRQ disabling > lock, none of these access paths run in hardware interrupt context. > > Therefore making it a BH disabling lock is safe and sufficient to > fix this bug. > > Reported-by: Kumar Sanghvi <kumaras@chelsio.com> > Reported-by: Josh Boyer <jwboyer@gmail.com> > Signed-off-by: David S. Miller <davem@davemloft.net> Using the same base kernel as I did to report the issue, plus this patch the issue seems to be gone. I can't really claim rds works overall, but the oops output is definitely cleared up. Thanks for such a quick turn around. josh -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Josh Boyer <jwboyer@gmail.com> Date: Tue, 24 Jan 2012 19:26:25 -0500 > Using the same base kernel as I did to report the issue, plus this patch the > issue seems to be gone. I can't really claim rds works overall, but the oops > output is definitely cleared up. > > Thanks for such a quick turn around. Thanks for testing, the fix is already in Linus's tree and I'll ship it off to -stable eventually too. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 24, 2012 at 8:49 PM, David Miller <davem@davemloft.net> wrote: > From: Josh Boyer <jwboyer@gmail.com> > Date: Tue, 24 Jan 2012 19:26:25 -0500 > >> Using the same base kernel as I did to report the issue, plus this patch the >> issue seems to be gone. I can't really claim rds works overall, but the oops >> output is definitely cleared up. >> >> Thanks for such a quick turn around. > > Thanks for testing, the fix is already in Linus's tree and I'll ship > it off to -stable eventually too. Great. FWIW, I tested a 3.2.1+this patch build and it works there as well. josh -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c index bb6ad81..424ff62 100644 --- a/net/rds/af_rds.c +++ b/net/rds/af_rds.c @@ -68,7 +68,6 @@ static int rds_release(struct socket *sock) { struct sock *sk = sock->sk; struct rds_sock *rs; - unsigned long flags; if (!sk) goto out; @@ -94,10 +93,10 @@ static int rds_release(struct socket *sock) rds_rdma_drop_keys(rs); rds_notify_queue_get(rs, NULL); - spin_lock_irqsave(&rds_sock_lock, flags); + spin_lock_bh(&rds_sock_lock); list_del_init(&rs->rs_item); rds_sock_count--; - spin_unlock_irqrestore(&rds_sock_lock, flags); + spin_unlock_bh(&rds_sock_lock); rds_trans_put(rs->rs_transport); @@ -409,7 +408,6 @@ static const struct proto_ops rds_proto_ops = { static int __rds_create(struct socket *sock, struct sock *sk, int protocol) { - unsigned long flags; struct rds_sock *rs; sock_init_data(sock, sk); @@ -426,10 +424,10 @@ static int __rds_create(struct socket *sock, struct sock *sk, int protocol) spin_lock_init(&rs->rs_rdma_lock); rs->rs_rdma_keys = RB_ROOT; - spin_lock_irqsave(&rds_sock_lock, flags); + spin_lock_bh(&rds_sock_lock); list_add_tail(&rs->rs_item, &rds_sock_list); rds_sock_count++; - spin_unlock_irqrestore(&rds_sock_lock, flags); + spin_unlock_bh(&rds_sock_lock); return 0; } @@ -471,12 +469,11 @@ static void rds_sock_inc_info(struct socket *sock, unsigned int len, { struct rds_sock *rs; struct rds_incoming *inc; - unsigned long flags; unsigned int total = 0; len /= sizeof(struct rds_info_message); - spin_lock_irqsave(&rds_sock_lock, flags); + spin_lock_bh(&rds_sock_lock); list_for_each_entry(rs, &rds_sock_list, rs_item) { read_lock(&rs->rs_recv_lock); @@ -492,7 +489,7 @@ static void rds_sock_inc_info(struct socket *sock, unsigned int len, read_unlock(&rs->rs_recv_lock); } - spin_unlock_irqrestore(&rds_sock_lock, flags); + spin_unlock_bh(&rds_sock_lock); lens->nr = total; lens->each = sizeof(struct rds_info_message); @@ -504,11 +501,10 @@ static void rds_sock_info(struct socket *sock, unsigned int len, { struct rds_info_socket sinfo; struct rds_sock *rs; - unsigned long flags; len /= sizeof(struct rds_info_socket); - spin_lock_irqsave(&rds_sock_lock, flags); + spin_lock_bh(&rds_sock_lock); if (len < rds_sock_count) goto out; @@ -529,7 +525,7 @@ out: lens->nr = rds_sock_count; lens->each = sizeof(struct rds_info_socket); - spin_unlock_irqrestore(&rds_sock_lock, flags); + spin_unlock_bh(&rds_sock_lock); } static void rds_exit(void)
Please try this patch: -------------------- rds: Make rds_sock_lock BH rather than IRQ safe. rds_sock_info() triggers locking warnings because we try to perform a local_bh_enable() (via sock_i_ino()) while hardware interrupts are disabled (via taking rds_sock_lock). There is no reason for rds_sock_lock to be a hardware IRQ disabling lock, none of these access paths run in hardware interrupt context. Therefore making it a BH disabling lock is safe and sufficient to fix this bug. Reported-by: Kumar Sanghvi <kumaras@chelsio.com> Reported-by: Josh Boyer <jwboyer@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net> --- net/rds/af_rds.c | 20 ++++++++------------ 1 files changed, 8 insertions(+), 12 deletions(-)