diff mbox

[2/5] rds: ib: replace spin_lock_irq with spin_lock_irqsave

Message ID 1489044405-26150-2-git-send-email-yanjun.zhu@oracle.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Zhu Yanjun March 9, 2017, 7:26 a.m. UTC
It is difficult to make sure the state of the interrupt when this
function is called. As such, it is safer to use spin_lock_irqsave
than spin_lock_irq.

Cc: Joe Jin <joe.jin@oracle.com>
Cc: Junxiao Bi <junxiao.bi@oracle.com>
Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>
---
 net/rds/ib_cm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Santosh Shilimkar March 9, 2017, 4:50 p.m. UTC | #1
On 3/8/2017 11:26 PM, Zhu Yanjun wrote:
> It is difficult to make sure the state of the interrupt when this
> function is called. As such, it is safer to use spin_lock_irqsave
> than spin_lock_irq.
>
There is no reason to hold irqs  and as such the code path is
safe from irq context. I don't see need of this change unless
you have test case which showed some issue.

Regards,
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhu Yanjun March 12, 2017, 2:33 a.m. UTC | #2
Sorry. I have no test case to show some issue.
But from Linux Kernel Development Second Edition by Robert Love.

Use spin_lock_irq is dangerous since spin_unlock_irq unconditionally 
enables interrupts.

We can assume the following scenario:

--->the interrupt is disabled.

         spin_lock_irq(lock_ptr);   <---this will disable interrupt again
         list_del(&ic->ib_node);
         spin_unlock_irq(lock_ptr); <---this will enable interrupt

---->the interrupt is enabled.

our code change the state of interrupt. This will make potential risk.
But spin_lock_irqsave/spin_unlock_irqrestore will not make potential risk.

Zhu Yanjun
On 2017/3/10 0:50, Santosh Shilimkar wrote:
> On 3/8/2017 11:26 PM, Zhu Yanjun wrote:
>> It is difficult to make sure the state of the interrupt when this
>> function is called. As such, it is safer to use spin_lock_irqsave
>> than spin_lock_irq.
>>
> There is no reason to hold irqs  and as such the code path is
> safe from irq context. I don't see need of this change unless
> you have test case which showed some issue.
>
> Regards,
> Santosh
>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Santosh Shilimkar March 12, 2017, 6:37 a.m. UTC | #3
On 3/11/17 6:33 PM, Yanjun Zhu wrote:
> Sorry. I have no test case to show some issue.
> But from Linux Kernel Development Second Edition by Robert Love.
>
Yes I know the book and what the API does :D

> Use spin_lock_irq is dangerous since spin_unlock_irq unconditionally
> enables interrupts.
>
> We can assume the following scenario:
>
> --->the interrupt is disabled.
>
>         spin_lock_irq(lock_ptr);   <---this will disable interrupt again
>         list_del(&ic->ib_node);
>         spin_unlock_irq(lock_ptr); <---this will enable interrupt
>
> ---->the interrupt is enabled.
>
> our code change the state of interrupt. This will make potential risk.
> But spin_lock_irqsave/spin_unlock_irqrestore will not make potential risk.
>
ic is well protected for any possible race and hence I asked
if you had any test case. Please re-post the series again with
the subject patch dropper for Dave to pick it up.

Regards,
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index eca3d5f..87ec4dd 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -963,6 +963,7 @@  void rds_ib_conn_free(void *arg)
 {
 	struct rds_ib_connection *ic = arg;
 	spinlock_t	*lock_ptr;
+	unsigned long	flags;
 
 	rdsdebug("ic %p\n", ic);
 
@@ -973,9 +974,9 @@  void rds_ib_conn_free(void *arg)
 	 */
 	lock_ptr = ic->rds_ibdev ? &ic->rds_ibdev->spinlock : &ib_nodev_conns_lock;
 
-	spin_lock_irq(lock_ptr);
+	spin_lock_irqsave(lock_ptr, flags);
 	list_del(&ic->ib_node);
-	spin_unlock_irq(lock_ptr);
+	spin_unlock_irqrestore(lock_ptr, flags);
 
 	rds_ib_recv_free_caches(ic);