[PATCH] netxen: Fix a sleep-in-atomic bug in netxen_nic_pci_mem_access_direct

Subsystems: networking drivers, netxen (1/10) gbe support, the rest

STALE3408d

2 messages, 2 authors, 2017-06-20 · open the first message on its own page

[PATCH] netxen: Fix a sleep-in-atomic bug in netxen_nic_pci_mem_access_direct

From: Jia-Ju Bai <hidden>
Date: 2017-06-19 02:46:25

The driver may sleep under a spin lock, and the function call path is:
netxen_nic_pci_mem_access_direct (acquire the lock by spin_lock)
  ioremap --> may sleep

To fix it, the lock is released before "ioremap", and the lock is 
acquired again after this function.

Signed-off-by: Jia-Ju Bai <redacted>
---
 drivers/net/ethernet/qlogic/netxen/netxen_nic_hw.c |    2 ++
 1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_hw.c b/drivers/net/ethernet/qlogic/netxen/netxen_nic_hw.c
index a996801..5ea553e 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_hw.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_hw.c
@@ -1419,7 +1419,9 @@ static u32 netxen_nic_io_read_2M(struct netxen_adapter *adapter,
 
 		mem_base = pci_resource_start(adapter->pdev, 0) +
 					(start & PAGE_MASK);
+		spin_unlock(&adapter->ahw.mem_lock);
 		mem_ptr = ioremap(mem_base, PAGE_SIZE);
+		spin_lock(&adapter->ahw.mem_lock);
 		if (mem_ptr == NULL) {
 			ret = -EIO;
 			goto unlock;
-- 
1.7.9.5

Re: [PATCH] netxen: Fix a sleep-in-atomic bug in netxen_nic_pci_mem_access_direct

From: David Miller <davem@davemloft.net>
Date: 2017-06-20 17:35:33

From: Jia-Ju Bai <redacted>
Date: Mon, 19 Jun 2017 10:48:53 +0800
The driver may sleep under a spin lock, and the function call path is:
netxen_nic_pci_mem_access_direct (acquire the lock by spin_lock)
  ioremap --> may sleep

To fix it, the lock is released before "ioremap", and the lock is 
acquired again after this function.

Signed-off-by: Jia-Ju Bai <redacted>
This style of change you are making is really starting to be a
problem.

You can't just drop locks like this, especially without explaining
why it's ok, and why the mutual exclusion this code was trying to
achieve is still going to be OK afterwards.

In fact, I see zero analysis of the locking situation here, why
it was needed in the first place, and why your change is OK in
that context.

Any locking change is delicate, and you must put the greatest of
care and consideration into it.

Just putting "unlock/lock" around the sleeping operation shows a
very low level of consideration for the implications of the change
you are making.

This isn't like making whitespace fixes, sorry...
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help