From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED, USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id ED399C43381 for ; Tue, 26 Mar 2019 06:45:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C5BB82075E for ; Tue, 26 Mar 2019 06:45:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731766AbfCZGp0 (ORCPT ); Tue, 26 Mar 2019 02:45:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52404 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731382AbfCZGpZ (ORCPT ); Tue, 26 Mar 2019 02:45:25 -0400 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5D49AC057F4B; Tue, 26 Mar 2019 06:45:24 +0000 (UTC) Received: from ming.t460p (ovpn-8-27.pek2.redhat.com [10.72.8.27]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4BB765C232; Tue, 26 Mar 2019 06:45:15 +0000 (UTC) Date: Tue, 26 Mar 2019 14:45:11 +0800 From: Ming Lei To: Bart Van Assche Cc: "Martin K . Petersen" , "James E . J . Bottomley" , linux-scsi@vger.kernel.org, Christoph Hellwig , Hannes Reinecke , Johannes Thumshirn , Jason Yan , stable@vger.kernel.org Subject: Re: [PATCH] sd: Fix a race between closing an sd device and sd I/O Message-ID: <20190326064509.GA13079@ming.t460p> References: <20190325170146.184414-1-bvanassche@acm.org> <20190326014402.GD30669@ming.t460p> <939cd5ce-6a23-2f2b-616d-39be3440542d@acm.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <939cd5ce-6a23-2f2b-616d-39be3440542d@acm.org> User-Agent: Mutt/1.9.1 (2017-09-22) X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Tue, 26 Mar 2019 06:45:24 +0000 (UTC) Sender: stable-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: stable@vger.kernel.org On Mon, Mar 25, 2019 at 06:56:28PM -0700, Bart Van Assche wrote: > On 3/25/19 6:44 PM, Ming Lei wrote: > > On Mon, Mar 25, 2019 at 10:01:46AM -0700, Bart Van Assche wrote: > > > The scsi_end_request() function calls scsi_cmd_to_driver() indirectly > > > and hence needs the disk->private_data pointer. Avoid that that pointer > > > is cleared before all affected I/O requests have finished. This patch > > > avoids that the following crash occurs: > > > > > > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000 > > > Call trace: > > > scsi_mq_uninit_cmd+0x1c/0x30 > > > scsi_end_request+0x7c/0x1b8 > > > scsi_io_completion+0x464/0x668 > > > scsi_finish_command+0xbc/0x160 > > > scsi_eh_flush_done_q+0x10c/0x170 > > > sas_scsi_recover_host+0x84c/0xa98 [libsas] > > > scsi_error_handler+0x140/0x5b0 > > > kthread+0x100/0x12c > > > ret_from_fork+0x10/0x18 > > > > > > Cc: Christoph Hellwig > > > Cc: Ming Lei > > > Cc: Hannes Reinecke > > > Cc: Johannes Thumshirn > > > Cc: Jason Yan > > > Cc: > > > Reported-by: Jason Yan > > > Signed-off-by: Bart Van Assche > > > --- > > > drivers/scsi/sd.c | 19 +++++++++++++------ > > > 1 file changed, 13 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > > > index ed34bfbc3844..0077880c0cc8 100644 > > > --- a/drivers/scsi/sd.c > > > +++ b/drivers/scsi/sd.c > > > @@ -1416,11 +1416,6 @@ static void sd_release(struct gendisk *disk, fmode_t mode) > > > scsi_set_medium_removal(sdev, SCSI_REMOVAL_ALLOW); > > > } > > > - /* > > > - * XXX and what if there are packets in flight and this close() > > > - * XXX is followed by a "rmmod sd_mod"? > > > - */ > > > - > > > scsi_disk_put(sdkp); > > > } > > > @@ -3483,9 +3478,21 @@ static void scsi_disk_release(struct device *dev) > > > { > > > struct scsi_disk *sdkp = to_scsi_disk(dev); > > > struct gendisk *disk = sdkp->disk; > > > - > > > + struct request_queue *q = disk->queue; > > > + > > > ida_free(&sd_index_ida, sdkp->index); > > > + /* > > > + * Wait until all requests that are in progress have completed. > > > + * This is necessary to avoid that e.g. scsi_end_request() crashes > > > + * due to clearing the disk->private_data pointer. Wait from inside > > > + * scsi_disk_release() instead of from sd_release() to avoid that > > > + * freezing and unfreezing the request queue affects user space I/O > > > + * in case multiple processes open a /dev/sd... node concurrently. > > > + */ > > > + blk_mq_freeze_queue(q); > > > + blk_mq_unfreeze_queue(q); > > > + > > > disk->private_data = NULL; > > > put_disk(disk); > > > put_device(&sdkp->device->sdev_gendev); > > > > No, this way may cause big performance issue, see my previous comment: > > > > https://marc.info/?l=linux-scsi&m=155321977714715&w=2 > > Have you had a look at this patch? Your comment applies to the previous > version of this patch. I don't think that it applies to the current version. OK, sorry for missing that, then this patch looks fine. It is still a bit over-kill for passthrough IO, but seems not a big deal. Thanks, Ming