From nobody Tue May 14 12:05:54 2024 Delivered-To: importer@patchew.org Authentication-Results: mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=pass(p=none dis=none) header.from=redhat.com ARC-Seal: i=1; a=rsa-sha256; t=1677014666; cv=none; d=zohomail.com; s=zohoarc; b=cNcALlqz/CKzhUtckA3a0hfkttNKtCYTKZ6oOk2KKGFowQqqTNvMJFKOfzsc1i2z8/r5mmO7qRYKe2Kae1F0LZbu0q64daESvkwWtB8lER2dF3a69PNPvYvHtWt6gWgSLvS4Lzm03qcEo6aSn6r0wurU+UQUuIZP8cLER+JlEF0= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1677014666; h=Content-Transfer-Encoding:Cc:Date:From:In-Reply-To:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:MIME-Version:Message-ID:References:Sender:Subject:To; bh=ybi1/y6ffmBuNPx5UvkNecPZkTQV43nX6PuqCNggXQE=; b=SKqbLOnpUPmx+8uL66nEwopu0N2+R9GvH8Y2HDYbTo/B6dzecfR+FsWY+TuVG4zUox+nVNZR8Ga5AQSKfGFMNwTuWZhuR7fXA0/RLr3ZEeVd88DIf++gI5LnpB6g4Bcni/RXBAh2GDvfMt/bJ167v2PoqVsHsT6WGqGIyvSlxjw= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=pass header.from= (p=none dis=none) Return-Path: Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mx.zohomail.com with SMTPS id 1677014666425486.26118774585666; Tue, 21 Feb 2023 13:24:26 -0800 (PST) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pUa6s-0007Gd-2e; Tue, 21 Feb 2023 16:23:38 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pUa6p-0007FV-VP for qemu-devel@nongnu.org; Tue, 21 Feb 2023 16:23:36 -0500 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pUa6o-0001V8-7P for qemu-devel@nongnu.org; Tue, 21 Feb 2023 16:23:35 -0500 Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-282-X_jGUwFHMsWkxTZglVJJ6Q-1; Tue, 21 Feb 2023 16:22:24 -0500 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 6B4181C04341; Tue, 21 Feb 2023 21:22:24 +0000 (UTC) Received: from localhost (unknown [10.39.192.52]) by smtp.corp.redhat.com (Postfix) with ESMTP id C323D1121314; Tue, 21 Feb 2023 21:22:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1677014613; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ybi1/y6ffmBuNPx5UvkNecPZkTQV43nX6PuqCNggXQE=; b=eCa83lzUGYrcCFJO2IqFUY6vY+SjnUFE/mNWRIQ0Lq3LOWXVSaocOtpDl8+iuDqgFG1aYw +8N8OlyvBk73HvPDqljIvUf9Uvmch3ksjbtLMz0u5qxSRCwyyT4jB2QNE9dFv09D4WNAxY Mp7ucUoAOgRbin8ARSh6/3xWtlKt6qU= X-MC-Unique: X_jGUwFHMsWkxTZglVJJ6Q-1 From: Stefan Hajnoczi To: Cc: "Michael S. Tsirkin" , Peter Xu , Paolo Bonzini , =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= , David Hildenbrand , Fam Zheng , , Stefan Hajnoczi , Eric Blake , Kevin Wolf Subject: [PATCH v3 1/3] scsi: protect req->aiocb with AioContext lock Date: Tue, 21 Feb 2023 16:22:16 -0500 Message-Id: <20230221212218.1378734-2-stefanha@redhat.com> In-Reply-To: <20230221212218.1378734-1-stefanha@redhat.com> References: <20230221212218.1378734-1-stefanha@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 Received-SPF: pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) client-ip=209.51.188.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Received-SPF: pass client-ip=170.10.129.124; envelope-from=stefanha@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: qemu-devel-bounces+importer=patchew.org@nongnu.org X-ZohoMail-DKIM: pass (identity @redhat.com) X-ZM-MESSAGEID: 1677014667739100001 Content-Type: text/plain; charset="utf-8" If requests are being processed in the IOThread when a SCSIDevice is unplugged, scsi_device_purge_requests() -> scsi_req_cancel_async() races with I/O completion callbacks. Both threads load and store req->aiocb. This can lead to assert(r->req.aiocb =3D=3D NULL) failures and undefined behavior. Protect r->req.aiocb with the AioContext lock to prevent the race. Reviewed-by: Eric Blake Reviewed-by: Kevin Wolf Signed-off-by: Stefan Hajnoczi --- hw/scsi/scsi-disk.c | 23 ++++++++++++++++------- hw/scsi/scsi-generic.c | 11 ++++++----- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index d4e360850f..115584f8b9 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -273,9 +273,11 @@ static void scsi_aio_complete(void *opaque, int ret) SCSIDiskReq *r =3D (SCSIDiskReq *)opaque; SCSIDiskState *s =3D DO_UPCAST(SCSIDiskState, qdev, r->req.dev); =20 + aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk)); + assert(r->req.aiocb !=3D NULL); r->req.aiocb =3D NULL; - aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk)); + if (scsi_disk_req_check_error(r, ret, true)) { goto done; } @@ -357,10 +359,11 @@ static void scsi_dma_complete(void *opaque, int ret) SCSIDiskReq *r =3D (SCSIDiskReq *)opaque; SCSIDiskState *s =3D DO_UPCAST(SCSIDiskState, qdev, r->req.dev); =20 + aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk)); + assert(r->req.aiocb !=3D NULL); r->req.aiocb =3D NULL; =20 - aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk)); if (ret < 0) { block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct); } else { @@ -393,10 +396,11 @@ static void scsi_read_complete(void *opaque, int ret) SCSIDiskReq *r =3D (SCSIDiskReq *)opaque; SCSIDiskState *s =3D DO_UPCAST(SCSIDiskState, qdev, r->req.dev); =20 + aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk)); + assert(r->req.aiocb !=3D NULL); r->req.aiocb =3D NULL; =20 - aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk)); if (ret < 0) { block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct); } else { @@ -446,10 +450,11 @@ static void scsi_do_read_cb(void *opaque, int ret) SCSIDiskReq *r =3D (SCSIDiskReq *)opaque; SCSIDiskState *s =3D DO_UPCAST(SCSIDiskState, qdev, r->req.dev); =20 + aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk)); + assert (r->req.aiocb !=3D NULL); r->req.aiocb =3D NULL; =20 - aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk)); if (ret < 0) { block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct); } else { @@ -530,10 +535,11 @@ static void scsi_write_complete(void * opaque, int re= t) SCSIDiskReq *r =3D (SCSIDiskReq *)opaque; SCSIDiskState *s =3D DO_UPCAST(SCSIDiskState, qdev, r->req.dev); =20 + aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk)); + assert (r->req.aiocb !=3D NULL); r->req.aiocb =3D NULL; =20 - aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk)); if (ret < 0) { block_acct_failed(blk_get_stats(s->qdev.conf.blk), &r->acct); } else { @@ -1737,10 +1743,11 @@ static void scsi_unmap_complete(void *opaque, int r= et) SCSIDiskReq *r =3D data->r; SCSIDiskState *s =3D DO_UPCAST(SCSIDiskState, qdev, r->req.dev); =20 + aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk)); + assert(r->req.aiocb !=3D NULL); r->req.aiocb =3D NULL; =20 - aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk)); if (scsi_disk_req_check_error(r, ret, true)) { scsi_req_unref(&r->req); g_free(data); @@ -1816,9 +1823,11 @@ static void scsi_write_same_complete(void *opaque, i= nt ret) SCSIDiskReq *r =3D data->r; SCSIDiskState *s =3D DO_UPCAST(SCSIDiskState, qdev, r->req.dev); =20 + aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk)); + assert(r->req.aiocb !=3D NULL); r->req.aiocb =3D NULL; - aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk)); + if (scsi_disk_req_check_error(r, ret, true)) { goto done; } diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c index 92cce20a4d..ac9fa662b4 100644 --- a/hw/scsi/scsi-generic.c +++ b/hw/scsi/scsi-generic.c @@ -111,10 +111,11 @@ static void scsi_command_complete(void *opaque, int r= et) SCSIGenericReq *r =3D (SCSIGenericReq *)opaque; SCSIDevice *s =3D r->req.dev; =20 + aio_context_acquire(blk_get_aio_context(s->conf.blk)); + assert(r->req.aiocb !=3D NULL); r->req.aiocb =3D NULL; =20 - aio_context_acquire(blk_get_aio_context(s->conf.blk)); scsi_command_complete_noio(r, ret); aio_context_release(blk_get_aio_context(s->conf.blk)); } @@ -269,11 +270,11 @@ static void scsi_read_complete(void * opaque, int ret) SCSIDevice *s =3D r->req.dev; int len; =20 + aio_context_acquire(blk_get_aio_context(s->conf.blk)); + assert(r->req.aiocb !=3D NULL); r->req.aiocb =3D NULL; =20 - aio_context_acquire(blk_get_aio_context(s->conf.blk)); - if (ret || r->req.io_canceled) { scsi_command_complete_noio(r, ret); goto done; @@ -386,11 +387,11 @@ static void scsi_write_complete(void * opaque, int re= t) =20 trace_scsi_generic_write_complete(ret); =20 + aio_context_acquire(blk_get_aio_context(s->conf.blk)); + assert(r->req.aiocb !=3D NULL); r->req.aiocb =3D NULL; =20 - aio_context_acquire(blk_get_aio_context(s->conf.blk)); - if (ret || r->req.io_canceled) { scsi_command_complete_noio(r, ret); goto done; --=20 2.39.1 From nobody Tue May 14 12:05:54 2024 Delivered-To: importer@patchew.org Authentication-Results: mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=pass(p=none dis=none) header.from=redhat.com ARC-Seal: i=1; a=rsa-sha256; t=1677014939; cv=none; d=zohomail.com; s=zohoarc; b=bHxWN0PxYNGoLtVnFBLP27tpp+wUA9A+cySVmRGKcgebmfcuTwGfDWxEGZpxHRI1n6CicOcul3MWVKMWxIg8PzP9kMVqr1DvRjzlwa+3uSSSVYyU9vNk05gaHJHNCZuqcwodQhmlJozxGJ87kcYQOpnk3LFyrqCS9Rj8jJCJuYo= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1677014939; h=Content-Transfer-Encoding:Cc:Date:From:In-Reply-To:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:MIME-Version:Message-ID:References:Sender:Subject:To; bh=eWEaLIlSX86vzi4W9dK/vlFbo0TTAz1exoZVoGzX5D0=; b=YTY4HWWxCoJWBZf7/7mttc2M4qGk3eGf/WvQYmv8V8bNhTnt8abun/Uakr/co760wpeHruokjWVU9BmjpwFkv6dlChX9MXYQh4f/e9qjvOoBYwff9tanpVPeosYpYV+CkDOaIpxrA8kHcSsYAuqP+u5NX1S2fYLwnYaDPED57nE= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=pass header.from= (p=none dis=none) Return-Path: Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mx.zohomail.com with SMTPS id 1677014939390479.7580245061896; Tue, 21 Feb 2023 13:28:59 -0800 (PST) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pUaBp-0002Ky-L3; Tue, 21 Feb 2023 16:28:45 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pUaBm-0002Jh-4c for qemu-devel@nongnu.org; Tue, 21 Feb 2023 16:28:42 -0500 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pUaBk-0002bx-Er for qemu-devel@nongnu.org; Tue, 21 Feb 2023 16:28:41 -0500 Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-15-e72YwRElPuO0P_LaKsX7gQ-1; Tue, 21 Feb 2023 16:22:27 -0500 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 47C061C04346; Tue, 21 Feb 2023 21:22:27 +0000 (UTC) Received: from localhost (unknown [10.39.192.52]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8968CC15BA0; Tue, 21 Feb 2023 21:22:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1677014919; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=eWEaLIlSX86vzi4W9dK/vlFbo0TTAz1exoZVoGzX5D0=; b=cdSFXLT0Dq3rxCR8iz8a06yy7Z3MzVF/kO21Y/OtxCUnor8G0BlAiZ2m0H+axBZSYM32Nx 1QfYcZqoQeLAhMhL5XETaurfXHV5rE1gPGNU0jERWXn8ZW4Yn41tKTI7IstEIZFNVav18D gfPxpPlldlRyu7gO6gFHTHNJzgzaghk= X-MC-Unique: e72YwRElPuO0P_LaKsX7gQ-1 From: Stefan Hajnoczi To: Cc: "Michael S. Tsirkin" , Peter Xu , Paolo Bonzini , =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= , David Hildenbrand , Fam Zheng , , Stefan Hajnoczi , Eric Blake Subject: [PATCH v3 2/3] dma-helpers: prevent dma_blk_cb() vs dma_aio_cancel() race Date: Tue, 21 Feb 2023 16:22:17 -0500 Message-Id: <20230221212218.1378734-3-stefanha@redhat.com> In-Reply-To: <20230221212218.1378734-1-stefanha@redhat.com> References: <20230221212218.1378734-1-stefanha@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 Received-SPF: pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) client-ip=209.51.188.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Received-SPF: pass client-ip=170.10.129.124; envelope-from=stefanha@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: qemu-devel-bounces+importer=patchew.org@nongnu.org X-ZohoMail-DKIM: pass (identity @redhat.com) X-ZM-MESSAGEID: 1677014941167100001 Content-Type: text/plain; charset="utf-8" dma_blk_cb() only takes the AioContext lock around ->io_func(). That means the rest of dma_blk_cb() is not protected. In particular, the DMAAIOCB field accesses happen outside the lock. There is a race when the main loop thread holds the AioContext lock and invokes scsi_device_purge_requests() -> bdrv_aio_cancel() -> dma_aio_cancel() while an IOThread executes dma_blk_cb(). The dbs->acb field determines how cancellation proceeds. If dma_aio_cancel() sees dbs->acb =3D=3D NULL while dma_blk_cb() is still running, the request can be completed twice (-ECANCELED and the actual return value). The following assertion can occur with virtio-scsi when an IOThread is used: ../hw/scsi/scsi-disk.c:368: scsi_dma_complete: Assertion `r->req.aiocb != =3D NULL' failed. Fix the race by holding the AioContext across dma_blk_cb(). Now dma_aio_cancel() under the AioContext lock will not see inconsistent/intermediate states. Cc: Paolo Bonzini Reviewed-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- hw/scsi/scsi-disk.c | 4 +--- softmmu/dma-helpers.c | 12 +++++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 115584f8b9..97c9b1c8cd 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -354,13 +354,12 @@ done: scsi_req_unref(&r->req); } =20 +/* Called with AioContext lock held */ static void scsi_dma_complete(void *opaque, int ret) { SCSIDiskReq *r =3D (SCSIDiskReq *)opaque; SCSIDiskState *s =3D DO_UPCAST(SCSIDiskState, qdev, r->req.dev); =20 - aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk)); - assert(r->req.aiocb !=3D NULL); r->req.aiocb =3D NULL; =20 @@ -370,7 +369,6 @@ static void scsi_dma_complete(void *opaque, int ret) block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct); } scsi_dma_complete_noio(r, ret); - aio_context_release(blk_get_aio_context(s->qdev.conf.blk)); } =20 static void scsi_read_complete_noio(SCSIDiskReq *r, int ret) diff --git a/softmmu/dma-helpers.c b/softmmu/dma-helpers.c index 7820fec54c..2463964805 100644 --- a/softmmu/dma-helpers.c +++ b/softmmu/dma-helpers.c @@ -113,17 +113,19 @@ static void dma_complete(DMAAIOCB *dbs, int ret) static void dma_blk_cb(void *opaque, int ret) { DMAAIOCB *dbs =3D (DMAAIOCB *)opaque; + AioContext *ctx =3D dbs->ctx; dma_addr_t cur_addr, cur_len; void *mem; =20 trace_dma_blk_cb(dbs, ret); =20 + aio_context_acquire(ctx); dbs->acb =3D NULL; dbs->offset +=3D dbs->iov.size; =20 if (dbs->sg_cur_index =3D=3D dbs->sg->nsg || ret < 0) { dma_complete(dbs, ret); - return; + goto out; } dma_blk_unmap(dbs); =20 @@ -164,9 +166,9 @@ static void dma_blk_cb(void *opaque, int ret) =20 if (dbs->iov.size =3D=3D 0) { trace_dma_map_wait(dbs); - dbs->bh =3D aio_bh_new(dbs->ctx, reschedule_dma, dbs); + dbs->bh =3D aio_bh_new(ctx, reschedule_dma, dbs); cpu_register_map_client(dbs->bh); - return; + goto out; } =20 if (!QEMU_IS_ALIGNED(dbs->iov.size, dbs->align)) { @@ -174,11 +176,11 @@ static void dma_blk_cb(void *opaque, int ret) QEMU_ALIGN_DOWN(dbs->iov.size, dbs->align)= ); } =20 - aio_context_acquire(dbs->ctx); dbs->acb =3D dbs->io_func(dbs->offset, &dbs->iov, dma_blk_cb, dbs, dbs->io_func_opaque); - aio_context_release(dbs->ctx); assert(dbs->acb); +out: + aio_context_release(ctx); } =20 static void dma_aio_cancel(BlockAIOCB *acb) --=20 2.39.1 From nobody Tue May 14 12:05:54 2024 Delivered-To: importer@patchew.org Authentication-Results: mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=pass(p=none dis=none) header.from=redhat.com ARC-Seal: i=1; a=rsa-sha256; t=1677014653; cv=none; d=zohomail.com; s=zohoarc; b=PwW/oplYUuqVJwShiP85MO3u0DL64HqPWd70V8zlUPikp8gyvfWJIUVcnJghko5ay1oALSa0AjVdBUq4oSKic6vDFLXvkvBKVx2omm+t5Rbk9Znz/rjjrX+P+GsMkOuSpE1ceHNk8f7/FpbQW7qolQIgUKmairRYdN2rSl5o5rY= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1677014653; h=Content-Transfer-Encoding:Cc:Date:From:In-Reply-To:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:MIME-Version:Message-ID:References:Sender:Subject:To; bh=ued9A3Zpvty2xRj8YmIG3CjQ2+7RNd+6SofyrHksWwc=; b=X0dFCcjYHDur0F9/u0sW1B5UBLuk30BlnCarR4AH594Spf4kJ8gwzBgFAKcThO/ZwxQRF0RN4E5BmfmCadV+qUe6QDvGhFw96X9xuxEyMljJyAdEe9ZznP78bFykYO9L+ZoAsU7iFP3WVzf27j73RwzIa+JymW1DXuXxsqkK7sk= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=pass header.from= (p=none dis=none) Return-Path: Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mx.zohomail.com with SMTPS id 1677014653808717.0521724197718; Tue, 21 Feb 2023 13:24:13 -0800 (PST) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pUa6z-0007JN-Cm; Tue, 21 Feb 2023 16:23:46 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pUa6w-0007IV-5b for qemu-devel@nongnu.org; Tue, 21 Feb 2023 16:23:42 -0500 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pUa6u-0001WG-7m for qemu-devel@nongnu.org; Tue, 21 Feb 2023 16:23:41 -0500 Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-191-c3TrWoaJPfmCnznb3ajZTA-1; Tue, 21 Feb 2023 16:22:30 -0500 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id D3F6B380670C; Tue, 21 Feb 2023 21:22:29 +0000 (UTC) Received: from localhost (unknown [10.39.192.52]) by smtp.corp.redhat.com (Postfix) with ESMTP id 36BDD492B04; Tue, 21 Feb 2023 21:22:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1677014619; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ued9A3Zpvty2xRj8YmIG3CjQ2+7RNd+6SofyrHksWwc=; b=EZA1/VpU7UsVxvkq2+5OfjpTXiqeIpZ7TfmuyV9nHRIOKge7xBDfk0UFUAdfDs5R9CfUi3 qDgKWsrduIunYQanzgS2T4R1IL0lILiR/TolUK6W9TZkxoS3I8fKNO1BIIHrLiGu167OEy QWelEvoLqYXHnm2kMPOccAOzh+UGRSU= X-MC-Unique: c3TrWoaJPfmCnznb3ajZTA-1 From: Stefan Hajnoczi To: Cc: "Michael S. Tsirkin" , Peter Xu , Paolo Bonzini , =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= , David Hildenbrand , Fam Zheng , , Stefan Hajnoczi , Qing Wang , Eric Blake Subject: [PATCH v3 3/3] virtio-scsi: reset SCSI devices from main loop thread Date: Tue, 21 Feb 2023 16:22:18 -0500 Message-Id: <20230221212218.1378734-4-stefanha@redhat.com> In-Reply-To: <20230221212218.1378734-1-stefanha@redhat.com> References: <20230221212218.1378734-1-stefanha@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Scanned-By: MIMEDefang 3.1 on 10.11.54.9 Received-SPF: pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) client-ip=209.51.188.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Received-SPF: pass client-ip=170.10.129.124; envelope-from=stefanha@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: qemu-devel-bounces+importer=patchew.org@nongnu.org X-ZohoMail-DKIM: pass (identity @redhat.com) X-ZM-MESSAGEID: 1677014656054100003 Content-Type: text/plain; charset="utf-8" When an IOThread is configured, the ctrl virtqueue is processed in the IOThread. TMFs that reset SCSI devices are currently called directly from the IOThread and trigger an assertion failure in blk_drain() from the following call stack: virtio_scsi_handle_ctrl_req -> virtio_scsi_do_tmf -> device_code_reset -> scsi_disk_reset -> scsi_device_purge_requests -> blk_drain ../block/block-backend.c:1780: void blk_drain(BlockBackend *): Assertion = `qemu_in_main_thread()' failed. The blk_drain() function is not designed to be called from an IOThread because it needs the Big QEMU Lock (BQL). This patch defers TMFs that reset SCSI devices to a Bottom Half (BH) that runs in the main loop thread under the BQL. This way it's safe to call blk_drain() and the assertion failure is avoided. Introduce s->tmf_bh_list for tracking TMF requests that have been deferred to the BH. When the BH runs it will grab the entire list and process all requests. Care must be taken to clear the list when the virtio-scsi device is reset or unrealized. Otherwise deferred TMF requests could execute later and lead to use-after-free or other undefined behavior. The s->resetting counter that's used by TMFs that reset SCSI devices is accessed from multiple threads. This patch makes that explicit by using atomic accessor functions. With this patch applied the counter is only modified by the main loop thread under the BQL but can be read by any thread. Reported-by: Qing Wang Cc: Paolo Bonzini Reviewed-by: Eric Blake Signed-off-by: Stefan Hajnoczi --- include/hw/virtio/virtio-scsi.h | 11 ++- hw/scsi/virtio-scsi.c | 169 +++++++++++++++++++++++++------- 2 files changed, 143 insertions(+), 37 deletions(-) diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scs= i.h index 37b75e15e3..779568ab5d 100644 --- a/include/hw/virtio/virtio-scsi.h +++ b/include/hw/virtio/virtio-scsi.h @@ -74,13 +74,22 @@ struct VirtIOSCSICommon { VirtQueue **cmd_vqs; }; =20 +struct VirtIOSCSIReq; + struct VirtIOSCSI { VirtIOSCSICommon parent_obj; =20 SCSIBus bus; - int resetting; + int resetting; /* written from main loop thread, read from any thread = */ bool events_dropped; =20 + /* + * TMFs deferred to main loop BH. These fields are protected by + * virtio_scsi_acquire(). + */ + QEMUBH *tmf_bh; + QTAILQ_HEAD(, VirtIOSCSIReq) tmf_bh_list; + /* Fields for dataplane below */ AioContext *ctx; /* one iothread per virtio-scsi-pci for now */ =20 diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index 2b649ca976..612c525d9d 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -43,13 +43,11 @@ typedef struct VirtIOSCSIReq { QEMUSGList qsgl; QEMUIOVector resp_iov; =20 - union { - /* Used for two-stage request submission */ - QTAILQ_ENTRY(VirtIOSCSIReq) next; + /* Used for two-stage request submission and TMFs deferred to BH */ + QTAILQ_ENTRY(VirtIOSCSIReq) next; =20 - /* Used for cancellation of request during TMFs */ - int remaining; - }; + /* Used for cancellation of request during TMFs */ + int remaining; =20 SCSIRequest *sreq; size_t resp_size; @@ -294,6 +292,122 @@ static inline void virtio_scsi_ctx_check(VirtIOSCSI *= s, SCSIDevice *d) } } =20 +static void virtio_scsi_do_one_tmf_bh(VirtIOSCSIReq *req) +{ + VirtIOSCSI *s =3D req->dev; + SCSIDevice *d =3D virtio_scsi_device_get(s, req->req.tmf.lun); + BusChild *kid; + int target; + + switch (req->req.tmf.subtype) { + case VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET: + if (!d) { + req->resp.tmf.response =3D VIRTIO_SCSI_S_BAD_TARGET; + goto out; + } + if (d->lun !=3D virtio_scsi_get_lun(req->req.tmf.lun)) { + req->resp.tmf.response =3D VIRTIO_SCSI_S_INCORRECT_LUN; + goto out; + } + qatomic_inc(&s->resetting); + device_cold_reset(&d->qdev); + qatomic_dec(&s->resetting); + break; + + case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET: + target =3D req->req.tmf.lun[1]; + qatomic_inc(&s->resetting); + + rcu_read_lock(); + QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) { + SCSIDevice *d1 =3D SCSI_DEVICE(kid->child); + if (d1->channel =3D=3D 0 && d1->id =3D=3D target) { + device_cold_reset(&d1->qdev); + } + } + rcu_read_unlock(); + + qatomic_dec(&s->resetting); + break; + + default: + g_assert_not_reached(); + break; + } + +out: + object_unref(OBJECT(d)); + + virtio_scsi_acquire(s); + virtio_scsi_complete_req(req); + virtio_scsi_release(s); +} + +/* Some TMFs must be processed from the main loop thread */ +static void virtio_scsi_do_tmf_bh(void *opaque) +{ + VirtIOSCSI *s =3D opaque; + QTAILQ_HEAD(, VirtIOSCSIReq) reqs =3D QTAILQ_HEAD_INITIALIZER(reqs); + VirtIOSCSIReq *req; + VirtIOSCSIReq *tmp; + + GLOBAL_STATE_CODE(); + + virtio_scsi_acquire(s); + + QTAILQ_FOREACH_SAFE(req, &s->tmf_bh_list, next, tmp) { + QTAILQ_REMOVE(&s->tmf_bh_list, req, next); + QTAILQ_INSERT_TAIL(&reqs, req, next); + } + + qemu_bh_delete(s->tmf_bh); + s->tmf_bh =3D NULL; + + virtio_scsi_release(s); + + QTAILQ_FOREACH_SAFE(req, &reqs, next, tmp) { + QTAILQ_REMOVE(&reqs, req, next); + virtio_scsi_do_one_tmf_bh(req); + } +} + +static void virtio_scsi_reset_tmf_bh(VirtIOSCSI *s) +{ + VirtIOSCSIReq *req; + VirtIOSCSIReq *tmp; + + GLOBAL_STATE_CODE(); + + virtio_scsi_acquire(s); + + if (s->tmf_bh) { + qemu_bh_delete(s->tmf_bh); + s->tmf_bh =3D NULL; + } + + QTAILQ_FOREACH_SAFE(req, &s->tmf_bh_list, next, tmp) { + QTAILQ_REMOVE(&s->tmf_bh_list, req, next); + + /* SAM-6 6.3.2 Hard reset */ + req->resp.tmf.response =3D VIRTIO_SCSI_S_TARGET_FAILURE; + virtio_scsi_complete_req(req); + } + + virtio_scsi_release(s); +} + +static void virtio_scsi_defer_tmf_to_bh(VirtIOSCSIReq *req) +{ + VirtIOSCSI *s =3D req->dev; + + QTAILQ_INSERT_TAIL(&s->tmf_bh_list, req, next); + + if (!s->tmf_bh) { + s->tmf_bh =3D qemu_bh_new(virtio_scsi_do_tmf_bh, s); + qemu_bh_schedule(s->tmf_bh); + } +} + /* Return 0 if the request is ready to be completed and return to guest; * -EINPROGRESS if the request is submitted and will be completed later, i= n the * case of async cancellation. */ @@ -301,8 +415,6 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSI= Req *req) { SCSIDevice *d =3D virtio_scsi_device_get(s, req->req.tmf.lun); SCSIRequest *r, *next; - BusChild *kid; - int target; int ret =3D 0; =20 virtio_scsi_ctx_check(s, d); @@ -359,15 +471,9 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCS= IReq *req) break; =20 case VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET: - if (!d) { - goto fail; - } - if (d->lun !=3D virtio_scsi_get_lun(req->req.tmf.lun)) { - goto incorrect_lun; - } - s->resetting++; - device_cold_reset(&d->qdev); - s->resetting--; + case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET: + virtio_scsi_defer_tmf_to_bh(req); + ret =3D -EINPROGRESS; break; =20 case VIRTIO_SCSI_T_TMF_ABORT_TASK_SET: @@ -410,22 +516,6 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCS= IReq *req) } break; =20 - case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET: - target =3D req->req.tmf.lun[1]; - s->resetting++; - - rcu_read_lock(); - QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) { - SCSIDevice *d1 =3D SCSI_DEVICE(kid->child); - if (d1->channel =3D=3D 0 && d1->id =3D=3D target) { - device_cold_reset(&d1->qdev); - } - } - rcu_read_unlock(); - - s->resetting--; - break; - case VIRTIO_SCSI_T_TMF_CLEAR_ACA: default: req->resp.tmf.response =3D VIRTIO_SCSI_S_FUNCTION_REJECTED; @@ -655,7 +745,7 @@ static void virtio_scsi_request_cancelled(SCSIRequest *= r) if (!req) { return; } - if (req->dev->resetting) { + if (qatomic_read(&req->dev->resetting)) { req->resp.cmd.response =3D VIRTIO_SCSI_S_RESET; } else { req->resp.cmd.response =3D VIRTIO_SCSI_S_ABORTED; @@ -831,9 +921,12 @@ static void virtio_scsi_reset(VirtIODevice *vdev) VirtIOSCSICommon *vs =3D VIRTIO_SCSI_COMMON(vdev); =20 assert(!s->dataplane_started); - s->resetting++; + + virtio_scsi_reset_tmf_bh(s); + + qatomic_inc(&s->resetting); bus_cold_reset(BUS(&s->bus)); - s->resetting--; + qatomic_dec(&s->resetting); =20 vs->sense_size =3D VIRTIO_SCSI_SENSE_DEFAULT_SIZE; vs->cdb_size =3D VIRTIO_SCSI_CDB_DEFAULT_SIZE; @@ -1053,6 +1146,8 @@ static void virtio_scsi_device_realize(DeviceState *d= ev, Error **errp) VirtIOSCSI *s =3D VIRTIO_SCSI(dev); Error *err =3D NULL; =20 + QTAILQ_INIT(&s->tmf_bh_list); + virtio_scsi_common_realize(dev, virtio_scsi_handle_ctrl, virtio_scsi_handle_event, @@ -1090,6 +1185,8 @@ static void virtio_scsi_device_unrealize(DeviceState = *dev) { VirtIOSCSI *s =3D VIRTIO_SCSI(dev); =20 + virtio_scsi_reset_tmf_bh(s); + qbus_set_hotplug_handler(BUS(&s->bus), NULL); virtio_scsi_common_unrealize(dev); } --=20 2.39.1