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.
--
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
