Backtrace in rds code

The backtrace below is trivial to trigger, even on a very current Linus tree.
It seems that this was brought up before in November[1], but the conversation
died.

I'm curious if anyone is looking into this issue?  Simply running 'rds-info' is
enough to hit the oops on my machine.

josh

[1] http://thread.gmane.org/gmane.linux.network/211976/focus=212054

[24673.860306] ------------[ cut here ]------------
[24673.860319] WARNING: at kernel/softirq.c:159 local_bh_enable_ip+0xba/0x100()
[24673.860323] Hardware name: XPS 8300
[24673.860326] Modules linked in: rds lockd sunrpc ip6t_REJECT
nf_conntrack_ipv6 nf_conntrack_ipv4 nf_defrag_ipv6 nf_defrag_ipv4
xt_state nf_conntrack ip6table_filter ip6_tables snd_hda_codec_hdmi
snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_seq
snd_seq_device snd_pcm snd_timer snd soundcore snd_page_alloc broadcom
dcdbas i2c_i801 microcode iTCO_wdt iTCO_vendor_support tg3 serio_raw
uinput usb_storage uas radeon ttm drm_kms_helper drm i2c_algo_bit
i2c_core [last unloaded: scsi_wait_scan]
[24673.860399] Pid: 23644, comm: rds-info Not tainted
3.3.0-0.rc1.git1.1.fc17.x86_64 #1
[24673.860403] Call Trace:
[24673.860410]  [<ffffffff8106022f>] warn_slowpath_common+0x7f/0xc0
[24673.860416]  [<ffffffff81548570>] ? sock_i_ino+0x40/0x60
[24673.860421]  [<ffffffff8106028a>] warn_slowpath_null+0x1a/0x20
[24673.860426]  [<ffffffff8106836a>] local_bh_enable_ip+0xba/0x100
[24673.860432]  [<ffffffff8169e6b4>] _raw_read_unlock_bh+0x44/0x50
[24673.860437]  [<ffffffff81548570>] sock_i_ino+0x40/0x60
[24673.860444]  [<ffffffffa035a591>] rds_sock_info+0xc1/0x120 [rds]
[24673.860452]  [<ffffffffa035d1b0>] rds_info_getsockopt+0x160/0x200 [rds]
[24673.860459]  [<ffffffffa035a1e3>] rds_getsockopt+0x93/0xf0 [rds]
[24673.860465]  [<ffffffff815475c3>] sys_getsockopt+0x73/0xe0
[24673.860471]  [<ffffffff816a7769>] system_call_fastpath+0x16/0x1b
[24673.860475] ---[ end trace 1aff530051b01526 ]---

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 <redacted>
Reported-by: Josh Boyer <redacted>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/rds/af_rds.c |   20 ++++++++------------
 1 files changed, 8 insertions(+), 12 deletions(-)

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)

-- 
1.7.7.6

On Tue, Jan 24, 2012 at 5:04 PM, David Miller [off-list ref] 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 <redacted>
Reported-by: Josh Boyer <redacted>
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

From: Josh Boyer <redacted>
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.

On Tue, Jan 24, 2012 at 8:49 PM, David Miller [off-list ref] wrote:

From: Josh Boyer <redacted>
Date: Tue, 24 Jan 2012 19:26:25 -0500

quoted

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