linux-kernel - Re: [PATCH RESEND] remove the queue unlock in scsi_requset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1344946071.3117.26.camel@dabdike.int.hansenpartnership.com>
Date:	Tue, 14 Aug 2012 13:07:51 +0100
From:	James Bottomley <James.Bottomley@...senPartnership.com>
To:	Chanho Min <chanho0207@...il.com>
Cc:	Mike Christie <michaelc@...wisc.edu>, linux-scsi@...r.kernel.org,
	linux-kernel@...r.kernel.org, Jens Axboe <axboe@...nel.dk>,
	Tejun Heo <tj@...nel.org>, Bart Van Assche <bvanassche@....org>
Subject: Re: [PATCH RESEND] remove the queue unlock in scsi_requset_fn

On Tue, 2012-08-14 at 18:48 +0900, Chanho Min wrote:
> We don't need to unlock the queue before put_device in scsi_request_fn()
> If we trigger the ->remove() function, It occur a oops from the caller.
> So sdev reference count should not be dropped to zero here.
> Also It was added before scsi_device_dev_release() was moved
> to user context, so it is outdated.

None of this sounds to be correct.  The user context comment is
irrelevant because if we happen to be in user context, all the release
functions will occur in line.  I also don't see why the sdev reference
couldn't drop to zero here.

The reason I think we could remove the lock drop is because the queue
reference cannot drop to zero here because the block layer is holding a
reference to run the queue.  It's only the queue ->release function that
would take the queue lock and therefore we're safe to hold it.

James


> Signed-off-by: Chanho Min <chanho.min@....com>
> Reviewed-by: Bart Van Assche <bvanassche@....org>
> ---
>  drivers/scsi/scsi_lib.c |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index ffd7773..cb2185a 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1626,11 +1626,7 @@ out_delay:
>  	if (sdev->device_busy == 0)
>  		blk_delay_queue(q, SCSI_QUEUE_DELAY);
>  out:
> -	/* must be careful here...if we trigger the ->remove() function
> -	 * we cannot be holding the q lock */
> -	spin_unlock_irq(q->queue_lock);
>  	put_device(&sdev->sdev_gendev);
> -	spin_lock_irq(q->queue_lock);
>  }
> 
>  u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/