On Wed, 15 May 2013, Heiko Carstens wrote:
> On Tue, May 14, 2013 at 04:24:02PM -0400, Mikulas Patocka wrote:
> > Hi
> >
> > When reviewing kernel for some crash, I found a very strange function
> > zfcp_qdio_sbal_get - it drops a spinlock, sometimes returns without the
> > spinlock, sometimes retakes the spinlock and returns.
> >
> > The callers of zfcp_qdio_sbal_get assume that the spinlock is still held,
> > but on successful return from zfcp_qdio_sbal_get it is not held.
> >
> > How is this supposed to work? It seems that no one ever ran this code with
> > lock debugging enabled.
> >
> > I am sending a patch, but I can't test it.
> >
> > Mikulas
>
> Your patch would deadlock...
>
> > drivers/s390/scsi/zfcp_qdio.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > Index: linux-2.6/drivers/s390/scsi/zfcp_qdio.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/s390/scsi/zfcp_qdio.c 2013-05-14 22:15:28.000000000 +0200
> > +++ linux-2.6/drivers/s390/scsi/zfcp_qdio.c 2013-05-14 22:15:49.000000000 +0200
> > @@ -250,11 +250,15 @@ int zfcp_qdio_sbal_get(struct zfcp_qdio
> > ret = wait_event_interruptible_timeout(qdio->req_q_wq,
> > zfcp_qdio_sbal_check(qdio), 5 * HZ);
>
> The check zfcp_qdio_sbal_check() grabs the spinlock again. That's really odd
> locking but works, except...
>
> >
> > - if (!(atomic_read(&qdio->adapter->status) & ZFCP_STATUS_ADAPTER_QDIOUP))
> > + if (!(atomic_read(&qdio->adapter->status) & ZFCP_STATUS_ADAPTER_QDIOUP)) {
> > + spin_lock_irq(&qdio->req_q_lock);
> > return -EIO;
> > + }
>
> It's racy for a particular case: if the timeout triggered (ret == 0) and at the
> same time ZFCP_STATUS_ADAPTER_QDIOUP is not set the function would return without
> the lock being held, while it should.
> That's a very tiny and unlikely race bit should be fixed anyway.
>
I see. So do you think this would fix it?
Mikulas
---
zfcp: Fix spinlock imbalance in zfcp_qdio_sbal_get
Signed-off-by: Mikulas Patocka <mpatocka@xxxxxxxxxx>
---
drivers/s390/scsi/zfcp_qdio.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Index: linux-2.6/drivers/s390/scsi/zfcp_qdio.c
===================================================================
--- linux-2.6.orig/drivers/s390/scsi/zfcp_qdio.c 2013-05-15 16:53:14.000000000 +0200
+++ linux-2.6/drivers/s390/scsi/zfcp_qdio.c 2013-05-15 16:54:23.000000000 +0200
@@ -250,8 +250,11 @@ int zfcp_qdio_sbal_get(struct zfcp_qdio
ret = wait_event_interruptible_timeout(qdio->req_q_wq,
zfcp_qdio_sbal_check(qdio), 5 * HZ);
- if (!(atomic_read(&qdio->adapter->status) & ZFCP_STATUS_ADAPTER_QDIOUP))
+ if (!(atomic_read(&qdio->adapter->status) & ZFCP_STATUS_ADAPTER_QDIOUP)) {
+ if (ret <= 0)
+ spin_lock_irq(&qdio->req_q_lock);
return -EIO;
+ }
if (ret > 0)
return 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
