| Message ID | 1489044405-26150-2-git-send-email-yanjun.zhu@oracle.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
>
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
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);
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(-)