From nobody Sun Nov 24 06:43:24 2024 Received: from mail-pg1-f178.google.com (mail-pg1-f178.google.com [209.85.215.178]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4E2082022D1; Wed, 6 Nov 2024 15:43:12 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.215.178 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730907793; cv=none; b=eTgjbtWuYIsDIeEwXRaFP0f21Nz0GkcY9wOT0CxFD0eYBAunHfZwq2aqAWK4mNMiYFnragZPq1Hjl0PIyGD3EVqEXO/BB9fR/Ja03WBXlqnCQEKpCP0w5Ks+9btZFwyvEie1XRkdBCim9PUdqFDbPGIdJIZxaw7LKm9wgA8JOKk= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730907793; c=relaxed/simple; bh=/zS4rHp/Hc8Im00JLYZ7il0zKi/FfwN8/dYxinyXBLU=; h=From:To:Cc:Subject:Date:Message-Id:In-Reply-To:References: MIME-Version; b=KgYw3KPNGo0Ya+T0ngKgzjAJWe8Np47KYpyyZG8QpBUjufptR+OJRYLuFQmdrIm54NYKv9N8DaE4Qm9aIYML3fxGX7JXjWVHFa5QRWExqBeREqnOhbHhQ7DV+b9uYB8XQmV/vIaDTy54nyRB/5c2cmU08IEVF81Fe3a/mH0i7RY= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=E7A484n4; arc=none smtp.client-ip=209.85.215.178 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="E7A484n4" Received: by mail-pg1-f178.google.com with SMTP id 41be03b00d2f7-7ea9739647bso4943478a12.0; Wed, 06 Nov 2024 07:43:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1730907792; x=1731512592; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:reply-to:references :in-reply-to:message-id:date:subject:cc:to:from:from:to:cc:subject :date:message-id:reply-to; bh=XKPFQifwAlPUKlGKrauY2NTcSBP0FbZ+JbSf6jtQ6SI=; b=E7A484n4TlENUSV6VGM1arsRtfMsPH4+UAYvMtta0FERnhoWHB6ZhoptGES1BpHd+p QdrBXn/wbR2oj4dfiUzkmx+KNtcOT4dFXiHWbvKYgUklDy5ndEFyT3o33M035xX8MSsC 4sr71uZhFnfbKltgPMM+VJBiMuUW1MZd7t9pxLDgC9KBbQhiq6hEUXW9d45uEvqVoUDn 51JgwgOegwu/FJk6kABzDK738KXtUo2vITj7ivVerF88BSmfvq3zmlEF9tMFrinzFxeE 1Uxg5nei23XTL9YoVukIkemZRajNeHKcpQc26yNC4tdXTVEwrAxSqwV6Pk+t3gSqYVCi UxOA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1730907792; x=1731512592; h=content-transfer-encoding:mime-version:reply-to:references :in-reply-to:message-id:date:subject:cc:to:from:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=XKPFQifwAlPUKlGKrauY2NTcSBP0FbZ+JbSf6jtQ6SI=; b=sICyCiIY0bPZMFzq48M75vd12rlOsO4b+kNKVJ6W0F8HrBX73qclp9wKD10hhhHQJ1 JgW+ZnQ37sT7NqGiqsEHY0zLNH9Ww2eRAnAH45SpM44YhOM16u1p6e0ARYebDEMGaIYJ a+hPnJZsY0Ba2SuCUoMB0M5gDsaOFPwNpnjYIhCfb/LytnEvoOdyk1mM58mMGQvZNiU9 bk7NvzyIF2dOAy16MxoaZcAy+3qA1iL8CELq4BIH/cWR3waBQ0t/TpFEXR1GQ1RcMd9K 5l8phQPArZtciP92l9xLjUxItc4BAlWscpTltNtc3Qr77RtjSMb5riI4h1SOfmpOKPSJ o2JA== X-Forwarded-Encrypted: i=1; AJvYcCVYlabOSYhVRgA/5wBlTtexL+QkZvGurr9kYxbnoN0uniIuopewBc/z9Jehwu0fPdcuZI4v9Dhisyrih7E=@vger.kernel.org X-Gm-Message-State: AOJu0YxS+nGYtt+EztAkW1TZpEGo/8FXt3ih6B0kGhPhHR+YDcbMvr8d RC/ps1U8I/YGmYIECxyJVPHkSYiKlEpjYCEf1NKANlept3sEm467 X-Google-Smtp-Source: AGHT+IEhwlqYUXuRMza+wBUb7ZYJYdahvXYYEonKwOarpN0Evb1igYe6fWpDaCgfPxNudrIaxdP3Ew== X-Received: by 2002:a05:6a21:e88:b0:1d8:f171:dccd with SMTP id adf61e73a8af0-1d9a851e08dmr50053872637.37.1730907791632; Wed, 06 Nov 2024 07:43:11 -0800 (PST) Received: from localhost.localdomain (c-67-160-120-253.hsd1.wa.comcast.net. [67.160.120.253]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-720bc2e9203sm12159431b3a.142.2024.11.06.07.43.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 06 Nov 2024 07:43:11 -0800 (PST) From: mhkelley58@gmail.com X-Google-Original-From: mhklinux@outlook.com To: kys@microsoft.com, haiyangz@microsoft.com, wei.liu@kernel.org, decui@microsoft.com, gregkh@linuxfoundation.org, vkuznets@redhat.com Cc: linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH v2 1/2] Drivers: hv: util: Don't force error code to ENODEV in util_probe() Date: Wed, 6 Nov 2024 07:42:46 -0800 Message-Id: <20241106154247.2271-2-mhklinux@outlook.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20241106154247.2271-1-mhklinux@outlook.com> References: <20241106154247.2271-1-mhklinux@outlook.com> Reply-To: mhklinux@outlook.com Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" From: Michael Kelley If the util_init function call in util_probe() returns an error code, util_probe() always return ENODEV, and the error code from the util_init function is lost. The error message output in the caller, vmbus_probe(), doesn't show the real error code. Fix this by just returning the error code from the util_init function. There doesn't seem to be a reason to force ENODEV, as other errors such as ENOMEM can already be returned from util_probe(). And the code in call_driver_probe() implies that ENODEV should mean that a matching driver wasn't found, which is not the case here. Suggested-by: Dexuan Cui Signed-off-by: Michael Kelley --- Changes in v2: None. This is the first version of Patch 1 of this series. The "v2" is due to changes to Patch 2 of the series. drivers/hv/hv_util.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c index c4f525325790..370722220134 100644 --- a/drivers/hv/hv_util.c +++ b/drivers/hv/hv_util.c @@ -590,10 +590,8 @@ static int util_probe(struct hv_device *dev, srv->channel =3D dev->channel; if (srv->util_init) { ret =3D srv->util_init(srv); - if (ret) { - ret =3D -ENODEV; + if (ret) goto error1; - } } =20 /* --=20 2.25.1 From nobody Sun Nov 24 06:43:24 2024 Received: from mail-pf1-f172.google.com (mail-pf1-f172.google.com [209.85.210.172]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 48E212038C6; Wed, 6 Nov 2024 15:43:14 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.172 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730907795; cv=none; b=ljdbvTw/+Nr/aKl0rVpwYCcnz9yAyUub8DsZGA2Q7cL7r1FAQPYymzWq16XEa22reWmLiAjhCOSDRMjMT2mG7CI2jEr1Cd4AHDnTiS3Qo/cMKCpyjRAb8erVJzOJEyEbCGQd67OMt81ri5hAHRDh4fd2Q2rZ4HbBnp4YD3qoqA0= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730907795; c=relaxed/simple; bh=XGi7BRfY0KVls2acmoQQYaBUdaC8qiG96OclMknjWwU=; h=From:To:Cc:Subject:Date:Message-Id:In-Reply-To:References: MIME-Version; b=t10uIrCZ0z5okJxk5ONIGloR+vePo0yZejqx6CnG3ypP20vcDTscy5PwectSNqdGDaBoTSdbRMZsg5arzWepq2S6Rh1dtD3Isl1RGWNS5EGYr/VKDC3NgznTbLNEVHXgFfoQq31ImnOsHIOyghVGv6w9tcSDJrpNPT9+8Z+WFnY= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=QXMcC7lV; arc=none smtp.client-ip=209.85.210.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="QXMcC7lV" Received: by mail-pf1-f172.google.com with SMTP id d2e1a72fcca58-720cb6ac25aso5398027b3a.3; Wed, 06 Nov 2024 07:43:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1730907793; x=1731512593; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:reply-to:references :in-reply-to:message-id:date:subject:cc:to:from:from:to:cc:subject :date:message-id:reply-to; bh=wtS8KUZc7HiS+6QKE3XMtdxXK2mRPt/KTa43TMAkQcA=; b=QXMcC7lVbO/zIaaRHSlou3KjguNmsVXk8hsaLYYx8PqGx8ZTgpfzbmEyoKT9m67IFL c2ds4PI7ThZlIYnLaLBXW45EzK0ABl8KasomJIoNFeLznI1tk5Z4bXAe04jd5DWrKLD1 PmGcPbhlBZ/KtHzd8UONzKB+zg3+pRSx0adRKEtVg9lF4rhQ/ZgujIkrNKltM/1hYcy9 1ccHd0rGXJfOqi5sSi0i6EPK+LoGUghfDdpzCx5YG5Bx+ihqUtVWIKBQEyi8V0js7GiZ s8gwX1oW/3Ed31Mhzj7HBtz0N3Mu7r44PObiGyjOn3jFIdA1WrGcZpwwuFRuuSx7/6Pw /njA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1730907793; x=1731512593; h=content-transfer-encoding:mime-version:reply-to:references :in-reply-to:message-id:date:subject:cc:to:from:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=wtS8KUZc7HiS+6QKE3XMtdxXK2mRPt/KTa43TMAkQcA=; b=Hkrh/Cvbu0h8nrGsIj2AvyQ/x2juUSxnOWLrft3xLMZ0fgR/55B2zOh+lOnO4ACxWD wSySGyFOQTdPcZpbCBiB31ISqFYr3Q2QcVYvsOwY5m253SJ704VIqYhsq85r2DpJCO+f IyxKHumodIKtzIMa2pwzbwLX/H+wpcIIWogaqs6oYvqBTR7uZjLdqcLVvEB5BuM4e4ea pUos8SUZ63CKizAJ919zbnRtSWPtuVlX0VyjykVbtYxe9ySCs542bK+CS9UprlEkUo/w N1169UatqjqY2w8qlbe/fpRPzTnt1RkxhfS3MtWcUD5SQMccAtBmIsmFVLpKDF+OO7Dy P/9g== X-Forwarded-Encrypted: i=1; AJvYcCXS9mUhOSTRcx2jlFyub42/a9CTN7fOToDShQoszTIUR/DpI+T0oYwKTZ186x7FlX5LIu6B/GOUzqxmnBo=@vger.kernel.org X-Gm-Message-State: AOJu0Yxjh5QA3U4qfDymmZTzpoiFDi2vzJYAlXiLa0xVafG6nqEllIwd weB4vyh+k0rMj0O4FxV42iE920IcUiUdu4ImgK+PWNAw66ZHwEmND+qz8A== X-Google-Smtp-Source: AGHT+IGEZsipgsh26rteEWDDgOowRw9ntHlN6sgpT6c1G/w19NjxozpuOnI/4lVzayWkvp0lmz7kgA== X-Received: by 2002:a05:6a00:2389:b0:71e:79a9:ec47 with SMTP id d2e1a72fcca58-72062f82516mr53113858b3a.6.1730907792490; Wed, 06 Nov 2024 07:43:12 -0800 (PST) Received: from localhost.localdomain (c-67-160-120-253.hsd1.wa.comcast.net. [67.160.120.253]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-720bc2e9203sm12159431b3a.142.2024.11.06.07.43.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 06 Nov 2024 07:43:12 -0800 (PST) From: mhkelley58@gmail.com X-Google-Original-From: mhklinux@outlook.com To: kys@microsoft.com, haiyangz@microsoft.com, wei.liu@kernel.org, decui@microsoft.com, gregkh@linuxfoundation.org, vkuznets@redhat.com Cc: linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH v2 2/2] Drivers: hv: util: Avoid accessing a ringbuffer not initialized yet Date: Wed, 6 Nov 2024 07:42:47 -0800 Message-Id: <20241106154247.2271-3-mhklinux@outlook.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20241106154247.2271-1-mhklinux@outlook.com> References: <20241106154247.2271-1-mhklinux@outlook.com> Reply-To: mhklinux@outlook.com Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" From: Michael Kelley If the KVP (or VSS) daemon starts before the VMBus channel's ringbuffer is fully initialized, we can hit the panic below: hv_utils: Registering HyperV Utility Driver hv_vmbus: registering driver hv_utils ... BUG: kernel NULL pointer dereference, address: 0000000000000000 CPU: 44 UID: 0 PID: 2552 Comm: hv_kvp_daemon Tainted: G E 6.11.0-rc3+ #1 RIP: 0010:hv_pkt_iter_first+0x12/0xd0 Call Trace: ... vmbus_recvpacket hv_kvp_onchannelcallback vmbus_on_event tasklet_action_common tasklet_action handle_softirqs irq_exit_rcu sysvec_hyperv_stimer0 asm_sysvec_hyperv_stimer0 ... kvp_register_done hvt_op_read vfs_read ksys_read __x64_sys_read This can happen because the KVP/VSS channel callback can be invoked even before the channel is fully opened: 1) as soon as hv_kvp_init() -> hvutil_transport_init() creates /dev/vmbus/hv_kvp, the kvp daemon can open the device file immediately and register itself to the driver by writing a message KVP_OP_REGISTER1 to the file (which is handled by kvp_on_msg() ->kvp_handle_handshake()) and reading the file for the driver's response, which is handled by hvt_op_read(), which calls hvt->on_read(), i.e. kvp_register_done(). 2) the problem with kvp_register_done() is that it can cause the channel callback to be called even before the channel is fully opened, and when the channel callback is starting to run, util_probe()-> vmbus_open() may have not initialized the ringbuffer yet, so the callback can hit the panic of NULL pointer dereference. To reproduce the panic consistently, we can add a "ssleep(10)" for KVP in __vmbus_open(), just before the first hv_ringbuffer_init(), and then we unload and reload the driver hv_utils, and run the daemon manually within the 10 seconds. Fix the panic by reordering the steps in util_probe() so the char dev entry used by the KVP or VSS daemon is not created until after vmbus_open() has completed. This reordering prevents the race condition from happening. Reported-by: Dexuan Cui Fixes: e0fa3e5e7df6 ("Drivers: hv: utils: fix a race on userspace daemons r= egistration") Cc: stable@vger.kernel.org Signed-off-by: Michael Kelley --- Changes in v2: v2 is a completely different approach to fixing the problem compared to v1 [1]. v1 adds synchronization to deal with the race condition if it happens. This v2 reorders initialization steps so the race condition can never happen in the first place. Note: we would also need to fix the fcopy driver code, but that has been removed in commit ec314f61e4fc ("Drivers: hv: Remove fcopy driver") in the mainline kernel since v6.10. For old 6.x LTS kernels, and the 5.x and 4.x LTS kernels, the fcopy driver needs to be fixed when the fix is backported to the stable kernel branches. [1] https://lore.kernel.org/linux-hyperv/20240909164719.41000-1-decui@micro= soft.com/ drivers/hv/hv_kvp.c | 6 ++++++ drivers/hv/hv_snapshot.c | 6 ++++++ drivers/hv/hv_util.c | 9 +++++++++ drivers/hv/hyperv_vmbus.h | 2 ++ include/linux/hyperv.h | 1 + 5 files changed, 24 insertions(+) diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c index d35b60c06114..77017d951826 100644 --- a/drivers/hv/hv_kvp.c +++ b/drivers/hv/hv_kvp.c @@ -767,6 +767,12 @@ hv_kvp_init(struct hv_util_service *srv) */ kvp_transaction.state =3D HVUTIL_DEVICE_INIT; =20 + return 0; +} + +int +hv_kvp_init_transport(void) +{ hvt =3D hvutil_transport_init(kvp_devname, CN_KVP_IDX, CN_KVP_VAL, kvp_on_msg, kvp_on_reset); if (!hvt) diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c index 0d2184be1691..397f4c8fa46c 100644 --- a/drivers/hv/hv_snapshot.c +++ b/drivers/hv/hv_snapshot.c @@ -388,6 +388,12 @@ hv_vss_init(struct hv_util_service *srv) */ vss_transaction.state =3D HVUTIL_DEVICE_INIT; =20 + return 0; +} + +int +hv_vss_init_transport(void) +{ hvt =3D hvutil_transport_init(vss_devname, CN_VSS_IDX, CN_VSS_VAL, vss_on_msg, vss_on_reset); if (!hvt) { diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c index 370722220134..36ee89c0358b 100644 --- a/drivers/hv/hv_util.c +++ b/drivers/hv/hv_util.c @@ -141,6 +141,7 @@ static struct hv_util_service util_heartbeat =3D { static struct hv_util_service util_kvp =3D { .util_cb =3D hv_kvp_onchannelcallback, .util_init =3D hv_kvp_init, + .util_init_transport =3D hv_kvp_init_transport, .util_pre_suspend =3D hv_kvp_pre_suspend, .util_pre_resume =3D hv_kvp_pre_resume, .util_deinit =3D hv_kvp_deinit, @@ -149,6 +150,7 @@ static struct hv_util_service util_kvp =3D { static struct hv_util_service util_vss =3D { .util_cb =3D hv_vss_onchannelcallback, .util_init =3D hv_vss_init, + .util_init_transport =3D hv_vss_init_transport, .util_pre_suspend =3D hv_vss_pre_suspend, .util_pre_resume =3D hv_vss_pre_resume, .util_deinit =3D hv_vss_deinit, @@ -611,6 +613,13 @@ static int util_probe(struct hv_device *dev, if (ret) goto error; =20 + if (srv->util_init_transport) { + ret =3D srv->util_init_transport(); + if (ret) { + vmbus_close(dev->channel); + goto error; + } + } return 0; =20 error: diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h index d2856023d53c..52cb744b4d7f 100644 --- a/drivers/hv/hyperv_vmbus.h +++ b/drivers/hv/hyperv_vmbus.h @@ -370,12 +370,14 @@ void vmbus_on_event(unsigned long data); void vmbus_on_msg_dpc(unsigned long data); =20 int hv_kvp_init(struct hv_util_service *srv); +int hv_kvp_init_transport(void); void hv_kvp_deinit(void); int hv_kvp_pre_suspend(void); int hv_kvp_pre_resume(void); void hv_kvp_onchannelcallback(void *context); =20 int hv_vss_init(struct hv_util_service *srv); +int hv_vss_init_transport(void); void hv_vss_deinit(void); int hv_vss_pre_suspend(void); int hv_vss_pre_resume(void); diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index 22c22fb91042..02a226bcf0ed 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -1559,6 +1559,7 @@ struct hv_util_service { void *channel; void (*util_cb)(void *); int (*util_init)(struct hv_util_service *); + int (*util_init_transport)(void); void (*util_deinit)(void); int (*util_pre_suspend)(void); int (*util_pre_resume)(void); --=20 2.25.1