Patchwork Backtrace in rds code

login
register
mail settings
Submitter David Miller
Date Jan. 24, 2012, 10:04 p.m.
Message ID <20120124.170410.829129930720684609.davem@davemloft.net>
Download mbox | patch
Permalink /patch/137659/
State Accepted
Delegated to: David Miller
Headers show

Comments

David Miller - Jan. 24, 2012, 10:04 p.m.
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(-)
Josh Boyer - Jan. 25, 2012, 12:26 a.m.
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
David Miller - Jan. 25, 2012, 1:49 a.m.
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
Josh Boyer - Jan. 25, 2012, 3:48 a.m.
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

Patch

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)