* Re: [PATCH net] net: mana: Fix TOCTOU double-fetch of hwc_msg_id from DMA buffer
From: patchwork-bot+netdevbpf @ 2026-05-19 11:10 UTC (permalink / raw)
To: Erni Sri Satya Vennela
Cc: kys, haiyangz, wei.liu, decui, longli, andrew+netdev, davem,
edumazet, kuba, pabeni, dipayanroy, horms, kees, shacharr,
stephen, linux-hyperv, netdev, linux-kernel
In-Reply-To: <20260514194156.466823-1-ernis@linux.microsoft.com>
Hello:
This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:
On Thu, 14 May 2026 12:41:51 -0700 you wrote:
> In mana_hwc_rx_event_handler(), resp->response.hwc_msg_id is read from
> DMA-coherent memory and bounds-checked, then mana_hwc_handle_resp()
> re-reads the same field from the same DMA buffer for test_bit() and
> pointer arithmetic.
>
> DMA-coherent memory is mapped uncacheable on x86 and is shared,
> unencrypted, in Confidential VMs (SEV-SNP/TDX), so each load goes
> directly to host-visible memory. A H/W can modify the value
> between the check and the use, bypassing the bounds validation.
>
> [...]
Here is the summary with links:
- [net] net: mana: Fix TOCTOU double-fetch of hwc_msg_id from DMA buffer
https://git.kernel.org/netdev/net/c/35f0f0a2536a
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH v2 0/5] treewide: Convert buses to use generic driver_override
From: Danilo Krummrich @ 2026-05-19 11:09 UTC (permalink / raw)
To: gregkh, rafael, linux, nipun.gupta, nikhil.agarwal, kys, haiyangz,
wei.liu, decui, longli, andersson, mathieu.poirier
Cc: driver-core, linux-kernel, linux-hyperv, linux-arm-msm,
linux-remoteproc
In-Reply-To: <DIG1MUKJVLEO.YGTSSYIO5T1K@kernel.org>
On Mon May 11, 2026 at 8:02 PM CEST, Danilo Krummrich wrote:
> On Tue May 5, 2026 at 3:37 PM CEST, Danilo Krummrich wrote:
>> This series is based on v7.1-rc1 with no additional dependencies, hence those
>> patches can be picked up by subsystems individually.
>
> Gentle ping on this one; I can alternatively also take those patches through the
> driver-core tree if you prefer.
Since the remaining patches of this series have been out since v7.0-rc5 without
any objections, I'm planning to pick it up through the driver-core tree for
v7.2-rc1.
Please let me know if there are any concerns or whether you prefer to take
individual patches through your own tree.
Thanks,
Danilo
^ permalink raw reply
* Re: [PATCH net] net: mana: Fix TOCTOU double-fetch of hwc_msg_id from DMA buffer
From: Paolo Abeni @ 2026-05-19 11:00 UTC (permalink / raw)
To: Erni Sri Satya Vennela, kys, haiyangz, wei.liu, decui, longli,
andrew+netdev, davem, edumazet, kuba, dipayanroy, horms, kees,
shacharr, stephen, linux-hyperv, netdev, linux-kernel
In-Reply-To: <20260514194156.466823-1-ernis@linux.microsoft.com>
On 5/14/26 9:41 PM, Erni Sri Satya Vennela wrote:
> In mana_hwc_rx_event_handler(), resp->response.hwc_msg_id is read from
> DMA-coherent memory and bounds-checked, then mana_hwc_handle_resp()
> re-reads the same field from the same DMA buffer for test_bit() and
> pointer arithmetic.
>
> DMA-coherent memory is mapped uncacheable on x86 and is shared,
> unencrypted, in Confidential VMs (SEV-SNP/TDX), so each load goes
> directly to host-visible memory. A H/W can modify the value
> between the check and the use, bypassing the bounds validation.
Sashiko noted there are more related issues in the nearby code:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260514194156.466823-1-ernis%40linux.microsoft.com
you may consider a follow-up.
/P
^ permalink raw reply
* [PATCH net-next v9] net: mana: Expose hardware diagnostic info via debugfs
From: Erni Sri Satya Vennela @ 2026-05-19 6:46 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, longli, andrew+netdev, davem,
edumazet, kuba, pabeni, kotaranov, horms, shradhagupta,
dipayanroy, ernis, kees, linux-hyperv, netdev, linux-kernel,
linux-rdma
Add debugfs entries to expose hardware configuration and diagnostic
information that aids in debugging driver initialization and runtime
operations without adding noise to dmesg.
The debugfs directory for each PCI device is named using pci_name()
(the unique BDF address), and its creation and removal is integrated
into mana_gd_setup() and mana_gd_cleanup_device() respectively, so
that all callers (probe, remove, suspend, resume, shutdown) share a
single code path.
Device-level entries (under /sys/kernel/debug/mana/<BDF>/):
- num_msix_usable, max_num_queues: Max resources from hardware
- gdma_protocol_ver, pf_cap_flags1: VF version negotiation results
- num_vports, bm_hostmode: Device configuration
Per-vPort entries (under /sys/kernel/debug/mana/<BDF>/vportN/):
- port_handle: Hardware vPort handle
- max_sq, max_rq: Max queues from vPort config
- indir_table_sz: Indirection table size
- steer_rx, steer_rss, steer_update_tab, steer_cqe_coalescing:
Last applied steering configuration parameters
Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com>
---
Change in v9:
* Change steer_update_tab type from u32 to bool and use
debugfs_create_bool() accordingly
* Guard debugfs_lookup_and_remove() calls in mana_remove() with a
NULL check on gc->mana_pci_debugfs
* Fix mana_gd_resume() RDMA failure unwind: call mana_rdma_remove()
to undo partial RDMA state and return err, instead of
mana_remove(true) + mana_gd_cleanup_device(), avoiding a UAF
where gf_stats_work could fire against an already-destroyed HWC
Changes in v8:
* Move debugfs_create_u16("num_vports", ...) and
debugfs_create_u8("bm_hostmode", ...) to after ac->num_ports has been
assigned and clamped to MAX_PORTS_IN_MANA_DEV, so the value exposed
via debugfs always reflects the final, hardware-reported count
rather than a transient zero or unclamped value.
* Update the stale comment above mana_gd_resume() to reflect the new
rollback-on-failure behavior.
Changes in v7:
* Rebase to latest main.
Changes in v6:
* Move out of patchset and create a separate patch.
Changes in v5:
* Update commit message.
* Fix conflicts to align with the new patches.
* Make it part of patchset.
Changes in v4:
* Rebase and fix conflicts.
Changes in v3:
* Rename mana_gd_cleanup to mana_gd_cleanup_device.
* Add creation of debugfs entries in mana_gd_setup.
* Add removal of debugfs entries in mana_gd_cleanup_device.
* Remove bm_hostmode and num_vports from debugfs in mana_remove itself,
because "ac" gets freed before debugfs_remove_recursive, to avoid
Use-After-Free error.
* Add "goto out:" in mana_cfg_vport_steering to avoid populating apc
values when resp.hdr.status is not NULL.
Changes in v2:
* Add debugfs_remove_recursice for gc>mana_pci_debugfs in
mana_gd_suspend to handle multiple duplicates creation in
mana_gd_setup and mana_gd_resume path.
* Move debugfs creation for num_vports and bm_hostmode out of
if(!resuming) condition since we have to create it again even for
resume.
* Recreate mana_pci_debugfs in mana_gd_resume.
---
.../net/ethernet/microsoft/mana/gdma_main.c | 73 ++++++++++---------
drivers/net/ethernet/microsoft/mana/mana_en.c | 35 +++++++++
include/net/mana/gdma.h | 1 +
include/net/mana/mana.h | 8 ++
4 files changed, 83 insertions(+), 34 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index 3bc3fff55999..712a0881d720 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -227,6 +227,11 @@ static int mana_gd_query_max_resources(struct pci_dev *pdev)
if (gc->max_num_queues == 0)
return -ENOSPC;
+ debugfs_create_u32("num_msix_usable", 0400, gc->mana_pci_debugfs,
+ &gc->num_msix_usable);
+ debugfs_create_u32("max_num_queues", 0400, gc->mana_pci_debugfs,
+ &gc->max_num_queues);
+
return 0;
}
@@ -1297,6 +1302,13 @@ int mana_gd_verify_vf_version(struct pci_dev *pdev)
return err ? err : -EPROTO;
}
gc->pf_cap_flags1 = resp.pf_cap_flags1;
+ gc->gdma_protocol_ver = resp.gdma_protocol_ver;
+
+ debugfs_create_x64("gdma_protocol_ver", 0400, gc->mana_pci_debugfs,
+ &gc->gdma_protocol_ver);
+ debugfs_create_x64("pf_cap_flags1", 0400, gc->mana_pci_debugfs,
+ &gc->pf_cap_flags1);
+
if (resp.pf_cap_flags1 & GDMA_DRV_CAP_FLAG_1_HWC_TIMEOUT_RECONFIG) {
err = mana_gd_query_hwc_timeout(pdev, &hwc->hwc_timeout);
if (err) {
@@ -1976,15 +1988,20 @@ static int mana_gd_setup(struct pci_dev *pdev)
struct gdma_context *gc = pci_get_drvdata(pdev);
int err;
+ gc->mana_pci_debugfs = debugfs_create_dir(pci_name(pdev),
+ mana_debugfs_root);
+
err = mana_gd_init_registers(pdev);
if (err)
- return err;
+ goto remove_debugfs;
mana_smc_init(&gc->shm_channel, gc->dev, gc->shm_base);
gc->service_wq = alloc_ordered_workqueue("gdma_service_wq", 0);
- if (!gc->service_wq)
- return -ENOMEM;
+ if (!gc->service_wq) {
+ err = -ENOMEM;
+ goto remove_debugfs;
+ }
err = mana_gd_setup_hwc_irqs(pdev);
if (err) {
@@ -2025,11 +2042,14 @@ static int mana_gd_setup(struct pci_dev *pdev)
free_workqueue:
destroy_workqueue(gc->service_wq);
gc->service_wq = NULL;
+remove_debugfs:
+ debugfs_remove_recursive(gc->mana_pci_debugfs);
+ gc->mana_pci_debugfs = NULL;
dev_err(&pdev->dev, "%s failed (error %d)\n", __func__, err);
return err;
}
-static void mana_gd_cleanup(struct pci_dev *pdev)
+static void mana_gd_cleanup_device(struct pci_dev *pdev)
{
struct gdma_context *gc = pci_get_drvdata(pdev);
@@ -2041,6 +2061,10 @@ static void mana_gd_cleanup(struct pci_dev *pdev)
destroy_workqueue(gc->service_wq);
gc->service_wq = NULL;
}
+
+ debugfs_remove_recursive(gc->mana_pci_debugfs);
+ gc->mana_pci_debugfs = NULL;
+
dev_dbg(&pdev->dev, "mana gdma cleanup successful\n");
}
@@ -2098,9 +2122,6 @@ static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
gc->dev = &pdev->dev;
xa_init(&gc->irq_contexts);
- gc->mana_pci_debugfs = debugfs_create_dir(pci_name(pdev),
- mana_debugfs_root);
-
err = mana_gd_setup(pdev);
if (err)
goto unmap_bar;
@@ -2129,16 +2150,8 @@ static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
cleanup_mana:
mana_remove(&gc->mana, false);
cleanup_gd:
- mana_gd_cleanup(pdev);
+ mana_gd_cleanup_device(pdev);
unmap_bar:
- /*
- * at this point we know that the other debugfs child dir/files
- * are either not yet created or are already cleaned up.
- * The pci debugfs folder clean-up now, will only be cleaning up
- * adapter-MTU file and apc->mana_pci_debugfs folder.
- */
- debugfs_remove_recursive(gc->mana_pci_debugfs);
- gc->mana_pci_debugfs = NULL;
xa_destroy(&gc->irq_contexts);
pci_iounmap(pdev, bar0_va);
free_gc:
@@ -2188,11 +2201,7 @@ static void mana_gd_remove(struct pci_dev *pdev)
mana_rdma_remove(&gc->mana_ib);
mana_remove(&gc->mana, false);
- mana_gd_cleanup(pdev);
-
- debugfs_remove_recursive(gc->mana_pci_debugfs);
-
- gc->mana_pci_debugfs = NULL;
+ mana_gd_cleanup_device(pdev);
xa_destroy(&gc->irq_contexts);
@@ -2214,15 +2223,11 @@ int mana_gd_suspend(struct pci_dev *pdev, pm_message_t state)
mana_rdma_remove(&gc->mana_ib);
mana_remove(&gc->mana, true);
- mana_gd_cleanup(pdev);
+ mana_gd_cleanup_device(pdev);
return 0;
}
-/* In case the NIC hardware stops working, the suspend and resume callbacks will
- * fail -- if this happens, it's safer to just report an error than try to undo
- * what has been done.
- */
int mana_gd_resume(struct pci_dev *pdev)
{
struct gdma_context *gc = pci_get_drvdata(pdev);
@@ -2234,13 +2239,17 @@ int mana_gd_resume(struct pci_dev *pdev)
err = mana_probe(&gc->mana, true);
if (err)
- return err;
+ goto cleanup_gd;
err = mana_rdma_probe(&gc->mana_ib);
if (err)
- return err;
+ mana_rdma_remove(&gc->mana_ib);
- return 0;
+ return err;
+
+cleanup_gd:
+ mana_gd_cleanup_device(pdev);
+ return err;
}
/* Quiesce the device for kexec. This is also called upon reboot/shutdown. */
@@ -2253,11 +2262,7 @@ static void mana_gd_shutdown(struct pci_dev *pdev)
mana_rdma_remove(&gc->mana_ib);
mana_remove(&gc->mana, true);
- mana_gd_cleanup(pdev);
-
- debugfs_remove_recursive(gc->mana_pci_debugfs);
-
- gc->mana_pci_debugfs = NULL;
+ mana_gd_cleanup_device(pdev);
pci_disable_device(pdev);
}
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index b2faa7cf398f..82f1461a48e9 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -1282,6 +1282,9 @@ static int mana_query_vport_cfg(struct mana_port_context *apc, u32 vport_index,
apc->port_handle = resp.vport;
ether_addr_copy(apc->mac_addr, resp.mac_addr);
+ apc->vport_max_sq = *max_sq;
+ apc->vport_max_rq = *max_rq;
+
return 0;
}
@@ -1436,6 +1439,11 @@ static int mana_cfg_vport_steering(struct mana_port_context *apc,
netdev_info(ndev, "Configured steering vPort %llu entries %u\n",
apc->port_handle, apc->indir_table_sz);
+
+ apc->steer_rx = rx;
+ apc->steer_rss = apc->rss_state;
+ apc->steer_update_tab = update_tab;
+ apc->steer_cqe_coalescing = req->cqe_coalescing_enable;
out:
kfree(req);
return err;
@@ -3178,6 +3186,23 @@ static int mana_init_port(struct net_device *ndev)
eth_hw_addr_set(ndev, apc->mac_addr);
sprintf(vport, "vport%d", port_idx);
apc->mana_port_debugfs = debugfs_create_dir(vport, gc->mana_pci_debugfs);
+
+ debugfs_create_u64("port_handle", 0400, apc->mana_port_debugfs,
+ &apc->port_handle);
+ debugfs_create_u32("max_sq", 0400, apc->mana_port_debugfs,
+ &apc->vport_max_sq);
+ debugfs_create_u32("max_rq", 0400, apc->mana_port_debugfs,
+ &apc->vport_max_rq);
+ debugfs_create_u32("indir_table_sz", 0400, apc->mana_port_debugfs,
+ &apc->indir_table_sz);
+ debugfs_create_u32("steer_rx", 0400, apc->mana_port_debugfs,
+ &apc->steer_rx);
+ debugfs_create_u32("steer_rss", 0400, apc->mana_port_debugfs,
+ &apc->steer_rss);
+ debugfs_create_bool("steer_update_tab", 0400, apc->mana_port_debugfs,
+ &apc->steer_update_tab);
+ debugfs_create_u32("steer_cqe_coalescing", 0400, apc->mana_port_debugfs,
+ &apc->steer_cqe_coalescing);
debugfs_create_u32("current_speed", 0400, apc->mana_port_debugfs,
&apc->speed);
return 0;
@@ -3695,6 +3720,11 @@ int mana_probe(struct gdma_dev *gd, bool resuming)
if (ac->num_ports > MAX_PORTS_IN_MANA_DEV)
ac->num_ports = MAX_PORTS_IN_MANA_DEV;
+ debugfs_create_u16("num_vports", 0400, gc->mana_pci_debugfs,
+ &ac->num_ports);
+ debugfs_create_u8("bm_hostmode", 0400, gc->mana_pci_debugfs,
+ &ac->bm_hostmode);
+
ac->per_port_queue_reset_wq =
create_singlethread_workqueue("mana_per_port_queue_reset_wq");
if (!ac->per_port_queue_reset_wq) {
@@ -3817,6 +3847,11 @@ void mana_remove(struct gdma_dev *gd, bool suspending)
mana_gd_deregister_device(gd);
+ if (gc->mana_pci_debugfs) {
+ debugfs_lookup_and_remove("bm_hostmode", gc->mana_pci_debugfs);
+ debugfs_lookup_and_remove("num_vports", gc->mana_pci_debugfs);
+ }
+
if (suspending)
return;
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 6d836060976a..70d62bc32837 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -442,6 +442,7 @@ struct gdma_context {
struct gdma_dev mana_ib;
u64 pf_cap_flags1;
+ u64 gdma_protocol_ver;
struct workqueue_struct *service_wq;
diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
index aa90a858c8e3..d9c27310fd04 100644
--- a/include/net/mana/mana.h
+++ b/include/net/mana/mana.h
@@ -568,6 +568,14 @@ struct mana_port_context {
/* Debugfs */
struct dentry *mana_port_debugfs;
+
+ /* Cached vport/steering config for debugfs */
+ u32 vport_max_sq;
+ u32 vport_max_rq;
+ u32 steer_rx;
+ u32 steer_rss;
+ bool steer_update_tab;
+ u32 steer_cqe_coalescing;
};
netdev_tx_t mana_start_xmit(struct sk_buff *skb, struct net_device *ndev);
--
2.34.1
^ permalink raw reply related
* Re: [PATCH V3 09/11] x86/hyperv: Implement Hyper-V virtual IOMMU
From: Jacob Pan @ 2026-05-19 5:41 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Mukesh R, hpa, robin.murphy, robh, wei.liu, mhklinux, muislam,
namjain, magnuskulke, anbelski, linux-kernel, linux-hyperv, iommu,
linux-pci, linux-arch, kys, haiyangz, decui, longli, tglx, mingo,
bp, dave.hansen, x86, joro, will, lpieralisi, kwilczynski,
bhelgaas, arnd, jacob.pan
In-Reply-To: <20260515182322.GI7702@ziepe.ca>
Hi Jason,
On Fri, 15 May 2026 15:23:22 -0300
Jason Gunthorpe <jgg@ziepe.ca> wrote:
> On Mon, May 11, 2026 at 07:02:57PM -0700, Mukesh R wrote:
> > +static struct iommu_domain *hv_iommu_domain_alloc_paging(struct
> > device *dev) +{
> > + struct hv_domain *hvdom;
> > + int rc;
> > +
> > + if (hv_l1vh_partition() && !hv_curr_thread_is_vmm()) {
> > + pr_err("Hyper-V: l1vh iommu does not support host
> > devices\n");
> > + return NULL;
> > + }
> > +
> > + hvdom = kzalloc(sizeof(struct hv_domain), GFP_KERNEL);
> > + if (hvdom == NULL)
> > + return NULL;
> > +
> > + spin_lock_init(&hvdom->mappings_lock);
> > + hvdom->mappings_tree = RB_ROOT_CACHED;
> > +
> > + /* Called under iommu group mutex, so single threaded */
> > + if (++unique_id == HV_DEVICE_DOMAIN_ID_S2_NULL) /* ie,
> > UINTMAX */
> > + goto out_err;
> > +
> > + hvdom->domid_num = unique_id;
> > + hvdom->partid = hv_get_current_partid();
> > + hvdom->iommu_dom.geometry = default_geometry;
> > + hvdom->iommu_dom.pgsize_bitmap = HV_IOMMU_PGSIZES;
> > +
> > + /* For guests, by default we do direct attaches, so no
> > domain in hyp */
> > + if (hv_dom_owner_is_vmm(hvdom) && !hv_no_attdev)
> > + hvdom->attached_dom = true;
>
> What are you thinking sending something like this?!?!?
>
> The function is called *alloc domain PAGING*, it does not, and can not
> allocate weird "special" domains that are not PAGING domains. I just
> spent a long time removing all this kind of crazyness from drivers.
>
> There is alot of other things I don't like in this patch, but this is
> too much.
>
> You have to drop this "direct attach" idea from the first iteration,
> Linux can't do it without alot more work, you should start with the
> basic paging domain mode.
>
Just wondering what work is needed to support this "direct attach"? I
felt this issue is due to trying to cram two distinct domain types
(paging domain & direct attach) into the VFIO container model where
only unmanaged paging domain is supported.
I am thinking if we were to switch to iommufd and let user(vmm) have
direct control of HWPT, vmm will be able to selectively use a
different domain type to handle direct attach. IMHO, it is essentially
the same as attaching nest parent domain without nested domain
immediately attached. The unprivileged guest may attach nested domain
directly with Hyper-V if nested translation is needed.
I understand nest parent is still a paging domain today and it is
expected to work with nested domain. So maybe we can make iommufd
accept nest parent w/o paging? i.e. no map/unmap and do not call
iopt_table_add_domain()? or maybe a new object?
From this driver POV, it will allocate a 2nd stage only domain with
different domain ops (w/o map/unmap) for "direct attach" thus avoid this
hack.
Thanks,
Jacob
^ permalink raw reply
* RE: [PATCH v3 31/41] x86/tsc: Pass KNOWN_FREQ and RELIABLE as params to registration
From: Michael Kelley @ 2026-05-19 3:12 UTC (permalink / raw)
To: Sean Christopherson, sashiko-reviews@lists.linux.dev
Cc: linux-hyperv@vger.kernel.org
In-Reply-To: <aguQIJQYsarMScnl@google.com>
From: Sean Christopherson <seanjc@google.com> Sent: Monday, May 18, 2026 3:18 PM
>
> +Michael (sashiko was quite stingy with its Cc list)
>
> On Fri, May 15, 2026, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [High] Hyper-V VMs with an invariant TSC but no frequency MSRs lose the X86_FEATURE_TSC_RELIABLE capability.
> > --
> >
> > commit 0c0fbc4f0451c30c0e87e0e3591ea4f487eff75c
> > Author: Sean Christopherson <seanjc@google.com>
> >
> > x86/tsc: Pass KNOWN_FREQ and RELIABLE as params to registration
> >
> > This commit adds a tsc_properties parameter to the paravirtual TSC
> > calibration registration routine to consolidate feature flag tracking.
> > It updates various hypervisor initialization paths to pass combined flags
> > instead of manually setting the CPU capabilities.
> >
> > > diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> > > --- a/arch/x86/kernel/cpu/mshyperv.c
> > > +++ b/arch/x86/kernel/cpu/mshyperv.c
> > > @@ -516,8 +516,13 @@ static void __init ms_hyperv_init_platform(void)
> > >
> > > if (ms_hyperv.features & HV_ACCESS_FREQUENCY_MSRS &&
> > > ms_hyperv.misc_features & HV_FEATURE_FREQUENCY_MSRS_AVAILABLE) {
> > > - tsc_register_calibration_routines(hv_get_tsc_khz, hv_get_tsc_khz);
> > > - setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
> > > + enum tsc_properties tsc_properties = TSC_FREQUENCY_KNOWN;
> > > +
> > > + if (ms_hyperv.features & HV_ACCESS_TSC_INVARIANT)
> > > + tsc_properties = TSC_FREQ_KNOWN_AND_RELIABLE;
> > > +
> > > + tsc_register_calibration_routines(hv_get_tsc_khz, hv_get_tsc_khz,
> > > + tsc_properties);
> > > }
> >
> > [ ... ]
> >
> > > @@ -629,7 +634,6 @@ static void __init ms_hyperv_init_platform(void)
> > > * is called.
> > > */
> > > wrmsrq(HV_X64_MSR_TSC_INVARIANT_CONTROL, HV_EXPOSE_INVARIANT_TSC);
> > > - setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
> > > }
> >
> > If a Hyper-V VM exposes an invariant TSC but lacks the frequency MSRs,
> > does it bypass the tsc_register_calibration_routines() block entirely?
>
> Yes.
>
> > Without the standalone setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE) call
> > here, it looks like these VMs will lose the reliable flag.
> >
> > Will this inadvertently enable the TSC watchdog, potentially causing a
> > performance regression if the system falsely marks the TSC as unstable due
> > to virtualization scheduling delays?
>
> Hmm, I was going to say that the change was intentional and desriable, but looking
> at this yet again, I don't think that's true. Enabling HV_EXPOSE_INVARIANT_TSC
> just means the kernel will (probably) set X86_FEATURE_CONSTANT_TSC and
> X86_FEATURE_NONSTOP_TSC during early_init_intel(), AFAICT it doesn't lead to
> X86_FEATURE_TSC_RELIABLE being set. And I think in this case, marking the TSC
> as reliable makes sense; even if the kernel doesn't user Hyper-V's calibration
> info, the host is still clearly telling the guest that the TSC is reliable.
Yes, I agree. But I'm doubtful that such a combination ever occurs in practice.
I've never seen an occurrence of a Hyper-V (even really old versions) guest
where the frequency MSRs are not available or are not accessible. The
Hyper-V spec allows for either condition, so we have the code to test the
flags. I've thought of the flags as always set, though I suppose one never
knows what the future holds.
>
> Michael, does keeping the
>
> setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
>
> but also passing TSC_FREQ_KNOWN_AND_RELIABLE to the calibration routine make
> sense?
I don't see that it would break anything. But it seems a bit disjointed in
that HV_ACCESS_TSC_INVARIANT is tested in two places in
ms_hyperv_init_platform(). Does TSC_RELIABLE *need* to be passed to
tsc_register_calibration_routines() if later code in ms_hyperv_init_platform()
does setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE)? In other words,
I'm suggesting let tsc_register_calibration_routines() handle the
TSC_FREQ_KNOWN case since that's what the calibration routines are
all about. Leave the setting of X86_FEATURE_TSC_RELIABLE to the
later code that tests HV_ACCESS_TSC_INVARIANT, instead of
duplicating the setup_force_cpu_cap() operation.
While combining FREQUENCY_KNOWN and RELIABLE into
tsc_register_calibration_routines() is convenient, the two
concepts turn out to be independent when looking strictly at
the Hyper-V spec and code written to follow that spec.
Combining them into the same function ends up being clumsy
in this case.
Michael
^ permalink raw reply
* Re: [PATCH v3 00/41] x86: Try to wrangle PV clocks vs. TSC
From: David Woodhouse @ 2026-05-18 23:38 UTC (permalink / raw)
To: Sean Christopherson, Kiryl Shutsemau, Paolo Bonzini,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li,
Ajay Kaher, Alexey Makhalov, Jan Kiszka, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Juergen Gross, Daniel Lezcano,
Thomas Gleixner, John Stultz
Cc: Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, x86, linux-coco, kvm, linux-hyperv, virtualization,
linux-kernel, xen-devel, Michael Kelley, Tom Lendacky,
Nikunj A Dadhania, Thomas Gleixner
In-Reply-To: <20260515191942.1892718-1-seanjc@google.com>
[-- Attachment #1: Type: text/plain, Size: 3647 bytes --]
On Fri, 2026-05-15 at 12:19 -0700, Sean Christopherson wrote:
> Dave/Thomas/Peter/Boris, what's the going rate for bribes to take something
> like this through the tip tree?
>
> The bulk of the changes are in kvmclock and TSC, but pretty much every
> hypervisor's guest-side code gets touched at some point. I am reaonsably
> confident in the correctness of the KVM changes. Michael tested Hyper-V in
> v2, and while there were conflicts when rebasing, they were largely
> superficial (and I've just jinxed myself). For all other hypervisors, assume
> the code is compile-tested only, but those changes are all quite small and
> straightforward.
>
> The only changes that are questionable/contentious are the last two patches,
> which have KVM-as-a-guest use CPUID 0x16 to get the CPU frequency, even on
> AMD (that's the dubious part). I very deliberately put them last, so that
> they can be dropped at will (I don't care terribly if those patches land).
> To merge them, I would want explicit Acks from Paolo and David W.
>
> So, except for the last two patches, to get the stuff I really care about
> landed, I think/hope it's just the TSC and guest-side CoCo changes that need
> reviews/acks?
>
> The primary goal of this series is (or at least was, when I started) to
> fix flaws with SNP and TDX guests where a PV clock provided by the untrusted
> hypervisor is used instead of the secure/trusted TSC that is controlled by
> trusted firmware.
>
> The secondary goal is to draft off of the SNP and TDX changes to slightly
> modernize running under KVM. Currently, KVM guests will use TSC for
> clocksource, but not sched_clock. And they ignore Intel's CPUID-based TSC
> and CPU frequency enumeration, even when using the TSC instead of kvmclock.
> And if the host provides the core crystal frequency in CPUID.0x15, then KVM
> guests can use that for the APIC timer period instead of manually calibrating
> the frequency.
>
> The tertiary goal is to clean up all of the PV clock code to deduplicate logic
> across hypervisors, and to hopefully make it all easier to maintain going
> forward.
I booted this in qemu with -cpu host,+invtsc,+vmware-cpuid-freq
I was expecting to see it eschew the kvmclock and use *only* the TSC.
Is there even any need for 'tsc-early' given that it's *told* the TSC
frequency in CPUID? Shouldn't it have detected that the TSC is known
before init_tsc_clocksource() runs?
And then it even spent some time at boot actually using the kvmclock as
clocksource... when ideally I don't think it would even have *enabled*
it at all?
[ 0.000000] clocksource: kvm-clock: mask: 0xffffffffffffffff max_cycles: 0x1cd42e4dffb, max_idle_ns: 881590591483 ns
[ 0.000000] tsc: Detected 2400.000 MHz processor
[ 0.008205] TSC deadline timer available
[ 0.008270] clocksource: refined-jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 1910969940391419 ns
[ 0.159085] clocksource: hpet: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604467 ns
[ 0.164074] clocksource: tsc-early: mask: 0xffffffffffffffff max_cycles: 0x22983777dd9, max_idle_ns: 440795300422 ns
[ 0.229087] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 1911260446275000 ns
[ 0.337095] clocksource: Switched to clocksource kvm-clock
[ 0.345246] clocksource: acpi_pm: mask: 0xffffff max_cycles: 0xffffff, max_idle_ns: 2085701024 ns
[ 0.356201] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x22983777dd9, max_idle_ns: 440795300422 ns
[ 0.360560] clocksource: Switched to clocksource tsc
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]
^ permalink raw reply
* Re: [PATCH v3 27/41] x86/kvmclock: Enable kvmclock on APs during onlining if kvmclock isn't sched_clock
From: Sean Christopherson @ 2026-05-18 23:04 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-hyperv
In-Reply-To: <20260515194705.09AFDC2BCB0@smtp.kernel.org>
On Fri, May 15, 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The logic to defer AP kvmclock registration to the CPUHP_AP_ONLINE_DYN state when it is not used as sched_clock is architecturally unsafe and will break timekeeping.
> --
>
> commit 88625772445f01c4397e3dfd4ee43222efd758cf
> Author: Sean Christopherson <seanjc@google.com>
>
> x86/kvmclock: Enable kvmclock on APs during onlining if kvmclock isn't sched_clock
>
> This commit prepares for making early_percpu_clock_init() a dedicated
> sched_clock hook. It adds logic to enable kvmclock on APs during CPU online
> if kvmclock is not used as sched_clock, and includes WARNs in the suspend
> and resume paths.
>
> > diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> > index e7250d21c6721..d3bb281c0805d 100644
> > --- a/arch/x86/kernel/kvmclock.c
> > +++ b/arch/x86/kernel/kvmclock.c
> [ ... ]
> > @@ -141,25 +142,51 @@ static void kvm_restore_sched_clock_state(void)
> [ ... ]
> > void kvmclock_cpu_action(enum kvm_guest_cpu_action action)
> > {
> > - /*
> > - * Don't disable kvmclock on the BSP during suspend. If kvmclock is
> > - * being used for sched_clock, then it needs to be kept alive until the
> > - * last minute, and restored as quickly as possible after resume.
> > - */
> > - if (action != KVM_GUEST_BSP_SUSPEND)
> > + switch (action) {
> [ ... ]
> > + case KVM_GUEST_AP_ONLINE:
> > + /*
> > + * Secondary CPUs use dedicated sched_clock hooks to enable
> > + * kvmclock early during bringup, there's nothing to be done
> > + * when during CPU online.
> > + */
> > + if (kvmclock_is_sched_clock)
> > + break;
> > + kvm_register_clock("secondary cpu, online");
> > + break;
>
> Will deferring AP kvmclock registration to the CPU online phase break
> timekeeping if kvmclock is still being used as the system clocksource?
>
> If the user boots with clocksource=kvm-clock or if kvmclock is selected as a
> fallback, it will be used as the clocksource despite not being sched_clock.
> The kvmclock clocksource read function accesses per-CPU memory that the
> hypervisor only populates after the AP registers its clock.
>
> If early registration in start_secondary() is skipped, as anticipated here,
> an AP will boot, enable local interrupts, and process timer ticks before
> reaching the late CPUHP_AP_ONLINE_DYN hotplug state.
>
> Could a call to ktime_get() during this window read uninitialized per-CPU
> memory and return 0? It seems this might cause the timekeeper to compute
> massive forward time jumps, leading to immediate timer expirations and
> watchdogs firing.
Hrm, yes, that seems plausible. Even if it's not actually possible in practice,
replacing x86_cpuinit.early_percpu_clock_init() with a dedicated
paravirt_sched_clock_start_secondary() isn't obviously better, and so definitely
isn't worth risking breakage, at least not in this series.
I'll scrap most of this, but still plumb in AP_ONLINE and BSP_RESUME for
documentation purposes.
> > + case KVM_GUEST_AP_OFFLINE:
> > + case KVM_GUEST_SHUTDOWN:
> > kvmclock_disable();
> > + break;
> > + default:
> > + WARN_ON_ONCE(1);
> > + break;
> > + }
> > }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260515191942.1892718-1-seanjc@google.com?part=27
^ permalink raw reply
* Re: [PATCH v3 31/41] x86/tsc: Pass KNOWN_FREQ and RELIABLE as params to registration
From: Sean Christopherson @ 2026-05-18 22:18 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-hyperv, Michael Kelley
In-Reply-To: <20260515194535.56B84C2BCB0@smtp.kernel.org>
+Michael (sashiko was quite stingy with its Cc list)
On Fri, May 15, 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Hyper-V VMs with an invariant TSC but no frequency MSRs lose the X86_FEATURE_TSC_RELIABLE capability.
> --
>
> commit 0c0fbc4f0451c30c0e87e0e3591ea4f487eff75c
> Author: Sean Christopherson <seanjc@google.com>
>
> x86/tsc: Pass KNOWN_FREQ and RELIABLE as params to registration
>
> This commit adds a tsc_properties parameter to the paravirtual TSC
> calibration registration routine to consolidate feature flag tracking.
> It updates various hypervisor initialization paths to pass combined flags
> instead of manually setting the CPU capabilities.
>
> > diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> > --- a/arch/x86/kernel/cpu/mshyperv.c
> > +++ b/arch/x86/kernel/cpu/mshyperv.c
> > @@ -516,8 +516,13 @@ static void __init ms_hyperv_init_platform(void)
> >
> > if (ms_hyperv.features & HV_ACCESS_FREQUENCY_MSRS &&
> > ms_hyperv.misc_features & HV_FEATURE_FREQUENCY_MSRS_AVAILABLE) {
> > - tsc_register_calibration_routines(hv_get_tsc_khz, hv_get_tsc_khz);
> > - setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
> > + enum tsc_properties tsc_properties = TSC_FREQUENCY_KNOWN;
> > +
> > + if (ms_hyperv.features & HV_ACCESS_TSC_INVARIANT)
> > + tsc_properties = TSC_FREQ_KNOWN_AND_RELIABLE;
> > +
> > + tsc_register_calibration_routines(hv_get_tsc_khz, hv_get_tsc_khz,
> > + tsc_properties);
> > }
>
> [ ... ]
>
> > @@ -629,7 +634,6 @@ static void __init ms_hyperv_init_platform(void)
> > * is called.
> > */
> > wrmsrq(HV_X64_MSR_TSC_INVARIANT_CONTROL, HV_EXPOSE_INVARIANT_TSC);
> > - setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
> > }
>
> If a Hyper-V VM exposes an invariant TSC but lacks the frequency MSRs,
> does it bypass the tsc_register_calibration_routines() block entirely?
Yes.
> Without the standalone setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE) call
> here, it looks like these VMs will lose the reliable flag.
>
> Will this inadvertently enable the TSC watchdog, potentially causing a
> performance regression if the system falsely marks the TSC as unstable due
> to virtualization scheduling delays?
Hmm, I was going to say that the change was intentional and desriable, but looking
at this yet again, I don't think that's true. Enabling HV_EXPOSE_INVARIANT_TSC
just means the kernel will (probably) set X86_FEATURE_CONSTANT_TSC and
X86_FEATURE_NONSTOP_TSC during early_init_intel(), AFAICT it doesn't lead to
X86_FEATURE_TSC_RELIABLE being set. And I think in this case, marking the TSC
as reliable makes sense; even if the kernel doesn't user Hyper-V's calibration
info, the host is still clearly telling the guest that the TSC is reliable.
Michael, does keeping the
setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
but also passing TSC_FREQ_KNOWN_AND_RELIABLE to the calibration routine make
sense?
^ permalink raw reply
* Re: [PATCH v3 02/41] x86/tsc: Add helper to register CPU and TSC freq calibration routines
From: Woodhouse, David @ 2026-05-18 21:59 UTC (permalink / raw)
To: tglx@kernel.org, longli@microsoft.com, luto@kernel.org,
alexey.makhalov@broadcom.com, jstultz@google.com,
dave.hansen@linux.intel.com, ajay.kaher@broadcom.com,
jan.kiszka@siemens.com, haiyangz@microsoft.com, kas@kernel.org,
seanjc@google.com, pbonzini@redhat.com, kys@microsoft.com,
decui@microsoft.com, daniel.lezcano@kernel.org,
wei.liu@kernel.org, peterz@infradead.org, jgross@suse.com
Cc: boris.ostrovsky@oracle.com, linux-coco@lists.linux.dev,
kvm@vger.kernel.org, mhklinux@outlook.com,
thomas.lendacky@amd.com, linux-kernel@vger.kernel.org,
bcm-kernel-feedback-list@broadcom.com, tglx@linutronix.de,
nikunj@amd.com, xen-devel@lists.xenproject.org,
linux-hyperv@vger.kernel.org, vkuznets@redhat.com,
rick.p.edgecombe@intel.com, virtualization@lists.linux.dev,
sboyd@kernel.org, x86@kernel.org
In-Reply-To: <20260515191942.1892718-3-seanjc@google.com>
[-- Attachment #1.1: Type: text/plain, Size: 999 bytes --]
On Fri, 2026-05-15 at 12:19 -0700, Sean Christopherson wrote:
>
> --- a/arch/x86/xen/time.c
> +++ b/arch/x86/xen/time.c
> @@ -569,7 +569,7 @@ static void __init xen_init_time_common(void)
> static_call_update(pv_steal_clock, xen_steal_clock);
> paravirt_set_sched_clock(xen_sched_clock);
>
> - x86_platform.calibrate_tsc = xen_tsc_khz;
> + tsc_register_calibration_routines(xen_tsc_khz, NULL);
> x86_platform.get_wallclock = xen_get_wallclock;
> }
>
xen_tsc_khz() doesn't use CPUID but really *should*.
Care to pull in
https://lore.kernel.org/all/20260509224824.3264567-31-dwmw2@infradead.org/
to your next round please?
(Without the misplaced changes in kvm/x86.c that should have been in
two different prior commits, and are now folded into those correctly in
my kvmclock5 branch ready for the next posting of that).
I'll drop that patch, and the similar x86/kvm one which you *have*
already taken in this series, from my next posting.
Thanks.
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5964 bytes --]
[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]
Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH net 1/2] net: mana: Add NULL guards in teardown path to prevent panic on attach failure
From: Harshitha Ramamurthy @ 2026-05-18 21:49 UTC (permalink / raw)
To: Dipayaan Roy
Cc: kys, haiyangz, wei.liu, decui, andrew+netdev, davem, edumazet,
kuba, pabeni, leon, longli, kotaranov, horms, shradhagupta,
ssengar, ernis, shirazsaleem, linux-hyperv, netdev, linux-kernel,
linux-rdma, stephen, jacob.e.keller, dipayanroy, leitao, kees,
john.fastabend, hawk, bpf, daniel, ast, sdf, yury.norov
In-Reply-To: <20260518194654.735580-2-dipayanroy@linux.microsoft.com>
On Mon, May 18, 2026 at 12:52 PM Dipayaan Roy
<dipayanroy@linux.microsoft.com> wrote:
>
> When queue allocation fails partway through, the error cleanup frees
> and NULLs apc->tx_qp and apc->rxqs. Multiple teardown paths such as
> mana_remove(), mana_change_mtu() recovery, and internal error handling
> in mana_alloc_queues() can subsequently call into functions that
> dereference these pointers without NULL checks:
>
> - mana_chn_setxdp() dereferences apc->rxqs[0], causing a NULL pointer
> dereference panic (CR2: 0000000000000000 at mana_chn_setxdp+0x26).
> - mana_destroy_vport() iterates apc->rxqs without a NULL check.
> - mana_fence_rqs() iterates apc->rxqs without a NULL check.
> - mana_dealloc_queues() iterates apc->tx_qp without a NULL check.
>
> Add NULL guards for apc->rxqs in mana_fence_rqs(),
> mana_destroy_vport(), and before the mana_chn_setxdp() call. Add a
> NULL guard for apc->tx_qp in mana_dealloc_queues() to skip TX queue
> draining when TX queues were never allocated or already freed.
>
> Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: Dipayaan Roy <dipayanroy@linux.microsoft.com>
> ---
> drivers/net/ethernet/microsoft/mana/mana_en.c | 70 +++++++++++--------
> 1 file changed, 41 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 9afc786b297a..0582803907a8 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -1727,6 +1727,9 @@ static void mana_fence_rqs(struct mana_port_context *apc)
> struct mana_rxq *rxq;
> int err;
>
> + if (!apc->rxqs)
> + return;
> +
> for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
> rxq = apc->rxqs[rxq_idx];
> err = mana_fence_rq(apc, rxq);
> @@ -2858,13 +2861,16 @@ static void mana_destroy_vport(struct mana_port_context *apc)
> struct mana_rxq *rxq;
> u32 rxq_idx;
>
> - for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
> - rxq = apc->rxqs[rxq_idx];
> - if (!rxq)
> - continue;
> + if (apc->rxqs) {
>
> - mana_destroy_rxq(apc, rxq, true);
> - apc->rxqs[rxq_idx] = NULL;
> + for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
> + rxq = apc->rxqs[rxq_idx];
> + if (!rxq)
> + continue;
> +
> + mana_destroy_rxq(apc, rxq, true);
> + apc->rxqs[rxq_idx] = NULL;
> + }
> }
>
> mana_destroy_txq(apc);
> @@ -3269,7 +3275,8 @@ static int mana_dealloc_queues(struct net_device *ndev)
> if (apc->port_is_up)
> return -EINVAL;
>
> - mana_chn_setxdp(apc, NULL);
> + if (apc->rxqs)
> + mana_chn_setxdp(apc, NULL);
Is this check required? mana_dealloc_queues() is only called by
mana_detach(). And mana_detach() calls `mana_dealloc_queues()` only if
the port state was previously up. apc->port_is_up seems to be set to
true only on successful queue allocation. Can apc->rxqs be NULL here?
>
> if (gd->gdma_context->is_pf && !apc->ac->bm_hostmode)
> mana_pf_deregister_filter(apc);
> @@ -3287,33 +3294,38 @@ static int mana_dealloc_queues(struct net_device *ndev)
> * number of queues.
> */
>
> - for (i = 0; i < apc->num_queues; i++) {
> - txq = &apc->tx_qp[i].txq;
> - tsleep = 1000;
> - while (atomic_read(&txq->pending_sends) > 0 &&
> - time_before(jiffies, timeout)) {
> - usleep_range(tsleep, tsleep + 1000);
> - tsleep <<= 1;
> - }
> - if (atomic_read(&txq->pending_sends)) {
> - err = pcie_flr(to_pci_dev(gd->gdma_context->dev));
> - if (err) {
> - netdev_err(ndev, "flr failed %d with %d pkts pending in txq %u\n",
> - err, atomic_read(&txq->pending_sends),
> - txq->gdma_txq_id);
> + if (apc->tx_qp) {
And same as above, is it possible for apc->tx_qp to be NULL here?
> + for (i = 0; i < apc->num_queues; i++) {
> + txq = &apc->tx_qp[i].txq;
> + tsleep = 1000;
> + while (atomic_read(&txq->pending_sends) > 0 &&
> + time_before(jiffies, timeout)) {
> + usleep_range(tsleep, tsleep + 1000);
> + tsleep <<= 1;
> + }
> + if (atomic_read(&txq->pending_sends)) {
> + err =
> + pcie_flr(to_pci_dev(gd->gdma_context->dev));
> + if (err) {
> + netdev_err(ndev, "flr failed %d with %d pkts pending in txq %u\n",
> + err,
> + atomic_read(&txq->pending_sends),
> + txq->gdma_txq_id);
> + }
> + break;
> }
> - break;
> }
> - }
>
> - for (i = 0; i < apc->num_queues; i++) {
> - txq = &apc->tx_qp[i].txq;
> - while ((skb = skb_dequeue(&txq->pending_skbs))) {
> - mana_unmap_skb(skb, apc);
> - dev_kfree_skb_any(skb);
> + for (i = 0; i < apc->num_queues; i++) {
> + txq = &apc->tx_qp[i].txq;
> + while ((skb = skb_dequeue(&txq->pending_skbs))) {
> + mana_unmap_skb(skb, apc);
> + dev_kfree_skb_any(skb);
> + }
> + atomic_set(&txq->pending_sends, 0);
> }
> - atomic_set(&txq->pending_sends, 0);
> }
> +
> /* We're 100% sure the queues can no longer be woken up, because
> * we're sure now mana_poll_tx_cq() can't be running.
> */
> --
> 2.43.0
>
>
^ permalink raw reply
* Re: [PATCH v8 2/2] net: mana: force full-page RX buffers via ethtool private flag
From: Dipayaan Roy @ 2026-05-18 21:33 UTC (permalink / raw)
To: Jakub Kicinski
Cc: kys, haiyangz, wei.liu, decui, andrew+netdev, davem, edumazet,
pabeni, leon, longli, kotaranov, horms, shradhagupta, ssengar,
ernis, shirazsaleem, linux-hyperv, netdev, linux-kernel,
linux-rdma, stephen, jacob.e.keller, dipayanroy, leitao, kees,
john.fastabend, hawk, bpf, daniel, ast, sdf, yury.norov
In-Reply-To: <20260512022133.856196-1-kuba@kernel.org>
On Mon, May 11, 2026 at 07:21:33PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: mana: force full-page RX buffers via ethtool private flag
>
> This patch adds an ethtool private flag "full-page-rx" that forces one
> RX buffer per page, bypassing the page_pool fragment path, to address
> a 15-20% throughput regression seen on some 4K PAGE_SIZE ARM64
> platforms under high connection counts. Toggling the flag triggers
> mana_detach() + mana_attach() to apply the new RX buffer layout.
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > index a28ca461c135..dfd222de11d0 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
>
> [ ... ]
>
> > @@ -590,6 +609,88 @@ static int mana_get_link_ksettings(struct net_device *ndev,
> > return 0;
> > }
> >
>
> [ ... ]
>
> > +static int mana_set_priv_flags(struct net_device *ndev, u32 priv_flags)
> > +{
> > + struct mana_port_context *apc = netdev_priv(ndev);
> > + u32 changed = apc->priv_flags ^ priv_flags;
> > + u32 old_priv_flags = apc->priv_flags;
> > + bool schedule_port_reset = false;
> > + int err = 0;
> > +
> > + if (!changed)
> > + return 0;
> > +
> > + /* Reject unknown bits */
> > + if (priv_flags & ~GENMASK(MANA_PRIV_FLAG_MAX - 1, 0))
> > + return -EINVAL;
> > +
> > + if (changed & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF)) {
> > + apc->priv_flags = priv_flags;
> > +
> > + if (!apc->port_is_up) {
> > + /* Port is down, flag updated to apply on next up
> > + * so just return.
> > + */
> > + return 0;
> > + }
> > +
> > + /* Pre-allocate buffers to prevent failure in mana_attach
> > + * later
> > + */
> > + err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);
> > + if (err) {
> > + netdev_err(ndev,
> > + "Insufficient memory for new allocations\n");
> > + apc->priv_flags = old_priv_flags;
> > + return err;
> > + }
> > +
> > + err = mana_detach(ndev, false);
> > + if (err) {
> > + netdev_err(ndev, "mana_detach failed: %d\n", err);
> > + apc->priv_flags = old_priv_flags;
> > +
> > + /* Port is in an inconsistent state. Restore
> > + * 'port_is_up' so that queue reset work handler
> > + * can properly detach and re-attach.
> > + */
> > + apc->port_is_up = true;
> > + schedule_port_reset = true;
> > + goto out;
> > + }
> > +
> > + err = mana_attach(ndev);
> > + if (err) {
> > + netdev_err(ndev, "mana_attach failed: %d\n", err);
> > + apc->priv_flags = old_priv_flags;
> > +
> > + /* Restore 'port_is_up' so the reset work handler
> > + * can properly detach/attach. Without this,
> > + * the handler sees port_is_up=false and skips
> > + * queue allocation, leaving the port dead.
> > + */
> > + apc->port_is_up = true;
> > + schedule_port_reset = true;
> > + }
> > + }
> > +
> > +out:
> > + mana_pre_dealloc_rxbufs(apc);
> > +
> > + if (schedule_port_reset)
> > + queue_work(apc->ac->per_port_queue_reset_wq,
> > + &apc->queue_reset_work);
> > +
> > + return err;
> > +}
>
> [High]
> Can this failure path oops inside the reset work handler?
>
> When mana_attach() fails after mana_detach() has already torn the vport
> down, apc->tx_qp has been set to NULL by mana_destroy_txq() and apc->rxqs
> has been cleared by mana_cleanup_port_context(). This code then forces
> apc->port_is_up back to true and queues apc->queue_reset_work:
>
> apc->port_is_up = true;
> schedule_port_reset = true;
>
> When mana_per_port_queue_reset_work_handler runs it calls
> mana_detach(ndev, false), which latches port_st_save = port_is_up = true
> and, because port_st_save is true, invokes mana_dealloc_queues(). The TX
> drain loop there does:
>
> txq = &apc->tx_qp[i].txq;
> atomic_read(&txq->pending_sends);
>
> with no NULL check on apc->tx_qp, and mana_destroy_vport further down
> iterates apc->rxqs[rxq_idx] while rxqs is NULL. Would that produce a
> NULL pointer dereference in the workqueue worker whenever mana_attach()
> fails here (for example on an allocation failure in
> mana_init_port_context or mana_create_txq, or a transient HW command
> failure in mana_query_vport_cfg / mana_cfg_vport)?
>
> The mana_detach() failure path above (goto out) reaches
> mana_dealloc_queues() through the same chain and looks to have the same
> exposure.
Hi Jakub,
Thanks for your comments, I have sent a separate fixes series to the
net tree that fixes the possible NULL pointer derefernce issue and also
makes the reset handler safe for the case where it runs after a failed
attach:
https://lore.kernel.org/all/20260518194654.735580-1-dipayanroy@linux.microsoft.com/
>
> For comparison, mana_change_mtu() handles a mana_attach() failure by
> returning the error without scheduling a reset. Would a similar
> treatment here avoid the asynchronous oops, or is there a reason the
> reset must be scheduled in this specific failure case?
> --
> pw-bot: cr
The full-page-rx private flag is intended to be driven by a udev rule
that fires automatically during VM provisioning on affected platforms.
If there is a transient failure, the VM fails to provision, requiring manual
intervention.The reset handler retries the attach, giving the port a
chance to recover to default config autonomously without intervention.
Regards
Dipayaan Roy
^ permalink raw reply
* Re: [PATCH v3 29/41] x86/paravirt: Plumb a return code into __paravirt_set_sched_clock()
From: Sean Christopherson @ 2026-05-18 21:14 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-hyperv
In-Reply-To: <20260515194804.AE30FC2BCB0@smtp.kernel.org>
On Fri, May 15, 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] kvm_sched_clock_read is registered as the system sched_clock before its required offset (kvm_sched_clock_offset) is initialized.
> --
>
> commit b6e429e4990c00d845488d46e8dc4132f3a2fd8b
> Author: Sean Christopherson <seanjc@google.com>
> x86/paravirt: Plumb a return code into __paravirt_set_sched_clock()
>
> This commit adds a return code to __paravirt_set_sched_clock() to allow the
> kernel to reject attempts to use a PV sched_clock. It updates the caller
> in kvmclock to check the return value and abort initialization if needed.
>
> > diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> > index d3bb281c0805d..9b3d1ed1a96dc 100644
> > --- a/arch/x86/kernel/kvmclock.c
> > +++ b/arch/x86/kernel/kvmclock.c
> [ ... ]
> > @@ -338,10 +338,12 @@ static int kvmclock_setup_percpu(unsigned int cpu)
> >
> > static __init void kvm_sched_clock_init(bool stable)
> > {
> > + if (__paravirt_set_sched_clock(kvm_sched_clock_read, stable,
> > + kvm_save_sched_clock_state,
> > + kvm_restore_sched_clock_state))
> > + return;
> > +
> > kvm_sched_clock_offset = kvm_clock_read();
>
> Could this cause a massive backward jump in system time if an NMI or early
> exception calls sched_clock() right after registration?
>
> __paravirt_set_sched_clock() uses static_call_update() to globally publish
> kvm_sched_clock_read() as the live system sched_clock. Since the
> initialization of kvm_sched_clock_offset is now delayed until after this
> registration completes, any early caller will execute kvm_sched_clock_read()
> with a zero offset, returning a huge absolute raw clock value.
>
> When the code resumes, it sets the offset to the current raw clock value.
> Subsequent sched_clock() calls will abruptly subtract this large offset,
> causing time to jump backwards and violating monotonic clock guarantees.
>
> Would it be safer to initialize kvm_sched_clock_offset before calling
> __paravirt_set_sched_clock(), since calculating the offset early has no
> side effects even if the registration is subsequently rejected?
Hmm, yes. I'll also add a comment explaining exactly that.
^ permalink raw reply
* Re: [PATCH v3 00/41] x86: Try to wrangle PV clocks vs. TSC
From: Sean Christopherson @ 2026-05-18 21:11 UTC (permalink / raw)
To: Kiryl Shutsemau, Paolo Bonzini, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Long Li, Ajay Kaher, Alexey Makhalov,
Jan Kiszka, Dave Hansen, Andy Lutomirski, Peter Zijlstra,
Juergen Gross, Daniel Lezcano, Thomas Gleixner, John Stultz,
Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, x86, linux-coco, kvm, linux-hyperv, virtualization,
linux-kernel, xen-devel, Michael Kelley, Tom Lendacky,
Nikunj A Dadhania, Thomas Gleixner, David Woodhouse
In-Reply-To: <20260515191942.1892718-1-seanjc@google.com>
On Fri, May 15, 2026, Sean Christopherson wrote:
> Dave/Thomas/Peter/Boris, what's the going rate for bribes to take something
> like this through the tip tree?
>
> The bulk of the changes are in kvmclock and TSC, but pretty much every
> hypervisor's guest-side code gets touched at some point. I am reaonsably
> confident in the correctness of the KVM changes. Michael tested Hyper-V in
> v2, and while there were conflicts when rebasing, they were largely
> superficial (and I've just jinxed myself). For all other hypervisors, assume
> the code is compile-tested only, but those changes are all quite small and
> straightforward.
>
> The only changes that are questionable/contentious are the last two patches,
> which have KVM-as-a-guest use CPUID 0x16 to get the CPU frequency, even on
> AMD (that's the dubious part). I very deliberately put them last, so that
> they can be dropped at will (I don't care terribly if those patches land).
> To merge them, I would want explicit Acks from Paolo and David W.
>
> So, except for the last two patches, to get the stuff I really care about
> landed, I think/hope it's just the TSC and guest-side CoCo changes that need
> reviews/acks?
FYI, don't bother reviewing this version. Sashiko found several glaring flaws,
but I just realized that sashiko-bot's emails are only being sent to myself and
linux-hyperv@vger.kernel.org. I'll make sure to highlight the changes in the
next version.
In the meantime, Sashiko's feedback is archived on lore if you want to see me
get torched by AI :-)
^ permalink raw reply
* Re: [PATCH v3 36/41] x86/kvmclock: Get local APIC bus frequency from PV CPUID Timing Info
From: Sean Christopherson @ 2026-05-18 20:57 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-hyperv
In-Reply-To: <20260515195557.DEF05C2BCB0@smtp.kernel.org>
On Fri, May 15, 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] lapic_timer_period is incorrectly assigned the raw APIC frequency in kHz, which fails to account for the kernel's HZ configuration and breaks APIC timer calibration for any HZ != 1000.
> --
>
> commit e5bbef05c7ba30fb00c716accae48693e295289a
> Author: Sean Christopherson <seanjc@google.com>
>
> x86/kvmclock: Get local APIC bus frequency from PV CPUID Timing Info
>
> This commit reads the local APIC bus frequency from the PV CPUID leaf
> and uses it to directly set lapic_timer_period, bypassing the standard
> calibration process.
>
> > diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> > --- a/arch/x86/kernel/kvmclock.c
> > +++ b/arch/x86/kernel/kvmclock.c
> [ ... ]
> > @@ -200,10 +200,19 @@ void kvmclock_cpu_action(enum kvm_guest_cpu_action action)
> > */
> > static unsigned long kvm_get_tsc_khz(void)
> > {
> > +#ifdef CONFIG_X86_LOCAL_APIC
> > + u32 apic_khz = kvm_para_apic_bus_khz();
> > +
> > /*
> > - * If KVM advertises the frequency directly in CPUID, use that
> > - * instead of reverse-calculating it from the KVM clock data.
> > + * Use the TSC frequency from KVM's (and other hypervisors') PV CPUID
> > + * leaf when available, instead of reverse-calculating it from the KVM
> > + * clock data. As a bonus, the CPUID leaf also includes the local APIC
> > + * bus/timer frequency.
> > */
> > + if (apic_khz)
> > + lapic_timer_period = apic_khz;
>
> Does setting lapic_timer_period directly to apic_khz assume that HZ is
> always configured to 1000?
>
> The lapic_timer_period variable represents the number of APIC timer ticks
> per jiffy. This is usually calculated as (frequency_in_hz / HZ), which would
> be (apic_khz * 1000) / HZ.
Ugh, TIL. That's quite evil. I'll fix in the next version.
^ permalink raw reply
* Re: [PATCH v3 37/41] x86/kvmclock: Use TSC for sched_clock if it's constant and non-stop
From: Sean Christopherson @ 2026-05-18 20:28 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-hyperv
In-Reply-To: <20260515200929.19F2CC2BCB3@smtp.kernel.org>
On Fri, May 15, 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Bypassing `kvm_sched_clock_init()` when `CONSTANT_TSC` is present
> ignores KVM's `PVCLOCK_TSC_STABLE_BIT`, leading to unfiltered backward time
> jumps in the core scheduler. --
>
> commit 57cf68835ef70733c037ca9ffa04fd39e1d70027
> Author: Sean Christopherson <seanjc@google.com>
>
> x86/kvmclock: Use TSC for sched_clock if it's constant and non-stop
>
> This commit updates kvmclock_init to prefer the TSC over kvmclock for
> sched_clock when the TSC is constant, non-stop, and not marked unstable.
> It does this by bypassing kvm_sched_clock_init and instead modifying the
> TSC properties so that TSC becomes the preferred clocksource.
>
> Link: https://lore.kernel.org/all/Z4hDK27OV7wK572A@google.com
>
> > diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> > --- a/arch/x86/kernel/kvmclock.c
> > +++ b/arch/x86/kernel/kvmclock.c
> [ ... ]
> > @@ -416,22 +416,22 @@ void __init kvmclock_init(void)
> [ ... ]
> > if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
> > boot_cpu_has(X86_FEATURE_NONSTOP_TSC) &&
> > !check_tsc_unstable()) {
> > kvm_clock.rating = 299;
> > tsc_properties = TSC_FREQ_KNOWN_AND_RELIABLE;
> > + } else {
> > + kvm_sched_clock_init(stable);
> > }
> >
> > - kvm_sched_clock_init(stable);
>
> Does bypassing kvm_sched_clock_init() when CONSTANT_TSC is present
> accidentally ignore KVM's PVCLOCK_TSC_STABLE_BIT?
>
> CONSTANT_TSC indicates the hardware TSC rate doesn't change with power
> states, but it doesn't guarantee cross-CPU synchronization, such as across
> multi-socket systems or differing vCPU offsets from hotplug and migration.
> KVM explicitly signals this cross-CPU stability via PVCLOCK_TSC_STABLE_BIT,
> which is mapped to the stable variable.
>
> Prior to this patch, kvm_sched_clock_init() was always called, invoking
> __paravirt_set_sched_clock() with the stable flag. This allowed the system
> to call clear_sched_clock_stable() if the hypervisor could not guarantee
> synchronization, enabling crucial per-CPU monotonicity filters in
> sched_clock_cpu().
>
> By skipping kvm_sched_clock_init() entirely and registering
> TSC_FREQ_KNOWN_AND_RELIABLE, the kernel's native cross-CPU TSC synchronization
> checks and the clocksource watchdog appear to be bypassed.
This is very much intented behavior.
> If KVM clears PVCLOCK_TSC_STABLE_BIT, could this result in the guest
> ignoring the condition and using raw, unfiltered rdtsc() values?
Yes, that's the entire point.
Note, PVCLOCK_TSC_STABLE_BIT is still set in pvclock_read_flags(&hv_clock_boot[0].pvti),
so anything else that directly consumes PVCLOCK_TSC_STABLE_BIT still does the
right thing. E.g. kvm_setup_vsyscall_timeinfo() will still set kvm_clock's vDSO
mode to VDSO_CLOCKMODE_PVCLOCK, to communicate that kvm_lock can be used for the
vDSO page, *if* it's chose.
> When tasks migrate between vCPUs with differing TSC offsets, this regression
> might cause backward time jumps in sched_clock(), which breaks rq_clock
> monotonicity and corrupts CFS vruntime and min_vruntime calculations.
Only if the host messed up and incorrectly advertised CONSTANT+NONSTOP.
^ permalink raw reply
* [PATCH net 2/2] net: mana: Skip redundant detach in queue reset handler if already detached
From: Dipayaan Roy @ 2026-05-18 19:43 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, andrew+netdev, davem, edumazet,
kuba, pabeni, leon, longli, kotaranov, horms, shradhagupta,
ssengar, ernis, shirazsaleem, linux-hyperv, netdev, linux-kernel,
linux-rdma, stephen, jacob.e.keller, dipayanroy, leitao, kees,
john.fastabend, hawk, bpf, daniel, ast, sdf, yury.norov
In-Reply-To: <20260518194654.735580-1-dipayanroy@linux.microsoft.com>
When mana_per_port_queue_reset_work_handler() runs, it unconditionally
calls mana_detach() which attempts to tear down queues that are already
freed, leading to NULL pointer dereferences on apc->tx_qp and
apc->rxqs.
Check netif_device_present() in the reset handler and skip
mana_detach() when the port is already in detached state. This avoids
the redundant teardown and goes directly to mana_attach() to retry
bringing the port back up.
Fixes: 3b194343c250 ("net: mana: Implement ndo_tx_timeout and serialize queue resets per port.")
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Dipayaan Roy <dipayanroy@linux.microsoft.com>
---
drivers/net/ethernet/microsoft/mana/mana_en.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 0582803907a8..907efadf6fd6 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -316,12 +316,19 @@ static void mana_per_port_queue_reset_work_handler(struct work_struct *work)
goto out;
}
+ /* If already detached (indicates detach succeeded but attach failed
+ * previously). Now skip mana detach and just retry mana_attach.
+ */
+ if (!netif_device_present(ndev))
+ goto attach;
+
err = mana_detach(ndev, false);
if (err) {
netdev_err(ndev, "mana_detach failed: %d\n", err);
goto dealloc_pre_rxbufs;
}
+attach:
err = mana_attach(ndev);
if (err)
netdev_err(ndev, "mana_attach failed: %d\n", err);
--
2.43.0
^ permalink raw reply related
* [PATCH net 1/2] net: mana: Add NULL guards in teardown path to prevent panic on attach failure
From: Dipayaan Roy @ 2026-05-18 19:43 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, andrew+netdev, davem, edumazet,
kuba, pabeni, leon, longli, kotaranov, horms, shradhagupta,
ssengar, ernis, shirazsaleem, linux-hyperv, netdev, linux-kernel,
linux-rdma, stephen, jacob.e.keller, dipayanroy, leitao, kees,
john.fastabend, hawk, bpf, daniel, ast, sdf, yury.norov
In-Reply-To: <20260518194654.735580-1-dipayanroy@linux.microsoft.com>
When queue allocation fails partway through, the error cleanup frees
and NULLs apc->tx_qp and apc->rxqs. Multiple teardown paths such as
mana_remove(), mana_change_mtu() recovery, and internal error handling
in mana_alloc_queues() can subsequently call into functions that
dereference these pointers without NULL checks:
- mana_chn_setxdp() dereferences apc->rxqs[0], causing a NULL pointer
dereference panic (CR2: 0000000000000000 at mana_chn_setxdp+0x26).
- mana_destroy_vport() iterates apc->rxqs without a NULL check.
- mana_fence_rqs() iterates apc->rxqs without a NULL check.
- mana_dealloc_queues() iterates apc->tx_qp without a NULL check.
Add NULL guards for apc->rxqs in mana_fence_rqs(),
mana_destroy_vport(), and before the mana_chn_setxdp() call. Add a
NULL guard for apc->tx_qp in mana_dealloc_queues() to skip TX queue
draining when TX queues were never allocated or already freed.
Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Dipayaan Roy <dipayanroy@linux.microsoft.com>
---
drivers/net/ethernet/microsoft/mana/mana_en.c | 70 +++++++++++--------
1 file changed, 41 insertions(+), 29 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 9afc786b297a..0582803907a8 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -1727,6 +1727,9 @@ static void mana_fence_rqs(struct mana_port_context *apc)
struct mana_rxq *rxq;
int err;
+ if (!apc->rxqs)
+ return;
+
for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
rxq = apc->rxqs[rxq_idx];
err = mana_fence_rq(apc, rxq);
@@ -2858,13 +2861,16 @@ static void mana_destroy_vport(struct mana_port_context *apc)
struct mana_rxq *rxq;
u32 rxq_idx;
- for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
- rxq = apc->rxqs[rxq_idx];
- if (!rxq)
- continue;
+ if (apc->rxqs) {
- mana_destroy_rxq(apc, rxq, true);
- apc->rxqs[rxq_idx] = NULL;
+ for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
+ rxq = apc->rxqs[rxq_idx];
+ if (!rxq)
+ continue;
+
+ mana_destroy_rxq(apc, rxq, true);
+ apc->rxqs[rxq_idx] = NULL;
+ }
}
mana_destroy_txq(apc);
@@ -3269,7 +3275,8 @@ static int mana_dealloc_queues(struct net_device *ndev)
if (apc->port_is_up)
return -EINVAL;
- mana_chn_setxdp(apc, NULL);
+ if (apc->rxqs)
+ mana_chn_setxdp(apc, NULL);
if (gd->gdma_context->is_pf && !apc->ac->bm_hostmode)
mana_pf_deregister_filter(apc);
@@ -3287,33 +3294,38 @@ static int mana_dealloc_queues(struct net_device *ndev)
* number of queues.
*/
- for (i = 0; i < apc->num_queues; i++) {
- txq = &apc->tx_qp[i].txq;
- tsleep = 1000;
- while (atomic_read(&txq->pending_sends) > 0 &&
- time_before(jiffies, timeout)) {
- usleep_range(tsleep, tsleep + 1000);
- tsleep <<= 1;
- }
- if (atomic_read(&txq->pending_sends)) {
- err = pcie_flr(to_pci_dev(gd->gdma_context->dev));
- if (err) {
- netdev_err(ndev, "flr failed %d with %d pkts pending in txq %u\n",
- err, atomic_read(&txq->pending_sends),
- txq->gdma_txq_id);
+ if (apc->tx_qp) {
+ for (i = 0; i < apc->num_queues; i++) {
+ txq = &apc->tx_qp[i].txq;
+ tsleep = 1000;
+ while (atomic_read(&txq->pending_sends) > 0 &&
+ time_before(jiffies, timeout)) {
+ usleep_range(tsleep, tsleep + 1000);
+ tsleep <<= 1;
+ }
+ if (atomic_read(&txq->pending_sends)) {
+ err =
+ pcie_flr(to_pci_dev(gd->gdma_context->dev));
+ if (err) {
+ netdev_err(ndev, "flr failed %d with %d pkts pending in txq %u\n",
+ err,
+ atomic_read(&txq->pending_sends),
+ txq->gdma_txq_id);
+ }
+ break;
}
- break;
}
- }
- for (i = 0; i < apc->num_queues; i++) {
- txq = &apc->tx_qp[i].txq;
- while ((skb = skb_dequeue(&txq->pending_skbs))) {
- mana_unmap_skb(skb, apc);
- dev_kfree_skb_any(skb);
+ for (i = 0; i < apc->num_queues; i++) {
+ txq = &apc->tx_qp[i].txq;
+ while ((skb = skb_dequeue(&txq->pending_skbs))) {
+ mana_unmap_skb(skb, apc);
+ dev_kfree_skb_any(skb);
+ }
+ atomic_set(&txq->pending_sends, 0);
}
- atomic_set(&txq->pending_sends, 0);
}
+
/* We're 100% sure the queues can no longer be woken up, because
* we're sure now mana_poll_tx_cq() can't be running.
*/
--
2.43.0
^ permalink raw reply related
* [PATCH net 0/2] net: mana: Fix NULL dereferences during teardown after attach failure.
From: Dipayaan Roy @ 2026-05-18 19:43 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, andrew+netdev, davem, edumazet,
kuba, pabeni, leon, longli, kotaranov, horms, shradhagupta,
ssengar, ernis, shirazsaleem, linux-hyperv, netdev, linux-kernel,
linux-rdma, stephen, jacob.e.keller, dipayanroy, leitao, kees,
john.fastabend, hawk, bpf, daniel, ast, sdf, yury.norov
When mana_attach() fails (e.g. during queue allocation), the error
cleanup frees apc->tx_qp and apc->rxqs and sets them to NULL. Multiple
subsequent teardown paths can then dereference these NULL pointers,
causing kernel panics.
Patch 1 adds NULL guards in the low-level teardown functions
(mana_fence_rqs, mana_destroy_vport, mana_dealloc_queues) so they are
safe to call regardless of queue initialization state. This covers all
callers: mana_remove(), mana_change_mtu() recovery, and internal error
paths in mana_alloc_queues().
Patch 2 addresses the queue reset work handler specifically, where an
unconditional mana_detach() on an already-detached port caused
redundant teardown. It checks netif_device_present() to skip the detach
and directly retry mana_attach().
Dipayaan Roy (2):
net: mana: Add NULL guards in teardown path to prevent panic on attach
failure
net: mana: Skip redundant detach in queue reset handler if already
detached
drivers/net/ethernet/microsoft/mana/mana_en.c | 77 ++++++++++++-------
1 file changed, 48 insertions(+), 29 deletions(-)
--
2.43.0
^ permalink raw reply
* Re: [PATCH v3 32/41] x86/tsc: Rejects attempts to override TSC calibration with lesser routine
From: Sean Christopherson @ 2026-05-18 19:17 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-hyperv
In-Reply-To: <20260515201628.9E3C5C2BCB3@smtp.kernel.org>
On Fri, May 15, 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Spurious WARN_ON stack trace on every boot for CoCo (SEV-SNP/TDX) guests under KVM.
> --
>
> commit e57255e4d076143a67d0e6e44f914aec31aed8c7
> Author: Sean Christopherson <seanjc@google.com>
>
> x86/tsc: Rejects attempts to override TSC calibration with lesser routine
>
> This commit adds a sanity check when registering a TSC frequency calibration
> routine to ensure the incoming routine is as robust as the outgoing one,
> rejecting it with a warning if it is not. This prevents replacing a trusted
> calibration source with a less trusted one.
>
> > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > index 98bef1d06fa9d..7a261214fa3ef 100644
> > --- a/arch/x86/kernel/tsc.c
> > +++ b/arch/x86/kernel/tsc.c
> > @@ -1319,8 +1319,13 @@ void tsc_register_calibration_routines(unsigned long (*calibrate_tsc)(void),
> >
> > if (properties & TSC_FREQUENCY_KNOWN)
> > setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
> > + else if (WARN_ON(boot_cpu_has(X86_FEATURE_TSC_KNOWN_FREQ)))
> > + return;
> > +
> > if (properties & TSC_RELIABLE)
> > setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
> > + else if (WARN_ON(boot_cpu_has(X86_FEATURE_TSC_RELIABLE)))
> > + return;
>
> Will this cause a spurious warning stack trace on every boot for CoCo
> (SEV-SNP/TDX) guests under KVM?
>
> For KVM CoCo guests, CoCo registers first via tsc_early_init(), setting both
> TSC_FREQUENCY_KNOWN and X86_FEATURE_TSC_RELIABLE.
>
> KVM then registers second via kvmclock_init(). At this point in the patch
> series, KVM only passes TSC_FREQUENCY_KNOWN, so the properties & TSC_RELIABLE
> check is false, and it falls through to this WARN_ON. Since CoCo already set
> the capability, the condition evaluates to true.
>
> I see this is fixed later in the series by the commit "x86/kvmclock: Mark TSC
> as reliable when it's constant and nonstop", which modifies KVM to pass
> TSC_FREQ_KNOWN_AND_RELIABLE on modern hardware, preventing this warning.
>
> Could the patch ordering be adjusted to prevent this intermediate regression?
Ah shoot, no. Well, not entirely. Because when running as a CoCo guest with a
trusted TSC, the kernel needs to prevent overwriting the TSC calibration, *period*.
I.e. changing the ordering will eliminate the unwanted WARN, but it won't fix the
underlying goof that the trusted calibration routines are still being overwritten
with untrusted routines.
Hrm, the SNP secure TSC code complicates things, but I suspect it's broken. If
it's indeed broken, then I think we can add a TSC_TRUSTED flag and then do:
if (WARN_ON_ONCE(!calibrate_tsc))
return;
if (properties & TSC_FREQUENCY_KNOWN)
setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
else if (WARN_ON(boot_cpu_has(X86_FEATURE_TSC_KNOWN_FREQ)))
return;
if (properties & TSC_RELIABLE)
setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
else if (WARN_ON(boot_cpu_has(X86_FEATURE_TSC_RELIABLE)))
return;
if (!cpu_has_trusted_tsc() || (properties & TSC_TRUSTED))
x86_platform.calibrate_tsc = calibrate_tsc;
if (calibrate_cpu)
x86_platform.calibrate_cpu = calibrate_cpu;
Tom / Nikunj,
Isn't it completely wrong to assume the CPU frequency is the same as the TSC
frequency? The changelog says the difference "does not apply", but that makes
no sense.
Use the GUEST_TSC_FREQ MSR to discover the TSC frequency instead of
relying on kvm-clock based frequency calibration. Override both CPU and
TSC frequency calibration callbacks with securetsc_get_tsc_khz(). Since
the difference between CPU base and TSC frequency does not apply in this
case, the same callback is being used.
E.g. if the host passed through APERF/MPERF, then the difference most definitely
applies. If TSC != CPU frequency, then knowingly using bad data is even worse
(far, far worse) than hoping the untrusted host is playing nice.
If the TSC and CPU frequencies are somehow guaranteed to be the same (which I
can't possibly imagine is the case), then the above won't work because we also
want to prevent overriding calibrate_cpu().
^ permalink raw reply
* Re: [PATCH net] net: mana: Fix TOCTOU double-fetch of hwc_msg_id from DMA buffer
From: Erni Sri Satya Vennela @ 2026-05-18 11:38 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-hyperv
In-Reply-To: <20260515194427.52CACC2BCB0@smtp.kernel.org>
> > @@ -269,13 +268,17 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
> > rx_req = &hwc_rxq->msg_buf->reqs[rx_req_idx];
>
> Since this patch addresses host-controlled DMA memory vulnerabilities in
> Confidential VMs, could there be another out-of-bounds access just before
> the msg_id check?
>
> Looking at how rx_req_idx is calculated earlier in this function:
>
> sge = (struct gdma_sge *)(wqe + 8 + dma_oob->inline_oob_size_div4 * 4);
> rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
> rx_req_idx = (sge->address - rq_base_addr) / hwc->max_req_msg_size;
>
> Because sge->address is read directly from the untrusted DMA queue, a malicious
> host could provide an arbitrary value, resulting in an out-of-bounds
> rx_req_idx.
>
> Using this unvalidated index to access hwc_rxq->msg_buf->reqs[rx_req_idx]
> yields an attacker-controlled rx_req pointer. When this corrupted rx_req is
> subsequently passed to mana_hwc_post_rx_wqe(), the driver writes to req->sge
> and req->wqe_req, which could allow arbitrary writes to guest kernel memory.
>
> Should rx_req_idx be bounds-checked against the size of the reqs array
> before it is used to index the array?
>
> > resp = (struct gdma_resp_hdr *)rx_req->buf_va;
> >
> > - if (resp->response.hwc_msg_id >= hwc->num_inflight_msg) {
> > - dev_err(hwc->dev, "HWC RX: wrong msg_id=%u\n",
> > - resp->response.hwc_msg_id);
> > + /* Read msg_id once from DMA buffer to prevent TOCTOU:
> > + * DMA memory is shared/unencrypted in CVMs - host can
> > + * modify it between reads.
> > + */
> > + msg_id = READ_ONCE(resp->response.hwc_msg_id);
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260514194156.466823-1-ernis@linux.microsoft.com?part=1
This issue is already addressed in a separate patch by Aditya Garg
<gargaditya@linux.microsoft.com>, which is currently under internal
review.
- Vennela
^ permalink raw reply
* Re: [PATCH v1 3/4] iommu/hyperv: Add para-virtualized IOMMU support for Hyper-V guest
From: Yu Zhang @ 2026-05-18 9:38 UTC (permalink / raw)
To: Mukesh R
Cc: Michael Kelley, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org, iommu@lists.linux.dev,
linux-pci@vger.kernel.org, linux-arch@vger.kernel.org,
wei.liu@kernel.org, kys@microsoft.com, haiyangz@microsoft.com,
decui@microsoft.com, longli@microsoft.com, joro@8bytes.org,
will@kernel.org, robin.murphy@arm.com, bhelgaas@google.com,
kwilczynski@kernel.org, lpieralisi@kernel.org, mani@kernel.org,
robh@kernel.org, arnd@arndb.de, jgg@ziepe.ca,
jacob.pan@linux.microsoft.com, tgopinath@linux.microsoft.com,
easwar.hariharan@linux.microsoft.com
In-Reply-To: <53754e0b-2af8-edd2-dfc0-293fac002a52@linux.microsoft.com>
On Fri, May 15, 2026 at 05:11:19PM -0700, Mukesh R wrote:
> On 5/15/26 09:53, Yu Zhang wrote:
> > On Fri, May 15, 2026 at 02:51:38PM +0000, Michael Kelley wrote:
> > > From: Yu Zhang <zhangyu1@linux.microsoft.com> Sent: Friday, May 15, 2026 7:00 AM
> > > >
> > > > On Thu, May 14, 2026 at 06:13:24PM +0000, Michael Kelley wrote:
> > > > > From: Yu Zhang <zhangyu1@linux.microsoft.com> Sent: Monday, May 11, 2026 9:24 AM
> > > > > >
> > > > > > Add a para-virtualized IOMMU driver for Linux guests running on Hyper-V.
> > > > > > This driver implements stage-1 IO translation within the guest OS.
> > > > > > It integrates with the Linux IOMMU core, utilizing Hyper-V hypercalls
> > > > > > for:
> > > > > > - Capability discovery
> > > > > > - Domain allocation, configuration, and deallocation
> > > > > > - Device attachment and detachment
> > > > > > - IOTLB invalidation
> > > > > >
> > > > > > The driver constructs x86-compatible stage-1 IO page tables in the
> > > > > > guest memory using consolidated IO page table helpers. This allows
> > > > > > the guest to manage stage-1 translations independently of vendor-
> > > > > > specific drivers (like Intel VT-d or AMD IOMMU).
> > > > > >
> > > > > > Hyper-V consumes this stage-1 IO page table when a device domain is
> > > > > > created and configured, and nests it with the host's stage-2 IO page
> > > > > > tables, therefore eliminating the VM exits for guest IOMMU mapping
> > > > > > operations. For unmapping operations, VM exits to perform the IOTLB
> > > > > > flush are still unavoidable.
> > > > > >
> > > > > > Hyper-V identifies each PCI pass-thru device by a logical device ID
> > > > > > in its hypercall interface. The vPCI driver (pci-hyperv) registers the
> > > > > > per-bus portion of this ID with the pvIOMMU driver during bus probe.
> > > > > > The pvIOMMU driver stores this mapping and combines it with the function
> > > > > > number of the endpoint PCI device to form the complete ID for hypercalls.
> > > > >
> > > > > As you are probably aware, Mukesh's patch series to support PCI
> > > > > pass-thru devices also needs to get the logical device ID. Maybe the
> > > > > registration mechanism needs to move somewhere that can be shared
> > > > > with his code.
> > > > >
> > > >
> > > > Thank you so much for the review, Michael!
> > > >
> > > > Yes, I looked at Mukesh's series and noticed his hv_pci_vmbus_device_id()
> > > > in pci-hyperv.c has the same dev_instance byte manipulation. We do need
> > > > a common registration mechanism.
> > > >
> > > > Any suggestion on where to put it? drivers/hv/hv_common.c seems like a
> > > > natural place, but the register/lookup functions are currently only
> > > > meaningful when CONFIG_HYPERV_PVIOMMU is set. If Mukesh's pass-thru
> > > > code also needs them, we might need a new shared Kconfig option that
> > > > both can select. Open to better ideas.
> > >
> > > Unfortunately, I have not looked at Mukesh's series in detail yet, so
> > > I don't have enough knowledge of the full situation to offer a good
> > > recommendation.
> > >
> >
> > Sorry I forgot to Cc Mukesh in the previous reply. :(
> > @Mukesh, any thoughts on sharing the logical device ID registration mechanism?
>
> Yeah, I went round and round trying to find the best place. I almost
> created virt/hyperv/hv_utils.c file. Maybe that is the best place?
Thanks for thinking about this, Mukesh!
I'm a bit hesitant about introducing virt/hyperv/. Currently virt/
only hosts KVM's architecture-neutral hypervisor core. And it feels
like the wrong layer for driver-level utility code. And drivers/hv/
seems like a more natural fit?
I'm also thinking about the config to gating these new interfaces(
register/lookup etc.), I am using CONFIG_HYPERV_PVIOMMU, and I guess
you may propably propose another one for the host side change(or just
CONFIG_MSHV_ROOT)?
B.R.
Yu
>
> Thanks,
> -Mukesh
>
>
> > > >
> > > > [...]
> > > >
> > > > > > +static void hv_flush_device_domain(struct hv_iommu_domain *hv_domain)
> > > > > > +{
> > > > > > + u64 status;
> > > > > > + unsigned long flags;
> > > > > > + struct hv_input_flush_device_domain *input;
> > > > > > +
> > > > > > + local_irq_save(flags);
> > > > > > +
> > > > > > + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> > > > > > + memset(input, 0, sizeof(*input));
> > > > > > + input->device_domain = hv_domain->device_domain;
> > > > >
> > > > > The previous version of this patch had code to set several other fields in
> > > > > the input. I wanted to confirm that not setting them in this version is
> > > > > intentional. Were they not needed?
> > > > >
> > > >
> > > > Oh. The RFC v1 set partition_id, owner_vtl, domain_id.type, and domain_id.id
> > > > individually. In this version, I just simplified it to a struct assignment.
> > > > No functional change.
> > >
> > > Of course! I should have looked more closely at the details before making
> > > this comment. :-(
> > >
> > > [...]
> > >
> > > > >
> > > > > Previous versions of this function did hv_iommu_detach_dev(). With that call
> > > > > removed from here, hv_iommu_detach_dev() is only called when attaching a
> > > > > domain to a device that already has a domain attached. Is it the case that
> > > > > Hyper-V doesn't require the detach as a cleanup step?
> > > > >
> > > >
> > > > The IOMMU core attaches the device to release_domain (our blocking domain)
> > > > before calling release_device(), so I believe the explicit detach in the RFC
> > > > was redundant. I simply didn't realize that at the time.
> > > >
> > >
> > > Got it. But after the IOMMU core attaches the device to the blocking
> > > domain, there's the possibility that the vPCI device is rescinded by
> > > Hyper-V and it goes away entirely. Or the device might be subjected
> > > to an "unbind/bind" cycle in Linux. Does the detach need to be done
> > > on the blocking domain in such cases? In this version of the patches, the
> > > Hyper-V "attach" and "detach" hypercalls still end up unbalanced. That
> > > seems a bit untidy at best, and I wonder if there are scenarios where
> > > Hyper-V will complain about the lack of balance.
> > >
> >
> > Thank you, Michael. May I ask what "the vPCI device is rescinded by
> > Hyper-V and it goes away entirely" mean?
> >
> > I realized it's a bit untidy. But I want to understand this issue more
> > clearly first. :)
> >
> > B.R.
> > Yu
>
^ permalink raw reply
* Re: [PATCH net v2] net: mana: Optimize irq affinity for low vcpu configs
From: Erni Sri Satya Vennela @ 2026-05-18 7:04 UTC (permalink / raw)
To: Yury Norov
Cc: Shradha Gupta, Dexuan Cui, Wei Liu, Haiyang Zhang,
K. Y. Srinivasan, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Konstantin Taranov, Simon Horman,
Dipayaan Roy, Shiraz Saleem, Michael Kelley, Long Li, Yury Norov,
linux-hyperv, linux-kernel, netdev, Paul Rosswurm, Shradha Gupta,
Saurabh Singh Sengar, stable
In-Reply-To: <af4X_52txN28b9RV@yury>
> > But one observation I had was that " irq_set_affinity_and_hint(*irqs++,
> > NULL);" is essentially a no-op and we end up relying on the initial
> > placement from pci_alloc_irq_vectors().
>
> Yes you are, assuming you're not binding them before in your call chain.
>
> > Even though in these tests we
> > were not able to reproduce it, but with this distribution there is a
> > chance we end up clustering the mana queue IRQs, while other vCPUs are
> > not running any network load.
>
> That sounds like an IRQ balancer bug which you're unable to reproduce.
>
> > It's because the placement depends on
> > system-wide IRQ state at allocation time.
>
> I don't understand this point. The
>
> irq_set_affinity_and_hint(*irqs++, NULL);
>
> simply means: I trust system IRQ balancer to pick the best CPU for my
> IRQ at runtime. It doesn't refer any "IRQ state at allocation time".
>
> > The linear approach however gaurantees each queue IRQ lands on a
> > distinct vCPU regardless of system state. Even after stressing the cpus
> > using stress-ng, we did not observe any significant throughput drop.
>
> If you just do nothing, it would lead to the same numbers, right? What
> does that "non-significant throughput drop" mean? It sounds like the
> linear approach is slightly worse.
The numbers are not worse, they almost same in both the cases.
>
> --
>
> So, as you can't demonstrate solid benefit for the 'linear' IRQ placement,
> I would just stick to the no-affinity logic.
Thankyou Yury,
We are investigating on more test scenarios and trying to
capture numbers with both, your proposed change and the one from this
patch. We will keep you updated about the results.
- Vennela
^ permalink raw reply
* Re: [PATCH v2 2/2] drm/hyperv: validate VMBus packet size in receive callback
From: sashiko-bot @ 2026-05-17 15:13 UTC (permalink / raw)
To: Berkant Koc; +Cc: linux-hyperv
In-Reply-To: <20260517-drm-hyperv-patch2-v2@berkoc.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incomplete validation of synchronous response payload sizes causes state leakage and deterministic false success.
--
commit 31b0cdbb563323a78bd6df8c18d507462a03a084
Author: Berkant Koc <me@berkoc.com>
drm/hyperv: validate VMBus packet size in receive callback
This commit updates the hyperv_receive callback to validate the received
packet size before processing it. It ensures that the packet contains at
least the message headers, passes bytes_recvd to the sub-handler, and
uses it to bound the memcpy for synchronous responses.
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> index 3b5065fe06e46..cdab4895dd404 100644
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
[ ... ]
> @@ -423,26 +423,35 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)
> return 0;
> }
>
> -static void hyperv_receive_sub(struct hv_device *hdev)
> +static void hyperv_receive_sub(struct hv_device *hdev, u32 bytes_recvd)
> {
> struct hyperv_drm_device *hv = hv_get_drvdata(hdev);
> struct synthvid_msg *msg;
> + size_t hdr_size;
>
> if (!hv)
> return;
>
> + hdr_size = sizeof(struct pipe_msg_hdr) +
> + sizeof(struct synthvid_msg_hdr);
> + if (bytes_recvd < hdr_size || bytes_recvd > VMBUS_MAX_PACKET_SIZE)
> + return;
> +
> msg = (struct synthvid_msg *)hv->recv_buf;
>
> /* Complete the wait event */
> if (msg->vid_hdr.type == SYNTHVID_VERSION_RESPONSE ||
> msg->vid_hdr.type == SYNTHVID_RESOLUTION_RESPONSE ||
> msg->vid_hdr.type == SYNTHVID_VRAM_LOCATION_ACK) {
> - memcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE);
> + memcpy(hv->init_buf, msg, bytes_recvd);
Is it possible for a truncated response to incorrectly report success here?
Since hv->init_buf is reused for both sending requests and receiving responses,
and their structures alias each other in the synthvid_msg union, a short packet
would leave the payload section of hv->init_buf unmodified.
For example, if a truncated SYNTHVID_VRAM_LOCATION_ACK packet contains only
headers, memcpy() will only copy the headers. The request's vram.user_ctx and
the response's vram_ack.user_ctx reside at the exact same memory offset in the
union. Because the payload in hv->init_buf is unmodified, msg->vram_ack.user_ctx
will perfectly retain the request's vram_pp value.
The waiting thread checks this with:
if (msg->vram_ack.user_ctx != vram_pp)
Because the memory was unchanged, this check will deterministically evaluate
to false, causing the driver to proceed as if the VRAM location was
successfully acknowledged despite the malformed packet.
Should this code validate that bytes_recvd is large enough to contain the
specific response type's payload before copying and calling complete()?
> complete(&hv->wait);
> return;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260517-drm-hyperv-cover-v2@berkoc.com?part=2
^ permalink raw reply
* Re: [PATCH v2 1/2] drm/hyperv: validate resolution_count and harden VSP request paths
From: sashiko-bot @ 2026-05-17 14:47 UTC (permalink / raw)
To: Berkant Koc; +Cc: linux-hyperv
In-Reply-To: <20260517-drm-hyperv-patch1-v2@berkoc.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The fix for delayed VSP responses is incomplete, leaving a critical data race on the shared `hv->init_buf` that allows type confusion and out-of-bounds memory reads.
--
commit 3d049f39a946f7898e25e92a422ae95b066e9989
Author: Berkant Koc <me@berkoc.com>
drm/hyperv: validate resolution_count and harden VSP request paths
This commit adds bounds checking for resolution_count when parsing
SYNTHVID_RESOLUTION_RESPONSE. It degrades gracefully to default
resolutions on failure, and adds reinit_completion() before VSP requests
to prevent consuming stale completions.
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
[ ... ]
> @@ -383,6 +385,7 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)
> sizeof(struct synthvid_supported_resolution_req);
> msg->resolution_req.maximum_resolution_count =
> SYNTHVID_MAX_RESOLUTION_COUNT;
> + reinit_completion(&hv->wait);
> hyperv_sendpacket(hdev, msg);
Does this reinit_completion() prevent a data race on the shared hv->init_buf?
If a timed-out request's delayed response arrives while a new request is
being constructed, could hyperv_receive_sub() concurrently overwrite
hv->init_buf without holding any locks?
Since the buffer is used for both sending and receiving, if a delayed
response arrives exactly between the memset(msg, 0) and
hyperv_sendpacket(hdev, msg), could msg->vid_hdr.size be overwritten by
the host's response?
If a host provides a crafted size, could this cause an integer overflow in
totalbytes_towrite within hv_ringbuffer_write(), bypassing ring buffer
free-space checks and causing an out-of-bounds read of guest memory into
the VMBus ring buffer?
Additionally, if the delayed response triggers complete(&hv->wait), is
there a chance the driver parses a mismatched response because it does not
validate that msg->vid_hdr.type matches the expected response?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260517-drm-hyperv-cover-v2@berkoc.com?part=1
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox