* [PATCH v4 1/3] hyperv: Add definitions for MSHV sleep state configuration
From: Praveen K Paladugu @ 2025-11-07 22:16 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, tglx, mingo, linux-hyperv,
linux-kernel, bp, dave.hansen, x86, hpa, arnd
Cc: anbelski, prapal, easwar.hariharan, nunodasneves, skinsburskii,
mhklinux
In-Reply-To: <20251107221700.45957-1-prapal@linux.microsoft.com>
Add the definitions required to configure sleep states in mshv hypervsior.
Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com>
Co-developed-by: Anatol Belski <anbelski@linux.microsoft.com>
Signed-off-by: Anatol Belski <anbelski@linux.microsoft.com>
Reviewed-by: Easwar Hariharan <easwar.hariharan@linux.microsoft.com>
Reviewed-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
---
include/hyperv/hvgdk_mini.h | 4 +++-
include/hyperv/hvhdk_mini.h | 33 +++++++++++++++++++++++++++++++++
2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
index 7499a679e60a..b43fa1c9ed2c 100644
--- a/include/hyperv/hvgdk_mini.h
+++ b/include/hyperv/hvgdk_mini.h
@@ -465,19 +465,21 @@ union hv_vp_assist_msr_contents { /* HV_REGISTER_VP_ASSIST_PAGE */
#define HVCALL_RESET_DEBUG_SESSION 0x006b
#define HVCALL_MAP_STATS_PAGE 0x006c
#define HVCALL_UNMAP_STATS_PAGE 0x006d
+#define HVCALL_SET_SYSTEM_PROPERTY 0x006f
#define HVCALL_ADD_LOGICAL_PROCESSOR 0x0076
#define HVCALL_GET_SYSTEM_PROPERTY 0x007b
#define HVCALL_MAP_DEVICE_INTERRUPT 0x007c
#define HVCALL_UNMAP_DEVICE_INTERRUPT 0x007d
#define HVCALL_RETARGET_INTERRUPT 0x007e
#define HVCALL_NOTIFY_PARTITION_EVENT 0x0087
+#define HVCALL_ENTER_SLEEP_STATE 0x0084
#define HVCALL_NOTIFY_PORT_RING_EMPTY 0x008b
#define HVCALL_REGISTER_INTERCEPT_RESULT 0x0091
#define HVCALL_ASSERT_VIRTUAL_INTERRUPT 0x0094
#define HVCALL_CREATE_PORT 0x0095
#define HVCALL_CONNECT_PORT 0x0096
#define HVCALL_START_VP 0x0099
-#define HVCALL_GET_VP_INDEX_FROM_APIC_ID 0x009a
+#define HVCALL_GET_VP_INDEX_FROM_APIC_ID 0x009a
#define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE 0x00af
#define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_LIST 0x00b0
#define HVCALL_SIGNAL_EVENT_DIRECT 0x00c0
diff --git a/include/hyperv/hvhdk_mini.h b/include/hyperv/hvhdk_mini.h
index f2d7b50de7a4..06cf30deb319 100644
--- a/include/hyperv/hvhdk_mini.h
+++ b/include/hyperv/hvhdk_mini.h
@@ -140,6 +140,7 @@ enum hv_snp_status {
enum hv_system_property {
/* Add more values when needed */
+ HV_SYSTEM_PROPERTY_SLEEP_STATE = 3,
HV_SYSTEM_PROPERTY_SCHEDULER_TYPE = 15,
HV_DYNAMIC_PROCESSOR_FEATURE_PROPERTY = 21,
HV_SYSTEM_PROPERTY_CRASHDUMPAREA = 47,
@@ -155,6 +156,19 @@ union hv_pfn_range { /* HV_SPA_PAGE_RANGE */
} __packed;
};
+enum hv_sleep_state {
+ HV_SLEEP_STATE_S1 = 1,
+ HV_SLEEP_STATE_S2 = 2,
+ HV_SLEEP_STATE_S3 = 3,
+ HV_SLEEP_STATE_S4 = 4,
+ HV_SLEEP_STATE_S5 = 5,
+ /*
+ * After hypervisor has received this, any follow up sleep
+ * state registration requests will be rejected.
+ */
+ HV_SLEEP_STATE_LOCK = 6
+};
+
enum hv_dynamic_processor_feature_property {
/* Add more values when needed */
HV_X64_DYNAMIC_PROCESSOR_FEATURE_MAX_ENCRYPTED_PARTITIONS = 13,
@@ -184,6 +198,25 @@ struct hv_output_get_system_property {
};
} __packed;
+struct hv_sleep_state_info {
+ u32 sleep_state; /* enum hv_sleep_state */
+ u8 pm1a_slp_typ;
+ u8 pm1b_slp_typ;
+} __packed;
+
+struct hv_input_set_system_property {
+ u32 property_id; /* enum hv_system_property */
+ u32 reserved;
+ union {
+ /* More fields to be filled in when needed */
+ struct hv_sleep_state_info set_sleep_state_info;
+ };
+} __packed;
+
+struct hv_input_enter_sleep_state { /* HV_INPUT_ENTER_SLEEP_STATE */
+ u32 sleep_state; /* enum hv_sleep_state */
+} __packed;
+
struct hv_input_map_stats_page {
u32 type; /* enum hv_stats_object_type */
u32 padding;
--
2.51.0
^ permalink raw reply related
* [PATCH v4 0/3] Add support for clean shutdown with MSHV
From: Praveen K Paladugu @ 2025-11-07 22:16 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, tglx, mingo, linux-hyperv,
linux-kernel, bp, dave.hansen, x86, hpa, arnd
Cc: anbelski, prapal, easwar.hariharan, nunodasneves, skinsburskii,
mhklinux
Add support for clean shutdown of the root partition when running on
MSHV Hypervisor.
v4:
- Adopted machine_ops to order invoking HV_ENTER_SLEEP_STATE as the
last step in shutdown sequence.
- This ensures rest of the cleanups are done before powering off
v3:
- Dropped acpi_sleep handlers as they are not used on mshv
- Applied ordering for hv_reboot_notifier
- Fixed build issues on i386, arm64 architectures
v2:
- Addressed review comments from v1.
- Moved all sleep state handling methods under CONFIG_ACPI stub
- - This fixes build issues on non-x86 architectures.
Praveen K Paladugu (3):
hyperv: Add definitions for MSHV sleep state configuration
hyperv: Use reboot notifier to configure sleep state
hyperv: Cleanly shutdown root partition with MSHV
arch/x86/hyperv/hv_init.c | 9 +++
arch/x86/include/asm/mshyperv.h | 4 ++
drivers/hv/mshv_common.c | 99 +++++++++++++++++++++++++++++++++
include/hyperv/hvgdk_mini.h | 4 +-
include/hyperv/hvhdk_mini.h | 33 +++++++++++
5 files changed, 148 insertions(+), 1 deletion(-)
--
2.51.0
^ permalink raw reply
* Re: [PATCH] mshv_eventfd: add WQ_PERCPU to alloc_workqueue users
From: Easwar Hariharan @ 2025-11-07 18:54 UTC (permalink / raw)
To: Marco Crivellari
Cc: linux-kernel, linux-hyperv, easwar.hariharan, Tejun Heo,
Lai Jiangshan, Frederic Weisbecker, Sebastian Andrzej Siewior,
Michal Hocko, K . Y . Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui
In-Reply-To: <20251107132712.182499-1-marco.crivellari@suse.com>
On 11/7/2025 5:27 AM, Marco Crivellari wrote:
> Currently if a user enqueues a work item using schedule_delayed_work() the
> used wq is "system_wq" (per-cpu wq) while queue_delayed_work() use
> WORK_CPU_UNBOUND (used when a cpu is not specified). The same applies to
> schedule_work() that is using system_wq and queue_work(), that makes use
> again of WORK_CPU_UNBOUND.
> This lack of consistency cannot be addressed without refactoring the API.
>
> alloc_workqueue() treats all queues as per-CPU by default, while unbound
> workqueues must opt-in via WQ_UNBOUND.
>
> This default is suboptimal: most workloads benefit from unbound queues,
> allowing the scheduler to place worker threads where they’re needed and
> reducing noise when CPUs are isolated.
>
> This continues the effort to refactor workqueue APIs, which began with
> the introduction of new workqueues and a new alloc_workqueue flag in:
>
> commit 128ea9f6ccfb ("workqueue: Add system_percpu_wq and system_dfl_wq")
> commit 930c2ea566af ("workqueue: Add new WQ_PERCPU flag")
>
> This change adds a new WQ_PERCPU flag to explicitly request
> alloc_workqueue() to be per-cpu when WQ_UNBOUND has not been specified.
>
> With the introduction of the WQ_PERCPU flag (equivalent to !WQ_UNBOUND),
> any alloc_workqueue() caller that doesn’t explicitly specify WQ_UNBOUND
> must now use WQ_PERCPU.
>
> Once migration is complete, WQ_UNBOUND can be removed and unbound will
> become the implicit default.
>
> Suggested-by: Tejun Heo <tj@kernel.org>
> Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
> ---
> drivers/hv/mshv_eventfd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Thank you for the well-written commit message.
Reviewed-by: Easwar Hariharan <easwar.hariharan@linux.microsoft.com>
Thanks,
Easwar (he/him)
^ permalink raw reply
* Re: [PATCH v2] mshv: Extend create partition ioctl to support cpu features
From: Wei Liu @ 2025-11-07 18:14 UTC (permalink / raw)
To: Nuno Das Neves
Cc: Wei Liu, linux-hyperv, linux-kernel, muislam, kys, haiyangz,
decui, longli, mhklinux, skinsburskii, romank, Jinank Jain
In-Reply-To: <28ab51c0-fe14-4122-8828-3f680207865d@linux.microsoft.com>
On Fri, Oct 31, 2025 at 01:08:45PM -0700, Nuno Das Neves wrote:
> On 10/31/2025 11:31 AM, Wei Liu wrote:
> > On Thu, Oct 30, 2025 at 02:40:31PM -0700, Nuno Das Neves wrote:
> >> From: Muminul Islam <muislam@microsoft.com>
> >>
> >> The existing mshv create partition ioctl does not provide a way to
> >> specify which cpu features are enabled in the guest. This was done
> >> to reduce unnecessary complexity in the API.
> >>
> >> However, some new scenarios require fine-grained control over the
> >> cpu feature bits.
> >>
> >> Define a new mshv_create_partition_v2 structure which supports passing
> >> through the disabled cpu flags and xsave flags to the hypervisor
> >> directly.
> >>
> >> When these are not specified (pt_num_cpu_fbanks == 0) or the old
> >> structure is used, define a set of default flags which cover most
> >> cases.
> >>
> >> Retain backward compatibility with the old structure via a new flag
> >> MSHV_PT_BIT_CPU_AND_XSAVE_FEATURES which enables the new struct.
> >>
> >> Co-developed-by: Jinank Jain <jinankjain@microsoft.com>
> >> Signed-off-by: Jinank Jain <jinankjain@microsoft.com>
> >> Signed-off-by: Muminul Islam <muislam@microsoft.com>
> >> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
> >> ---
> >> Changes in v2:
> >> - Fix compilation issues [kernel test robot]
> >>
> >> ---
> >> drivers/hv/mshv_root_main.c | 176 ++++++++++++++++++++++++++++++++----
> >> include/hyperv/hvhdk.h | 86 +++++++++++++++++-
> >
> > There is no mention of updating hvhdk.h in the commit message.
> >
> Ah, that's true..
>
> > Can you split out this part to a separate commit?
>
> I put the header changes in this patch because a patch containing
> those alone doesn't have much merit on its own.
>
> I know we have split header changes into separate patches in the
> past but I'm not sure it's always the right choice.
>
This makes it easier for me or other maintainers to track specifically
the header changes.
> Thinking about this, I could also split it up another way: one
> patch to introduce the new cpu features flags and use them in the
> driver, and one patch to introduce mshv_create_partition_v2.
That's fine by me.
Wei
>
> Nuno>
> > Wei
>
^ permalink raw reply
* Re: [PATCH v2 0/2] mshv: Allow mappings that overlap in uaddr
From: Wei Liu @ 2025-11-07 18:13 UTC (permalink / raw)
To: Nuno Das Neves
Cc: linux-hyperv, linux-kernel, mhklinux, magnuskulke, kys, haiyangz,
wei.liu, decui, longli, skinsburskii, prapal, mrathor, muislam
In-Reply-To: <1762467211-8213-1-git-send-email-nunodasneves@linux.microsoft.com>
On Thu, Nov 06, 2025 at 02:13:29PM -0800, Nuno Das Neves wrote:
> Currently the MSHV driver rejects mappings that would overlap in
> userspace. Remove this limitation as it is overly restrictive and
> allowing overlap is useful for VMMs.
>
> Before make this change, fix the region overlap checking logic
> which is broken.
>
> ---
> Changes in v2:
> - Add a patch to fix the overlap checking [Michael Kelley]
> - Move deletion of mshv_partition_region_by_uaddr() to the fix patch
>
> ---
> Magnus Kulke (1):
> mshv: Allow mappings that overlap in uaddr
>
> Nuno Das Neves (1):
> mshv: Fix create memory region overlap check
Applied to hyperv-next. Thanks.
>
> drivers/hv/mshv_root_main.c | 27 +++++++--------------------
> include/uapi/linux/mshv.h | 2 +-
> 2 files changed, 8 insertions(+), 21 deletions(-)
>
> --
> 2.34.1
>
^ permalink raw reply
* Re: [PATCH] mshv_eventfd: add WQ_PERCPU to alloc_workqueue users
From: Wei Liu @ 2025-11-07 18:11 UTC (permalink / raw)
To: Marco Crivellari
Cc: linux-kernel, linux-hyperv, Tejun Heo, Lai Jiangshan,
Frederic Weisbecker, Sebastian Andrzej Siewior, Michal Hocko,
K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui
In-Reply-To: <20251107132712.182499-1-marco.crivellari@suse.com>
On Fri, Nov 07, 2025 at 02:27:12PM +0100, Marco Crivellari wrote:
> Currently if a user enqueues a work item using schedule_delayed_work() the
> used wq is "system_wq" (per-cpu wq) while queue_delayed_work() use
> WORK_CPU_UNBOUND (used when a cpu is not specified). The same applies to
> schedule_work() that is using system_wq and queue_work(), that makes use
> again of WORK_CPU_UNBOUND.
> This lack of consistency cannot be addressed without refactoring the API.
>
> alloc_workqueue() treats all queues as per-CPU by default, while unbound
> workqueues must opt-in via WQ_UNBOUND.
>
> This default is suboptimal: most workloads benefit from unbound queues,
> allowing the scheduler to place worker threads where they’re needed and
> reducing noise when CPUs are isolated.
>
> This continues the effort to refactor workqueue APIs, which began with
> the introduction of new workqueues and a new alloc_workqueue flag in:
>
> commit 128ea9f6ccfb ("workqueue: Add system_percpu_wq and system_dfl_wq")
> commit 930c2ea566af ("workqueue: Add new WQ_PERCPU flag")
>
> This change adds a new WQ_PERCPU flag to explicitly request
> alloc_workqueue() to be per-cpu when WQ_UNBOUND has not been specified.
>
> With the introduction of the WQ_PERCPU flag (equivalent to !WQ_UNBOUND),
> any alloc_workqueue() caller that doesn’t explicitly specify WQ_UNBOUND
> must now use WQ_PERCPU.
>
> Once migration is complete, WQ_UNBOUND can be removed and unbound will
> become the implicit default.
>
> Suggested-by: Tejun Heo <tj@kernel.org>
> Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
Applied to hyperv-next. Thanks.
I modified the subject prefix to "mshv" and added a new line in the
commit message body to separate the first two paragraphs while at it.
Wei
> ---
> drivers/hv/mshv_eventfd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hv/mshv_eventfd.c b/drivers/hv/mshv_eventfd.c
> index 806674722868..2a80af1d610a 100644
> --- a/drivers/hv/mshv_eventfd.c
> +++ b/drivers/hv/mshv_eventfd.c
> @@ -592,7 +592,7 @@ static void mshv_irqfd_release(struct mshv_partition *pt)
>
> int mshv_irqfd_wq_init(void)
> {
> - irqfd_cleanup_wq = alloc_workqueue("mshv-irqfd-cleanup", 0, 0);
> + irqfd_cleanup_wq = alloc_workqueue("mshv-irqfd-cleanup", WQ_PERCPU, 0);
> if (!irqfd_cleanup_wq)
> return -ENOMEM;
>
> --
> 2.51.1
>
^ permalink raw reply
* Re: [PATCH net-next v8 06/14] vsock/virtio: add netns to virtio transport common
From: Bobby Eshleman @ 2025-11-07 15:47 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Shuah Khan, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Stefan Hajnoczi, Michael S. Tsirkin,
Jason Wang, Xuan Zhuo, Eugenio Pérez, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Bryan Tan, Vishnu Dasa,
Broadcom internal kernel review list, virtualization, netdev,
linux-kselftest, linux-kernel, kvm, linux-hyperv, berrange,
Bobby Eshleman
In-Reply-To: <g34g7deirdtzowtpz5pngfpuzvr62u43psmgct34iliu4bhju4@rkrxdy7n2at3>
On Fri, Nov 07, 2025 at 04:07:39PM +0100, Stefano Garzarella wrote:
> On Fri, Nov 07, 2025 at 06:33:33AM -0800, Bobby Eshleman wrote:
> > > > > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> > > > > index dcc8a1d5851e..b8e52c71920a 100644
> > > > > --- a/net/vmw_vsock/virtio_transport_common.c
> > > > > +++ b/net/vmw_vsock/virtio_transport_common.c
> > > > > @@ -316,6 +316,15 @@ static struct sk_buff *virtio_transport_alloc_skb(struct virtio_vsock_pkt_info *
> > > > > info->flags,
> > > > > zcopy);
> > > > >
> > > > > + /*
> > > > > + * If there is no corresponding socket, then we don't have a
> > > > > + * corresponding namespace. This only happens For VIRTIO_VSOCK_OP_RST.
> > > > > + */
> > > >
> > > > So, in virtio_transport_recv_pkt() should we check that `net` is not set?
> > > >
> > > > Should we set it to NULL here?
> > > >
> > >
> > > Sounds good to me.
> > >
> > > > > + if (vsk) {
> > > > > + virtio_vsock_skb_set_net(skb, info->net);
> > > >
> > > > Ditto here about the net refcnt, can the net disappear?
> > > > Should we use get_net() in some way, or the socket will prevent that?
> > > >
> > >
> > > As long as the socket has an outstanding skb it can't be destroyed and
> > > so will have a reference to the net, that is after skb_set_owner_w() and
> > > freeing... so I think this is okay.
> > >
> > > But, maybe we could simplify the implied relationship between skb, sk,
> > > and net by removing the VIRTIO_VSOCK_SKB_CB(skb)->net entirely, and only
> > > ever referring to sock_net(skb->sk)? I remember originally having a
> > > reason for adding it to the cb, but my hunch is it that it was probably
> > > some confusion over the !vsk case.
> > >
> > > WDYT?
> > >
> >
> > ... now I remember the reason, because I didn't want two different
> > places for storing the net for RX and TX.
>
> Yeah, but if we can reuse skb->sk for one path and pass it as parameter to
> the other path (see my prev email), why store it?
>
> Or even in the TX maybe it can be passed to .send_pkt() in some way, e.g.
> storing it in struct virtio_vsock_sock instead that for each skb.
>
> Stefano
>
That's a good point, the rx path only needs to pass to recv_pkt(), it is
not needed after the socket lookup there.
With TX, it does look like we could get rid of it via the
virtio_vsock_sock.
Best,
Bobby
^ permalink raw reply
* RE: [PATCH v2 2/2] mshv: Allow mappings that overlap in uaddr
From: Michael Kelley @ 2025-11-07 15:25 UTC (permalink / raw)
To: Nuno Das Neves, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, magnuskulke@linux.microsoft.com
Cc: kys@microsoft.com, haiyangz@microsoft.com, wei.liu@kernel.org,
decui@microsoft.com, longli@microsoft.com,
skinsburskii@linux.microsoft.com, prapal@linux.microsoft.com,
mrathor@linux.microsoft.com, muislam@microsoft.com
In-Reply-To: <1762467211-8213-3-git-send-email-nunodasneves@linux.microsoft.com>
From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Thursday, November 6, 2025 2:14 PM
>
> From: Magnus Kulke <magnuskulke@linux.microsoft.com>
>
> Currently the MSHV driver rejects mappings that would overlap in
> userspace.
>
> Some VMMs require the same memory to be mapped to different parts of
> the guest's address space, and so working around this restriction is
> difficult.
>
> The hypervisor itself doesn't prohibit mappings that overlap in uaddr,
> (really in SPA; system physical addresses), so supporting this in the
> driver doesn't require any extra work: only the checks need to be
> removed.
>
> Since no userspace code until now has been able to overlap regions in
> userspace, relaxing this constraint can't break any existing code.
>
> Signed-off-by: Magnus Kulke <magnuskulke@linux.microsoft.com>
> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
> ---
> drivers/hv/mshv_root_main.c | 8 ++------
> include/uapi/linux/mshv.h | 2 +-
> 2 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index 25a68912a78d..b1821b18fa09 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
> @@ -1220,12 +1220,8 @@ static int mshv_partition_create_region(struct mshv_partition *partition,
>
> /* Reject overlapping regions */
> hlist_for_each_entry(rg, &partition->pt_mem_regions, hnode) {
> - u64 rg_size = rg->nr_pages << HV_HYP_PAGE_SHIFT;
> -
> - if ((mem->guest_pfn + nr_pages <= rg->start_gfn ||
> - rg->start_gfn + rg->nr_pages <= mem->guest_pfn) &&
> - (mem->userspace_addr + mem->size <= rg->start_uaddr ||
> - rg->start_uaddr + rg_size <= mem->userspace_addr))
> + if (mem->guest_pfn + nr_pages <= rg->start_gfn ||
> + rg->start_gfn + rg->nr_pages <= mem->guest_pfn)
> continue;
>
> return -EEXIST;
> diff --git a/include/uapi/linux/mshv.h b/include/uapi/linux/mshv.h
> index 9091946cba23..b10c8d1cb2ad 100644
> --- a/include/uapi/linux/mshv.h
> +++ b/include/uapi/linux/mshv.h
> @@ -123,7 +123,7 @@ enum {
> * @rsvd: MBZ
> *
> * Map or unmap a region of userspace memory to Guest Physical Addresses (GPA).
> - * Mappings can't overlap in GPA space or userspace.
> + * Mappings can't overlap in GPA space.
> * To unmap, these fields must match an existing mapping.
> */
> struct mshv_user_mem_region {
> --
> 2.34.1
Reviewed-by: Michael Kelley <mhklinux@outlook.com>
^ permalink raw reply
* RE: [PATCH v2 1/2] mshv: Fix create memory region overlap check
From: Michael Kelley @ 2025-11-07 15:24 UTC (permalink / raw)
To: Nuno Das Neves, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, magnuskulke@linux.microsoft.com
Cc: kys@microsoft.com, haiyangz@microsoft.com, wei.liu@kernel.org,
decui@microsoft.com, longli@microsoft.com,
skinsburskii@linux.microsoft.com, prapal@linux.microsoft.com,
mrathor@linux.microsoft.com, muislam@microsoft.com
In-Reply-To: <1762467211-8213-2-git-send-email-nunodasneves@linux.microsoft.com>
From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Thursday, November 6, 2025 2:14 PM
>
> The current check is incorrect; it only checks if the beginning or end
> of a region is within an existing region. This doesn't account for
> userspace specifying a region that begins before and ends after an
> existing region.
>
> Change the logic to a range intersection check against gfns and uaddrs
> for each region.
>
> Remove mshv_partition_region_by_uaddr() as it is no longer used.
>
> Fixes: 621191d709b1 ("Drivers: hv: Introduce mshv_root module to expose /dev/mshv to VMMs")
> Reported-by: Michael Kelley <mhklinux@outlook.com>
> Closes: https://lore.kernel.org/linux-hyperv/SN6PR02MB41575BE0406D3AB22E1D7DB5D4C2A@SN6PR02MB4157.namprd02.prod.outlook.com/
> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
> ---
> drivers/hv/mshv_root_main.c | 31 +++++++++++--------------------
> 1 file changed, 11 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index 814465a0912d..25a68912a78d 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
> @@ -1206,21 +1206,6 @@ mshv_partition_region_by_gfn(struct mshv_partition *partition, u64 gfn)
> return NULL;
> }
>
> -static struct mshv_mem_region *
> -mshv_partition_region_by_uaddr(struct mshv_partition *partition, u64 uaddr)
> -{
> - struct mshv_mem_region *region;
> -
> - hlist_for_each_entry(region, &partition->pt_mem_regions, hnode) {
> - if (uaddr >= region->start_uaddr &&
> - uaddr < region->start_uaddr +
> - (region->nr_pages << HV_HYP_PAGE_SHIFT))
> - return region;
> - }
> -
> - return NULL;
> -}
> -
> /*
> * NB: caller checks and makes sure mem->size is page aligned
> * Returns: 0 with regionpp updated on success, or -errno
> @@ -1230,15 +1215,21 @@ static int mshv_partition_create_region(struct mshv_partition *partition,
> struct mshv_mem_region **regionpp,
> bool is_mmio)
> {
> - struct mshv_mem_region *region;
> + struct mshv_mem_region *region, *rg;
> u64 nr_pages = HVPFN_DOWN(mem->size);
>
> /* Reject overlapping regions */
> - if (mshv_partition_region_by_gfn(partition, mem->guest_pfn) ||
> - mshv_partition_region_by_gfn(partition, mem->guest_pfn + nr_pages - 1) ||
> - mshv_partition_region_by_uaddr(partition, mem->userspace_addr) ||
> - mshv_partition_region_by_uaddr(partition, mem->userspace_addr + mem->size - 1))
> + hlist_for_each_entry(rg, &partition->pt_mem_regions, hnode) {
> + u64 rg_size = rg->nr_pages << HV_HYP_PAGE_SHIFT;
> +
> + if ((mem->guest_pfn + nr_pages <= rg->start_gfn ||
> + rg->start_gfn + rg->nr_pages <= mem->guest_pfn) &&
> + (mem->userspace_addr + mem->size <= rg->start_uaddr ||
> + rg->start_uaddr + rg_size <= mem->userspace_addr))
> + continue;
> +
> return -EEXIST;
> + }
>
> region = vzalloc(sizeof(*region) + sizeof(struct page *) * nr_pages);
> if (!region)
> --
> 2.34.1
Reviewed-by: Michael Kelley <mhklinux@outlook.com>
^ permalink raw reply
* Re: [PATCH net-next v8 06/14] vsock/virtio: add netns to virtio transport common
From: Stefano Garzarella @ 2025-11-07 15:07 UTC (permalink / raw)
To: Bobby Eshleman
Cc: Shuah Khan, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Stefan Hajnoczi, Michael S. Tsirkin,
Jason Wang, Xuan Zhuo, Eugenio Pérez, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Bryan Tan, Vishnu Dasa,
Broadcom internal kernel review list, virtualization, netdev,
linux-kselftest, linux-kernel, kvm, linux-hyperv, berrange,
Bobby Eshleman
In-Reply-To: <aQ4DPSgu3xJhLkZ4@devvm11784.nha0.facebook.com>
On Fri, Nov 07, 2025 at 06:33:33AM -0800, Bobby Eshleman wrote:
>> > > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>> > > index dcc8a1d5851e..b8e52c71920a 100644
>> > > --- a/net/vmw_vsock/virtio_transport_common.c
>> > > +++ b/net/vmw_vsock/virtio_transport_common.c
>> > > @@ -316,6 +316,15 @@ static struct sk_buff *virtio_transport_alloc_skb(struct virtio_vsock_pkt_info *
>> > > info->flags,
>> > > zcopy);
>> > >
>> > > + /*
>> > > + * If there is no corresponding socket, then we don't have a
>> > > + * corresponding namespace. This only happens For VIRTIO_VSOCK_OP_RST.
>> > > + */
>> >
>> > So, in virtio_transport_recv_pkt() should we check that `net` is not set?
>> >
>> > Should we set it to NULL here?
>> >
>>
>> Sounds good to me.
>>
>> > > + if (vsk) {
>> > > + virtio_vsock_skb_set_net(skb, info->net);
>> >
>> > Ditto here about the net refcnt, can the net disappear?
>> > Should we use get_net() in some way, or the socket will prevent that?
>> >
>>
>> As long as the socket has an outstanding skb it can't be destroyed and
>> so will have a reference to the net, that is after skb_set_owner_w() and
>> freeing... so I think this is okay.
>>
>> But, maybe we could simplify the implied relationship between skb, sk,
>> and net by removing the VIRTIO_VSOCK_SKB_CB(skb)->net entirely, and only
>> ever referring to sock_net(skb->sk)? I remember originally having a
>> reason for adding it to the cb, but my hunch is it that it was probably
>> some confusion over the !vsk case.
>>
>> WDYT?
>>
>
>... now I remember the reason, because I didn't want two different
>places for storing the net for RX and TX.
Yeah, but if we can reuse skb->sk for one path and pass it as parameter
to the other path (see my prev email), why store it?
Or even in the TX maybe it can be passed to .send_pkt() in some way,
e.g. storing it in struct virtio_vsock_sock instead that for each skb.
Stefano
^ permalink raw reply
* Re: [PATCH net-next v8 06/14] vsock/virtio: add netns to virtio transport common
From: Bobby Eshleman @ 2025-11-07 14:33 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Shuah Khan, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Stefan Hajnoczi, Michael S. Tsirkin,
Jason Wang, Xuan Zhuo, Eugenio Pérez, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Bryan Tan, Vishnu Dasa,
Broadcom internal kernel review list, virtualization, netdev,
linux-kselftest, linux-kernel, kvm, linux-hyperv, berrange,
Bobby Eshleman
In-Reply-To: <aQ1e3/DZbgnYw4Ja@devvm11784.nha0.facebook.com>
> > > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> > > index dcc8a1d5851e..b8e52c71920a 100644
> > > --- a/net/vmw_vsock/virtio_transport_common.c
> > > +++ b/net/vmw_vsock/virtio_transport_common.c
> > > @@ -316,6 +316,15 @@ static struct sk_buff *virtio_transport_alloc_skb(struct virtio_vsock_pkt_info *
> > > info->flags,
> > > zcopy);
> > >
> > > + /*
> > > + * If there is no corresponding socket, then we don't have a
> > > + * corresponding namespace. This only happens For VIRTIO_VSOCK_OP_RST.
> > > + */
> >
> > So, in virtio_transport_recv_pkt() should we check that `net` is not set?
> >
> > Should we set it to NULL here?
> >
>
> Sounds good to me.
>
> > > + if (vsk) {
> > > + virtio_vsock_skb_set_net(skb, info->net);
> >
> > Ditto here about the net refcnt, can the net disappear?
> > Should we use get_net() in some way, or the socket will prevent that?
> >
>
> As long as the socket has an outstanding skb it can't be destroyed and
> so will have a reference to the net, that is after skb_set_owner_w() and
> freeing... so I think this is okay.
>
> But, maybe we could simplify the implied relationship between skb, sk,
> and net by removing the VIRTIO_VSOCK_SKB_CB(skb)->net entirely, and only
> ever referring to sock_net(skb->sk)? I remember originally having a
> reason for adding it to the cb, but my hunch is it that it was probably
> some confusion over the !vsk case.
>
> WDYT?
>
... now I remember the reason, because I didn't want two different
places for storing the net for RX and TX.
Best,
Bobby
^ permalink raw reply
* Re: [PATCH net-next v8 06/14] vsock/virtio: add netns to virtio transport common
From: Stefano Garzarella @ 2025-11-07 14:30 UTC (permalink / raw)
To: Bobby Eshleman
Cc: Shuah Khan, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Stefan Hajnoczi, Michael S. Tsirkin,
Jason Wang, Xuan Zhuo, Eugenio Pérez, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Bryan Tan, Vishnu Dasa,
Broadcom internal kernel review list, virtualization, netdev,
linux-kselftest, linux-kernel, kvm, linux-hyperv, berrange,
Bobby Eshleman
In-Reply-To: <aQ1e3/DZbgnYw4Ja@devvm11784.nha0.facebook.com>
On Thu, Nov 06, 2025 at 06:52:15PM -0800, Bobby Eshleman wrote:
>On Thu, Nov 06, 2025 at 05:20:05PM +0100, Stefano Garzarella wrote:
>> On Thu, Oct 23, 2025 at 11:27:45AM -0700, Bobby Eshleman wrote:
>> > From: Bobby Eshleman <bobbyeshleman@meta.com>
>> >
>> > Enable network namespace support in the virtio-vsock common transport
>> > layer by declaring namespace pointers in the transmit and receive
>> > paths.
>> >
>> > The changes include:
>> > 1. Add a 'net' field to virtio_vsock_pkt_info to carry the namespace
>> > pointer for outgoing packets.
>> > 2. Store the namespace and namespace mode in the skb control buffer when
>> > allocating packets (except for VIRTIO_VSOCK_OP_RST packets which do
>> > not have an associated socket).
>> > 3. Retrieve namespace information from skbs on the receive path for
>> > lookups using vsock_find_connected_socket_net() and
>> > vsock_find_bound_socket_net().
>> >
>> > This allows users of virtio transport common code
>> > (vhost-vsock/virtio-vsock) to later enable namespace support.
>> >
>> > Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com>
>> > ---
>> > Changes in v7:
>> > - add comment explaining the !vsk case in
>> > virtio_transport_alloc_skb()
>> > ---
>> > include/linux/virtio_vsock.h | 1 +
>> > net/vmw_vsock/virtio_transport_common.c | 21 +++++++++++++++++++--
>> > 2 files changed, 20 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>> > index 29290395054c..f90646f82993 100644
>> > --- a/include/linux/virtio_vsock.h
>> > +++ b/include/linux/virtio_vsock.h
>> > @@ -217,6 +217,7 @@ struct virtio_vsock_pkt_info {
>> > u32 remote_cid, remote_port;
>> > struct vsock_sock *vsk;
>> > struct msghdr *msg;
>> > + struct net *net;
>> > u32 pkt_len;
>> > u16 type;
>> > u16 op;
>> > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>> > index dcc8a1d5851e..b8e52c71920a 100644
>> > --- a/net/vmw_vsock/virtio_transport_common.c
>> > +++ b/net/vmw_vsock/virtio_transport_common.c
>> > @@ -316,6 +316,15 @@ static struct sk_buff *virtio_transport_alloc_skb(struct virtio_vsock_pkt_info *
>> > info->flags,
>> > zcopy);
>> >
>> > + /*
>> > + * If there is no corresponding socket, then we don't have a
>> > + * corresponding namespace. This only happens For VIRTIO_VSOCK_OP_RST.
>> > + */
>>
>> So, in virtio_transport_recv_pkt() should we check that `net` is not set?
>>
>> Should we set it to NULL here?
>>
>
>Sounds good to me.
>
>> > + if (vsk) {
>> > + virtio_vsock_skb_set_net(skb, info->net);
>>
>> Ditto here about the net refcnt, can the net disappear?
>> Should we use get_net() in some way, or the socket will prevent that?
>>
>
>As long as the socket has an outstanding skb it can't be destroyed and
>so will have a reference to the net, that is after skb_set_owner_w() and
>freeing... so I think this is okay.
>
>But, maybe we could simplify the implied relationship between skb, sk,
>and net by removing the VIRTIO_VSOCK_SKB_CB(skb)->net entirely, and only
>ever referring to sock_net(skb->sk)? I remember originally having a
>reason for adding it to the cb, but my hunch is it that it was probably
>some confusion over the !vsk case.
>
>WDYT?
If vsk == NULL, I'm expecting that also skb->sk is not valid, right?
Indeed we call skb_set_owner_w() only if vsk != NULL in
virtio_transport_alloc_skb().
Maybe we need to change virtio_transport_recv_pkt() where the `net`
should be passed in some way by the caller, so maybe this is the reason
why you needed it in the cb. But also in that case, I think we can get
the `net` in some way and pass it to virtio_transport_recv_pkt() and
avoid the change in the cb:
- vsock_lookpback.c in vsock_loopback_work() we can use vsock->net
- vhost/vsock.c in vhost_vsock_handle_tx_kick(), ditto we can use
vsock->net
- virtio_transport.c we can just pass always the dummy_net
Same fot the net_mode.
Maybe the real problem is in the send_pkt callbacks, where the skb is
used to get the socket, but as you mention, I think in this path
skb_set_owner_w() is already called, so we can get that info from there
in some way.
Not sure, but yeah, if we can remove that, it will be much clear IMO.
Thanks,
Stefano
^ permalink raw reply
* Re: [PATCH net-next v8 04/14] vsock: add netns to vsock core
From: Stefano Garzarella @ 2025-11-07 13:53 UTC (permalink / raw)
To: Bobby Eshleman
Cc: Shuah Khan, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Stefan Hajnoczi, Michael S. Tsirkin,
Jason Wang, Xuan Zhuo, Eugenio Pérez, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Bryan Tan, Vishnu Dasa,
Broadcom internal kernel review list, virtualization, netdev,
linux-kselftest, linux-kernel, kvm, linux-hyperv, berrange,
Bobby Eshleman
In-Reply-To: <aQ1TXjb8AWIzgAu4@devvm11784.nha0.facebook.com>
On Thu, Nov 06, 2025 at 06:03:10PM -0800, Bobby Eshleman wrote:
>On Thu, Nov 06, 2025 at 05:18:00PM +0100, Stefano Garzarella wrote:
>> On Thu, Oct 23, 2025 at 11:27:43AM -0700, Bobby Eshleman wrote:
>> > From: Bobby Eshleman <bobbyeshleman@meta.com>
>> >
>> > Add netns logic to vsock core. Additionally, modify transport hook
>> > prototypes to be used by later transport-specific patches (e.g.,
>> > *_seqpacket_allow()).
>> >
>> > Namespaces are supported primarily by changing socket lookup functions
>> > (e.g., vsock_find_connected_socket()) to take into account the socket
>> > namespace and the namespace mode before considering a candidate socket a
>> > "match".
>> >
>> > Introduce a dummy namespace struct, __vsock_global_dummy_net, to be
>> > used by transports that do not support namespacing. This dummy always
>> > has mode "global" to preserve previous CID behavior.
>> >
>> > This patch also introduces the sysctl /proc/sys/net/vsock/ns_mode that
>> > accepts the "global" or "local" mode strings.
>> >
>> > The transports (besides vhost) are modified to use the global dummy,
>> > which makes them behave as if always in the global namespace. Vhost is
>> > an exception because it inherits its namespace from the process that
>> > opens the vhost device.
>> >
>> > Add netns functionality (initialization, passing to transports, procfs,
>> > etc...) to the af_vsock socket layer. Later patches that add netns
>> > support to transports depend on this patch.
>> >
>> > seqpacket_allow() callbacks are modified to take a vsk so that transport
>> > implementations can inspect sock_net(sk) and vsk->net_mode when performing
>> > lookups (e.g., vhost does this in its future netns patch). Because the
>> > API change affects all transports, it seemed more appropriate to make
>> > this internal API change in the "vsock core" patch then in the "vhost"
>> > patch.
>> >
>> > Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com>
>> > ---
>> > Changes in v7:
>> > - hv_sock: fix hyperv build error
>> > - explain why vhost does not use the dummy
>> > - explain usage of __vsock_global_dummy_net
>> > - explain why VSOCK_NET_MODE_STR_MAX is 8 characters
>> > - use switch-case in vsock_net_mode_string()
>> > - avoid changing transports as much as possible
>> > - add vsock_find_{bound,connected}_socket_net()
>> > - rename `vsock_hdr` to `sysctl_hdr`
>> > - add virtio_vsock_alloc_linear_skb() wrapper for setting dummy net and
>> > global mode for virtio-vsock, move skb->cb zero-ing into wrapper
>> > - explain seqpacket_allow() change
>> > - move net setting to __vsock_create() instead of vsock_create() so
>> > that child sockets also have their net assigned upon accept()
>> >
>> > Changes in v6:
>> > - unregister sysctl ops in vsock_exit()
>> > - af_vsock: clarify description of CID behavior
>> > - af_vsock: fix buf vs buffer naming, and length checking
>> > - af_vsock: fix length checking w/ correct ctl_table->maxlen
>> >
>> > Changes in v5:
>> > - vsock_global_net() -> vsock_global_dummy_net()
>> > - update comments for new uAPI
>> > - use /proc/sys/net/vsock/ns_mode instead of /proc/net/vsock_ns_mode
>> > - add prototype changes so patch remains compilable
>> > ---
>> > drivers/vhost/vsock.c | 4 +-
>> > include/linux/virtio_vsock.h | 21 ++++
>> > include/net/af_vsock.h | 14 ++-
>> > net/vmw_vsock/af_vsock.c | 264 ++++++++++++++++++++++++++++++++++++---
>> > net/vmw_vsock/virtio_transport.c | 7 +-
>> > net/vmw_vsock/vsock_loopback.c | 4 +-
>> > 6 files changed, 288 insertions(+), 26 deletions(-)
>> >
>> > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
>> > index ae01457ea2cd..34adf0cf9124 100644
>> > --- a/drivers/vhost/vsock.c
>> > +++ b/drivers/vhost/vsock.c
>> > @@ -404,7 +404,7 @@ static bool vhost_transport_msgzerocopy_allow(void)
>> > return true;
>> > }
>> >
>> > -static bool vhost_transport_seqpacket_allow(u32 remote_cid);
>> > +static bool vhost_transport_seqpacket_allow(struct vsock_sock *vsk, u32 remote_cid);
>> >
>> > static struct virtio_transport vhost_transport = {
>> > .transport = {
>> > @@ -460,7 +460,7 @@ static struct virtio_transport vhost_transport = {
>> > .send_pkt = vhost_transport_send_pkt,
>> > };
>> >
>> > -static bool vhost_transport_seqpacket_allow(u32 remote_cid)
>> > +static bool vhost_transport_seqpacket_allow(struct vsock_sock *vsk, u32 remote_cid)
>> > {
>> > struct vhost_vsock *vsock;
>> > bool seqpacket_allow = false;
>> > diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>> > index 7f334a32133c..29290395054c 100644
>> > --- a/include/linux/virtio_vsock.h
>> > +++ b/include/linux/virtio_vsock.h
>> > @@ -153,6 +153,27 @@ static inline void virtio_vsock_skb_set_net_mode(struct sk_buff *skb,
>> > VIRTIO_VSOCK_SKB_CB(skb)->net_mode = net_mode;
>> > }
>> >
>> > +static inline struct sk_buff *
>> > +virtio_vsock_alloc_rx_skb(unsigned int size, gfp_t mask)
>> > +{
>> > + struct sk_buff *skb;
>> > +
>> > + skb = virtio_vsock_alloc_linear_skb(size, mask);
>> > + if (!skb)
>> > + return NULL;
>> > +
>> > + memset(skb->head, 0, VIRTIO_VSOCK_SKB_HEADROOM);
>> > +
>> > + /* virtio-vsock does not yet support namespaces, so on receive
>> > + * we force legacy namespace behavior using the global dummy net
>> > + * and global net mode.
>> > + */
>> > + virtio_vsock_skb_set_net(skb, vsock_global_dummy_net());
>> > + virtio_vsock_skb_set_net_mode(skb, VSOCK_NET_MODE_GLOBAL);
>> > +
>> > + return skb;
>> > +}
>>
>> Why we are introducing this change in this patch?
>>
>> Where the net of the virtio's skb is read?
>>
>
>Oh good point, this is a weird place for this. I'll move this to where
>it is actually used.
>
>[...]
>
>> >
>> > +static int vsock_net_mode_string(const struct ctl_table *table, int write,
>> > + void *buffer, size_t *lenp, loff_t *ppos)
>> > +{
>> > + char data[VSOCK_NET_MODE_STR_MAX] = {0};
>> > + enum vsock_net_mode mode;
>> > + struct ctl_table tmp;
>> > + struct net *net;
>> > + int ret;
>> > +
>> > + if (!table->data || !table->maxlen || !*lenp) {
>> > + *lenp = 0;
>> > + return 0;
>> > + }
>> > +
>> > + net = current->nsproxy->net_ns;
>> > + tmp = *table;
>> > + tmp.data = data;
>> > +
>> > + if (!write) {
>> > + const char *p;
>> > +
>> > + mode = vsock_net_mode(net);
>> > +
>> > + switch (mode) {
>> > + case VSOCK_NET_MODE_GLOBAL:
>> > + p = VSOCK_NET_MODE_STR_GLOBAL;
>> > + break;
>> > + case VSOCK_NET_MODE_LOCAL:
>> > + p = VSOCK_NET_MODE_STR_LOCAL;
>> > + break;
>> > + default:
>> > + WARN_ONCE(true, "netns has invalid vsock mode");
>> > + *lenp = 0;
>> > + return 0;
>> > + }
>> > +
>> > + strscpy(data, p, sizeof(data));
>> > + tmp.maxlen = strlen(p);
>> > + }
>> > +
>> > + ret = proc_dostring(&tmp, write, buffer, lenp, ppos);
>> > + if (ret)
>> > + return ret;
>> > +
>> > + if (write) {
>>
>> Do we need to check some capability, e.g. CAP_NET_ADMIN ?
>>
>
>We get that for free via the sysctl_net registration, through this path
>on open (CAP_NET_ADMIN is checked in net_ctl_permissions):
>
> net_ctl_permissions+1
> sysctl_perm+24
> proc_sys_permission+117
> inode_permission+217
> link_path_walk+162
> path_openat+152
> do_filp_open+171
> do_sys_openat2+98
> __x64_sys_openat+69
> do_syscall_64+93
>
>Verified with:
>
>cp /bin/echo /tmp/echo_netadmin
>setcap cap_net_admin+ep /tmp/echo_netadmin
>
>(non-root user fails with regular echo, succeeds with
>/tmp/echo_netadmin)
Thanks for checking!
Stefano
^ permalink raw reply
* [PATCH] mshv_eventfd: add WQ_PERCPU to alloc_workqueue users
From: Marco Crivellari @ 2025-11-07 13:27 UTC (permalink / raw)
To: linux-kernel, linux-hyperv
Cc: Tejun Heo, Lai Jiangshan, Frederic Weisbecker,
Sebastian Andrzej Siewior, Marco Crivellari, Michal Hocko,
K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui
Currently if a user enqueues a work item using schedule_delayed_work() the
used wq is "system_wq" (per-cpu wq) while queue_delayed_work() use
WORK_CPU_UNBOUND (used when a cpu is not specified). The same applies to
schedule_work() that is using system_wq and queue_work(), that makes use
again of WORK_CPU_UNBOUND.
This lack of consistency cannot be addressed without refactoring the API.
alloc_workqueue() treats all queues as per-CPU by default, while unbound
workqueues must opt-in via WQ_UNBOUND.
This default is suboptimal: most workloads benefit from unbound queues,
allowing the scheduler to place worker threads where they’re needed and
reducing noise when CPUs are isolated.
This continues the effort to refactor workqueue APIs, which began with
the introduction of new workqueues and a new alloc_workqueue flag in:
commit 128ea9f6ccfb ("workqueue: Add system_percpu_wq and system_dfl_wq")
commit 930c2ea566af ("workqueue: Add new WQ_PERCPU flag")
This change adds a new WQ_PERCPU flag to explicitly request
alloc_workqueue() to be per-cpu when WQ_UNBOUND has not been specified.
With the introduction of the WQ_PERCPU flag (equivalent to !WQ_UNBOUND),
any alloc_workqueue() caller that doesn’t explicitly specify WQ_UNBOUND
must now use WQ_PERCPU.
Once migration is complete, WQ_UNBOUND can be removed and unbound will
become the implicit default.
Suggested-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
---
drivers/hv/mshv_eventfd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hv/mshv_eventfd.c b/drivers/hv/mshv_eventfd.c
index 806674722868..2a80af1d610a 100644
--- a/drivers/hv/mshv_eventfd.c
+++ b/drivers/hv/mshv_eventfd.c
@@ -592,7 +592,7 @@ static void mshv_irqfd_release(struct mshv_partition *pt)
int mshv_irqfd_wq_init(void)
{
- irqfd_cleanup_wq = alloc_workqueue("mshv-irqfd-cleanup", 0, 0);
+ irqfd_cleanup_wq = alloc_workqueue("mshv-irqfd-cleanup", WQ_PERCPU, 0);
if (!irqfd_cleanup_wq)
return -ENOMEM;
--
2.51.1
^ permalink raw reply related
* Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute
From: ally heev @ 2025-11-07 7:27 UTC (permalink / raw)
To: Przemek Kitszel, Alexander Lobakin
Cc: Tony Nguyen, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Aleksandr Loktionov, intel-wired-lan, netdev,
linux-kernel, linux-hyperv, Dan Carpenter
In-Reply-To: <afa219b7-9ce3-4da8-a339-8f363d77824e@intel.com>
On Fri, 2025-11-07 at 06:39 +0100, Przemek Kitszel wrote:
> On 11/6/25 17:05, ally heev wrote:
> > On Thu, 2025-11-06 at 15:07 +0100, Alexander Lobakin wrote:
> > [..]
> > > >
> > > > diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
> > > > index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
> > > > --- a/drivers/net/ethernet/intel/ice/ice_flow.c
> > > > +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
> > > > @@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
> > > > struct ice_parser_profile *prof, enum ice_block blk)
> > > > {
> > > > u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
> > > > - struct ice_flow_prof_params *params __free(kfree);
> > > > u8 fv_words = hw->blk[blk].es.fvw;
> > > > int status;
> > > > int i, idx;
> > > >
> > > > - params = kzalloc(sizeof(*params), GFP_KERNEL);
> > > > + struct ice_flow_prof_params *params __free(kfree) =
> > > > + kzalloc(sizeof(*params), GFP_KERNEL);
> > >
> > > Please don't do it that way. It's not C++ with RAII and
> > > declare-where-you-use.
> > > Just leave the variable declarations where they are, but initialize them
> > > with `= NULL`.
>
> +1
>
> > >
> > > Variable declarations must be in one block and sorted from the longest
> > > to the shortest.
> > >
> > > But most important, I'm not even sure how you could trigger an
> > > "undefined behaviour" here. Both here and below the variable tagged with
> > > `__free` is initialized right after the declaration block, before any
> > > return. So how to trigger an UB here?
> >
> > It doesn't occur here. But, many maintainers/developers consider it a
> > bad practice because if the function returns before initialization or
> > use of `goto` can cause such behaviors.
>
> we were bitten by that already, scenario is as follow:
> 0. have a good code w/o UB and w/o redundant = NULL
> 1. add some early return, say:
> if (dest_vsi == fdir_vsi)
> return -EINVAL;
> 2. almost granted that person adding 1. will forget to add = NULL to all
> declarations marked __free
>
> >
> > Here though, the definitions are still at the top right? Maybe I could
> > just sort them
>
> we discourage putting any operations, including allocations, that may
> fail into the declarations block
>
Makes sense. I will just initialize them with NULL then
^ permalink raw reply
* Re: [PATCH] drivers/hv: Use kmalloc_array() instead of kmalloc()
From: Wei Liu @ 2025-11-07 6:52 UTC (permalink / raw)
To: Rahul Kumar
Cc: kys, haiyangz, wei.liu, decui, linux-hyperv, linux-kernel,
linux-kernel-mentees, skhan
In-Reply-To: <20251104121618.1396291-1-rk0006818@gmail.com>
On Tue, Nov 04, 2025 at 05:46:18PM +0530, Rahul Kumar wrote:
> Documentation/process/deprecated.rst recommends against the use of
> kmalloc with dynamic size calculations due to the risk of overflow and
> smaller allocation being made than the caller was expecting.
>
> Replace kmalloc() with kmalloc_array() in hv_common.c to make the
> intended allocation size clearer and avoid potential overflow issues.
>
> The number of pages (pgcount) is bounded, so overflow is not a
> practical concern here. However, using kmalloc_array() better reflects
> the intent to allocate an array and improves consistency with other
> allocations.
>
> No functional change intended.
>
> Signed-off-by: Rahul Kumar <rk0006818@gmail.com>
Applied to hyperv-next. Thanks.
> ---
> drivers/hv/hv_common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index e109a620c83f..68689beb383c 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -487,7 +487,7 @@ int hv_common_cpu_init(unsigned int cpu)
> * online and then taken offline
> */
> if (!*inputarg) {
> - mem = kmalloc(pgcount * HV_HYP_PAGE_SIZE, flags);
> + mem = kmalloc_array(pgcount, HV_HYP_PAGE_SIZE, flags);
> if (!mem)
> return -ENOMEM;
>
> --
> 2.43.0
>
^ permalink raw reply
* Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute
From: Przemek Kitszel @ 2025-11-07 5:39 UTC (permalink / raw)
To: ally heev, Alexander Lobakin
Cc: Tony Nguyen, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Aleksandr Loktionov, intel-wired-lan, netdev,
linux-kernel, linux-hyperv, Dan Carpenter
In-Reply-To: <00748f83a8ae688b7063f36844e38073d29b5e19.camel@gmail.com>
On 11/6/25 17:05, ally heev wrote:
> On Thu, 2025-11-06 at 15:07 +0100, Alexander Lobakin wrote:
> [..]
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
>>> index 6d5c939dc8a515c252cd2b77d155b69fa264ee92..3590dacf3ee57879b3809d715e40bb290e40c4aa 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_flow.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
>>> @@ -1573,12 +1573,13 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
>>> struct ice_parser_profile *prof, enum ice_block blk)
>>> {
>>> u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX);
>>> - struct ice_flow_prof_params *params __free(kfree);
>>> u8 fv_words = hw->blk[blk].es.fvw;
>>> int status;
>>> int i, idx;
>>>
>>> - params = kzalloc(sizeof(*params), GFP_KERNEL);
>>> + struct ice_flow_prof_params *params __free(kfree) =
>>> + kzalloc(sizeof(*params), GFP_KERNEL);
>>
>> Please don't do it that way. It's not C++ with RAII and
>> declare-where-you-use.
>> Just leave the variable declarations where they are, but initialize them
>> with `= NULL`.
+1
>>
>> Variable declarations must be in one block and sorted from the longest
>> to the shortest.
>>
>> But most important, I'm not even sure how you could trigger an
>> "undefined behaviour" here. Both here and below the variable tagged with
>> `__free` is initialized right after the declaration block, before any
>> return. So how to trigger an UB here?
>
> It doesn't occur here. But, many maintainers/developers consider it a
> bad practice because if the function returns before initialization or
> use of `goto` can cause such behaviors.
we were bitten by that already, scenario is as follow:
0. have a good code w/o UB and w/o redundant = NULL
1. add some early return, say:
if (dest_vsi == fdir_vsi)
return -EINVAL;
2. almost granted that person adding 1. will forget to add = NULL to all
declarations marked __free
>
> Here though, the definitions are still at the top right? Maybe I could
> just sort them
we discourage putting any operations, including allocations, that may
fail into the declarations block
^ permalink raw reply
* Re: [PATCH net-next v8 07/14] vhost/vsock: add netns support
From: Bobby Eshleman @ 2025-11-07 3:07 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Shuah Khan, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Stefan Hajnoczi, Michael S. Tsirkin,
Jason Wang, Xuan Zhuo, Eugenio Pérez, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Bryan Tan, Vishnu Dasa,
Broadcom internal kernel review list, virtualization, netdev,
linux-kselftest, linux-kernel, kvm, linux-hyperv, berrange,
Bobby Eshleman
In-Reply-To: <juxkmz3vskdopukejobv745j6qqx45hhcdjtjw7gcpgz6fj5ws@ckz7dvyup6mq>
On Thu, Nov 06, 2025 at 05:21:35PM +0100, Stefano Garzarella wrote:
> On Thu, Oct 23, 2025 at 11:27:46AM -0700, Bobby Eshleman wrote:
> > From: Bobby Eshleman <bobbyeshleman@meta.com>
> >
> > Add the ability to isolate vhost-vsock flows using namespaces.
> >
> > The VM, via the vhost_vsock struct, inherits its namespace from the
> > process that opens the vhost-vsock device. vhost_vsock lookup functions
> > are modified to take into account the mode (e.g., if CIDs are matching
> > but modes don't align, then return NULL).
> >
> > vhost_vsock now acquires a reference to the namespace.
> >
> > Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com>
> > ---
> > Changes in v7:
> > - remove the check_global flag of vhost_vsock_get(), that logic was both
> > wrong and not necessary, reuse vsock_net_check_mode() instead
> > - remove 'delete me' comment
> > Changes in v5:
> > - respect pid namespaces when assigning namespace to vhost_vsock
> > ---
> > drivers/vhost/vsock.c | 44 ++++++++++++++++++++++++++++++++++----------
> > 1 file changed, 34 insertions(+), 10 deletions(-)
[...]
> > static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
> > {
> > +
> > struct vhost_virtqueue **vqs;
> > struct vhost_vsock *vsock;
> > + struct net *net;
> > int ret;
> >
> > /* This struct is large and allocation could fail, fall back to vmalloc
> > @@ -669,6 +684,14 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
> > goto out;
> > }
> >
> > + net = current->nsproxy->net_ns;
> > + vsock->net = get_net_track(net, &vsock->ns_tracker, GFP_KERNEL);
> > +
> > + /* Cache the mode of the namespace so that if that netns mode changes,
> > + * the vhost_vsock will continue to function as expected.
> > + */
>
> I think we should document this in the commit description and in both we
> should add also the reason. (IIRC, it was to simplify everything and prevent
> a VM from changing modes when running and then tracking all its packets)
>
Sounds good!
> > + vsock->net_mode = vsock_net_mode(net);
> > +
> > vsock->guest_cid = 0; /* no CID assigned yet */
> > vsock->seqpacket_allow = false;
> >
> > @@ -708,7 +731,7 @@ static void vhost_vsock_reset_orphans(struct sock *sk)
> > */
> >
> > /* If the peer is still valid, no need to reset connection */
> > - if (vhost_vsock_get(vsk->remote_addr.svm_cid))
> > + if (vhost_vsock_get(vsk->remote_addr.svm_cid, sock_net(sk), vsk->net_mode))
> > return;
> >
> > /* If the close timeout is pending, let it expire. This avoids races
> > @@ -753,6 +776,7 @@ static int vhost_vsock_dev_release(struct inode *inode, struct file *file)
> > virtio_vsock_skb_queue_purge(&vsock->send_pkt_queue);
> >
> > vhost_dev_cleanup(&vsock->dev);
> > + put_net_track(vsock->net, &vsock->ns_tracker);
>
> Doing this after virtio_vsock_skb_queue_purge() should ensure that all skbs
> have been drained, so there should be no one flying with this netns. Perhaps
> this clarifies my doubts about the skb net, but should we do something
> similar for loopback as well?
100% - for loopback the skb purge is done in the net exit hook, which is
called just before netns destruction. Maybe it is worth commenting that
context there too.
> And maybe we should document that also in the virtio_vsock_skb_cb.
>
sgtm!
Best,
Bobby
^ permalink raw reply
* Re: [PATCH net-next v8 06/14] vsock/virtio: add netns to virtio transport common
From: Bobby Eshleman @ 2025-11-07 2:52 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Shuah Khan, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Stefan Hajnoczi, Michael S. Tsirkin,
Jason Wang, Xuan Zhuo, Eugenio Pérez, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Bryan Tan, Vishnu Dasa,
Broadcom internal kernel review list, virtualization, netdev,
linux-kselftest, linux-kernel, kvm, linux-hyperv, berrange,
Bobby Eshleman
In-Reply-To: <hkwlp6wpiik35zesxqfe6uw7m6uayd4tcbvrg55qhhej3ox33q@lah2dwed477g>
On Thu, Nov 06, 2025 at 05:20:05PM +0100, Stefano Garzarella wrote:
> On Thu, Oct 23, 2025 at 11:27:45AM -0700, Bobby Eshleman wrote:
> > From: Bobby Eshleman <bobbyeshleman@meta.com>
> >
> > Enable network namespace support in the virtio-vsock common transport
> > layer by declaring namespace pointers in the transmit and receive
> > paths.
> >
> > The changes include:
> > 1. Add a 'net' field to virtio_vsock_pkt_info to carry the namespace
> > pointer for outgoing packets.
> > 2. Store the namespace and namespace mode in the skb control buffer when
> > allocating packets (except for VIRTIO_VSOCK_OP_RST packets which do
> > not have an associated socket).
> > 3. Retrieve namespace information from skbs on the receive path for
> > lookups using vsock_find_connected_socket_net() and
> > vsock_find_bound_socket_net().
> >
> > This allows users of virtio transport common code
> > (vhost-vsock/virtio-vsock) to later enable namespace support.
> >
> > Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com>
> > ---
> > Changes in v7:
> > - add comment explaining the !vsk case in virtio_transport_alloc_skb()
> > ---
> > include/linux/virtio_vsock.h | 1 +
> > net/vmw_vsock/virtio_transport_common.c | 21 +++++++++++++++++++--
> > 2 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
> > index 29290395054c..f90646f82993 100644
> > --- a/include/linux/virtio_vsock.h
> > +++ b/include/linux/virtio_vsock.h
> > @@ -217,6 +217,7 @@ struct virtio_vsock_pkt_info {
> > u32 remote_cid, remote_port;
> > struct vsock_sock *vsk;
> > struct msghdr *msg;
> > + struct net *net;
> > u32 pkt_len;
> > u16 type;
> > u16 op;
> > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> > index dcc8a1d5851e..b8e52c71920a 100644
> > --- a/net/vmw_vsock/virtio_transport_common.c
> > +++ b/net/vmw_vsock/virtio_transport_common.c
> > @@ -316,6 +316,15 @@ static struct sk_buff *virtio_transport_alloc_skb(struct virtio_vsock_pkt_info *
> > info->flags,
> > zcopy);
> >
> > + /*
> > + * If there is no corresponding socket, then we don't have a
> > + * corresponding namespace. This only happens For VIRTIO_VSOCK_OP_RST.
> > + */
>
> So, in virtio_transport_recv_pkt() should we check that `net` is not set?
>
> Should we set it to NULL here?
>
Sounds good to me.
> > + if (vsk) {
> > + virtio_vsock_skb_set_net(skb, info->net);
>
> Ditto here about the net refcnt, can the net disappear?
> Should we use get_net() in some way, or the socket will prevent that?
>
As long as the socket has an outstanding skb it can't be destroyed and
so will have a reference to the net, that is after skb_set_owner_w() and
freeing... so I think this is okay.
But, maybe we could simplify the implied relationship between skb, sk,
and net by removing the VIRTIO_VSOCK_SKB_CB(skb)->net entirely, and only
ever referring to sock_net(skb->sk)? I remember originally having a
reason for adding it to the cb, but my hunch is it that it was probably
some confusion over the !vsk case.
WDYT?
[...]
> >
> > return virtio_transport_send_pkt_info(vsk, &info);
> > @@ -1578,7 +1593,9 @@ static bool virtio_transport_valid_type(u16 type)
> > void virtio_transport_recv_pkt(struct virtio_transport *t,
> > struct sk_buff *skb)
> > {
> > + enum vsock_net_mode net_mode = virtio_vsock_skb_net_mode(skb);
> > struct virtio_vsock_hdr *hdr = virtio_vsock_hdr(skb);
> > + struct net *net = virtio_vsock_skb_net(skb);
>
> Okay, so this is where the skb net is read, so why we touch the virtio-vsock
> driver (virtio_transport.c) in the other patch where we changed just
> af_vsock.c?
>
> IMO we should move that change here, or in a separate commit.
> Or maybe I missed some dependency :-)
>
100% agree.
> Thanks,
> Stefano
>
Thanks!
-Bobby
^ permalink raw reply
* Re: [PATCH net-next v8 05/14] vsock/loopback: add netns support
From: Bobby Eshleman @ 2025-11-07 2:17 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Shuah Khan, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Stefan Hajnoczi, Michael S. Tsirkin,
Jason Wang, Xuan Zhuo, Eugenio Pérez, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Bryan Tan, Vishnu Dasa,
Broadcom internal kernel review list, virtualization, netdev,
linux-kselftest, linux-kernel, kvm, linux-hyperv, berrange,
Bobby Eshleman
In-Reply-To: <tuclqrdg5p2uzfvczhcdig7jlifvhqtlafe4xcqy4x4p3vrya6@jq5mujdluze5>
On Thu, Nov 06, 2025 at 05:18:36PM +0100, Stefano Garzarella wrote:
> On Thu, Oct 23, 2025 at 11:27:44AM -0700, Bobby Eshleman wrote:
> > From: Bobby Eshleman <bobbyeshleman@meta.com>
> >
> > Add NS support to vsock loopback. Sockets in a global mode netns
> > communicate with each other, regardless of namespace. Sockets in a local
> > mode netns may only communicate with other sockets within the same
> > namespace.
> >
> > Use pernet_ops to install a vsock_loopback for every namespace that is
> > created (to be used if local mode is enabled).
> >
> > Retroactively call init/exit on every namespace when the vsock_loopback
> > module is loaded in order to initialize the per-ns device.
> >
> > Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com>
> > ---
>
> I'm a bit confused, should we move this after the next patch that add
> support of netns in the virtio common module?
>
> Or this is a pre-requisite?
>
Yes let's do that, it does need common.
[...]
> > #endif /* __NET_NET_NAMESPACE_VSOCK_H */
> > diff --git a/net/vmw_vsock/vsock_loopback.c b/net/vmw_vsock/vsock_loopback.c
> > index a8f218f0c5a3..474083d4cfcb 100644
> > --- a/net/vmw_vsock/vsock_loopback.c
> > +++ b/net/vmw_vsock/vsock_loopback.c
> > @@ -28,8 +28,16 @@ static u32 vsock_loopback_get_local_cid(void)
> >
> > static int vsock_loopback_send_pkt(struct sk_buff *skb)
> > {
> > - struct vsock_loopback *vsock = &the_vsock_loopback;
> > + struct vsock_loopback *vsock;
> > int len = skb->len;
> > + struct net *net;
> > +
> > + net = virtio_vsock_skb_net(skb);
> > +
> > + if (virtio_vsock_skb_net_mode(skb) == VSOCK_NET_MODE_LOCAL)
> > + vsock = (struct vsock_loopback *)net->vsock.priv;
>
> Is there some kind of refcount on the net?
> What I mean is, are we sure this pointer is still valid? Could the net
> disappear in the meantime?
I only considered the case of net being removed, which I think is okay
because user sockets take a net reference in sk_alloc(), and we can't
reach this point after the sock is destroyed and the reference is
released because the transport will be unassigned prior.
But... I'm now realizing there is the case of
virtio_transport_reset_no_sock() where skb net is null. I can't see why
that wouldn't be possible for loopback?
Let's handle that case to be sure...
>
> The rest LGTM!
>
> Thanks, Stefano
>
Best,
Bobby
^ permalink raw reply
* Re: [PATCH net-next v8 04/14] vsock: add netns to vsock core
From: Bobby Eshleman @ 2025-11-07 2:03 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Shuah Khan, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Stefan Hajnoczi, Michael S. Tsirkin,
Jason Wang, Xuan Zhuo, Eugenio Pérez, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Bryan Tan, Vishnu Dasa,
Broadcom internal kernel review list, virtualization, netdev,
linux-kselftest, linux-kernel, kvm, linux-hyperv, berrange,
Bobby Eshleman
In-Reply-To: <zxy7e4xihxujtlcnqjdgfxaqckfurop77eukbose74nzaxyv64@7djyz3gv4eys>
On Thu, Nov 06, 2025 at 05:18:00PM +0100, Stefano Garzarella wrote:
> On Thu, Oct 23, 2025 at 11:27:43AM -0700, Bobby Eshleman wrote:
> > From: Bobby Eshleman <bobbyeshleman@meta.com>
> >
> > Add netns logic to vsock core. Additionally, modify transport hook
> > prototypes to be used by later transport-specific patches (e.g.,
> > *_seqpacket_allow()).
> >
> > Namespaces are supported primarily by changing socket lookup functions
> > (e.g., vsock_find_connected_socket()) to take into account the socket
> > namespace and the namespace mode before considering a candidate socket a
> > "match".
> >
> > Introduce a dummy namespace struct, __vsock_global_dummy_net, to be
> > used by transports that do not support namespacing. This dummy always
> > has mode "global" to preserve previous CID behavior.
> >
> > This patch also introduces the sysctl /proc/sys/net/vsock/ns_mode that
> > accepts the "global" or "local" mode strings.
> >
> > The transports (besides vhost) are modified to use the global dummy,
> > which makes them behave as if always in the global namespace. Vhost is
> > an exception because it inherits its namespace from the process that
> > opens the vhost device.
> >
> > Add netns functionality (initialization, passing to transports, procfs,
> > etc...) to the af_vsock socket layer. Later patches that add netns
> > support to transports depend on this patch.
> >
> > seqpacket_allow() callbacks are modified to take a vsk so that transport
> > implementations can inspect sock_net(sk) and vsk->net_mode when performing
> > lookups (e.g., vhost does this in its future netns patch). Because the
> > API change affects all transports, it seemed more appropriate to make
> > this internal API change in the "vsock core" patch then in the "vhost"
> > patch.
> >
> > Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com>
> > ---
> > Changes in v7:
> > - hv_sock: fix hyperv build error
> > - explain why vhost does not use the dummy
> > - explain usage of __vsock_global_dummy_net
> > - explain why VSOCK_NET_MODE_STR_MAX is 8 characters
> > - use switch-case in vsock_net_mode_string()
> > - avoid changing transports as much as possible
> > - add vsock_find_{bound,connected}_socket_net()
> > - rename `vsock_hdr` to `sysctl_hdr`
> > - add virtio_vsock_alloc_linear_skb() wrapper for setting dummy net and
> > global mode for virtio-vsock, move skb->cb zero-ing into wrapper
> > - explain seqpacket_allow() change
> > - move net setting to __vsock_create() instead of vsock_create() so
> > that child sockets also have their net assigned upon accept()
> >
> > Changes in v6:
> > - unregister sysctl ops in vsock_exit()
> > - af_vsock: clarify description of CID behavior
> > - af_vsock: fix buf vs buffer naming, and length checking
> > - af_vsock: fix length checking w/ correct ctl_table->maxlen
> >
> > Changes in v5:
> > - vsock_global_net() -> vsock_global_dummy_net()
> > - update comments for new uAPI
> > - use /proc/sys/net/vsock/ns_mode instead of /proc/net/vsock_ns_mode
> > - add prototype changes so patch remains compilable
> > ---
> > drivers/vhost/vsock.c | 4 +-
> > include/linux/virtio_vsock.h | 21 ++++
> > include/net/af_vsock.h | 14 ++-
> > net/vmw_vsock/af_vsock.c | 264 ++++++++++++++++++++++++++++++++++++---
> > net/vmw_vsock/virtio_transport.c | 7 +-
> > net/vmw_vsock/vsock_loopback.c | 4 +-
> > 6 files changed, 288 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> > index ae01457ea2cd..34adf0cf9124 100644
> > --- a/drivers/vhost/vsock.c
> > +++ b/drivers/vhost/vsock.c
> > @@ -404,7 +404,7 @@ static bool vhost_transport_msgzerocopy_allow(void)
> > return true;
> > }
> >
> > -static bool vhost_transport_seqpacket_allow(u32 remote_cid);
> > +static bool vhost_transport_seqpacket_allow(struct vsock_sock *vsk, u32 remote_cid);
> >
> > static struct virtio_transport vhost_transport = {
> > .transport = {
> > @@ -460,7 +460,7 @@ static struct virtio_transport vhost_transport = {
> > .send_pkt = vhost_transport_send_pkt,
> > };
> >
> > -static bool vhost_transport_seqpacket_allow(u32 remote_cid)
> > +static bool vhost_transport_seqpacket_allow(struct vsock_sock *vsk, u32 remote_cid)
> > {
> > struct vhost_vsock *vsock;
> > bool seqpacket_allow = false;
> > diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
> > index 7f334a32133c..29290395054c 100644
> > --- a/include/linux/virtio_vsock.h
> > +++ b/include/linux/virtio_vsock.h
> > @@ -153,6 +153,27 @@ static inline void virtio_vsock_skb_set_net_mode(struct sk_buff *skb,
> > VIRTIO_VSOCK_SKB_CB(skb)->net_mode = net_mode;
> > }
> >
> > +static inline struct sk_buff *
> > +virtio_vsock_alloc_rx_skb(unsigned int size, gfp_t mask)
> > +{
> > + struct sk_buff *skb;
> > +
> > + skb = virtio_vsock_alloc_linear_skb(size, mask);
> > + if (!skb)
> > + return NULL;
> > +
> > + memset(skb->head, 0, VIRTIO_VSOCK_SKB_HEADROOM);
> > +
> > + /* virtio-vsock does not yet support namespaces, so on receive
> > + * we force legacy namespace behavior using the global dummy net
> > + * and global net mode.
> > + */
> > + virtio_vsock_skb_set_net(skb, vsock_global_dummy_net());
> > + virtio_vsock_skb_set_net_mode(skb, VSOCK_NET_MODE_GLOBAL);
> > +
> > + return skb;
> > +}
>
> Why we are introducing this change in this patch?
>
> Where the net of the virtio's skb is read?
>
Oh good point, this is a weird place for this. I'll move this to where
it is actually used.
[...]
> >
> > +static int vsock_net_mode_string(const struct ctl_table *table, int write,
> > + void *buffer, size_t *lenp, loff_t *ppos)
> > +{
> > + char data[VSOCK_NET_MODE_STR_MAX] = {0};
> > + enum vsock_net_mode mode;
> > + struct ctl_table tmp;
> > + struct net *net;
> > + int ret;
> > +
> > + if (!table->data || !table->maxlen || !*lenp) {
> > + *lenp = 0;
> > + return 0;
> > + }
> > +
> > + net = current->nsproxy->net_ns;
> > + tmp = *table;
> > + tmp.data = data;
> > +
> > + if (!write) {
> > + const char *p;
> > +
> > + mode = vsock_net_mode(net);
> > +
> > + switch (mode) {
> > + case VSOCK_NET_MODE_GLOBAL:
> > + p = VSOCK_NET_MODE_STR_GLOBAL;
> > + break;
> > + case VSOCK_NET_MODE_LOCAL:
> > + p = VSOCK_NET_MODE_STR_LOCAL;
> > + break;
> > + default:
> > + WARN_ONCE(true, "netns has invalid vsock mode");
> > + *lenp = 0;
> > + return 0;
> > + }
> > +
> > + strscpy(data, p, sizeof(data));
> > + tmp.maxlen = strlen(p);
> > + }
> > +
> > + ret = proc_dostring(&tmp, write, buffer, lenp, ppos);
> > + if (ret)
> > + return ret;
> > +
> > + if (write) {
>
> Do we need to check some capability, e.g. CAP_NET_ADMIN ?
>
We get that for free via the sysctl_net registration, through this path
on open (CAP_NET_ADMIN is checked in net_ctl_permissions):
net_ctl_permissions+1
sysctl_perm+24
proc_sys_permission+117
inode_permission+217
link_path_walk+162
path_openat+152
do_filp_open+171
do_sys_openat2+98
__x64_sys_openat+69
do_syscall_64+93
Verified with:
cp /bin/echo /tmp/echo_netadmin
setcap cap_net_admin+ep /tmp/echo_netadmin
(non-root user fails with regular echo, succeeds with
/tmp/echo_netadmin)
Best regards,
Bobby
^ permalink raw reply
* Re: [PATCH net-next v8 01/14] vsock: a per-net vsock NS mode state
From: Bobby Eshleman @ 2025-11-07 1:09 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Shuah Khan, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Stefan Hajnoczi, Michael S. Tsirkin,
Jason Wang, Xuan Zhuo, Eugenio Pérez, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Bryan Tan, Vishnu Dasa,
Broadcom internal kernel review list, virtualization, netdev,
linux-kselftest, linux-kernel, kvm, linux-hyperv, berrange,
Bobby Eshleman
In-Reply-To: <iiakzdk7n7onhu5sncjd7poh5sk34nrtvusbiulsel5uswuekv@p2yzmblg6xx7>
On Thu, Nov 06, 2025 at 05:16:29PM +0100, Stefano Garzarella wrote:
> On Thu, Oct 23, 2025 at 11:27:40AM -0700, Bobby Eshleman wrote:
> > From: Bobby Eshleman <bobbyeshleman@meta.com>
[...]
> > @@ -65,6 +66,7 @@ struct vsock_sock {
> > u32 peer_shutdown;
> > bool sent_request;
> > bool ignore_connecting_rst;
> > + enum vsock_net_mode net_mode;
> >
> > /* Protected by lock_sock(sk) */
> > u64 buffer_size;
> > @@ -256,4 +258,58 @@ static inline bool vsock_msgzerocopy_allow(const struct vsock_transport *t)
> > {
> > return t->msgzerocopy_allow && t->msgzerocopy_allow();
> > }
> > +
> > +static inline enum vsock_net_mode vsock_net_mode(struct net *net)
> > +{
> > + enum vsock_net_mode ret;
> > +
> > + spin_lock_bh(&net->vsock.lock);
> > + ret = net->vsock.mode;
>
> Do we really need a spin_lock just to set/get a variable?
> What about WRITE_ONCE/READ_ONCE and/or atomic ?
>
> Not a strong opinion, just to check if we can do something like this:
>
> static inline enum vsock_net_mode vsock_net_mode(struct net *net)
> {
> return READ_ONCE(net->vsock.mode);
> }
>
> static inline bool vsock_net_write_mode(struct net *net, u8 mode)
> {
> // Or using test_and_set_bit() if you prefer
> if (xchg(&net->vsock.mode_locked, true))
> return false;
>
> WRITE_ONCE(net->vsock.mode, mode);
> return true;
> }
>
I think that works and seems worth it to avoid the lock on the read
side. I'll move this over for the next rev.
[...]
Best,
Bobby
^ permalink raw reply
* Re: [PATCH net-next v8 00/14] vsock: add namespace support to vhost-vsock
From: Bobby Eshleman @ 2025-11-07 1:00 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Shuah Khan, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Stefan Hajnoczi, Michael S. Tsirkin,
Jason Wang, Xuan Zhuo, Eugenio Pérez, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Bryan Tan, Vishnu Dasa,
Broadcom internal kernel review list, virtualization, netdev,
linux-kselftest, linux-kernel, kvm, linux-hyperv, berrange,
Bobby Eshleman
In-Reply-To: <4vleifija3dfkvhvqixov5d6cefsr5wnwae74xwc5wz55wi6ic@su3h4ffnp3et>
On Thu, Nov 06, 2025 at 05:23:53PM +0100, Stefano Garzarella wrote:
> On Mon, Oct 27, 2025 at 10:25:29AM -0700, Bobby Eshleman wrote:
> > On Mon, Oct 27, 2025 at 02:28:31PM +0100, Stefano Garzarella wrote:
[...]
>
> I just reviewed the code changes. I skipped the selftest, since we are still
> discussing the other series (indeed I can't apply this anymore on top of
> that), so I'll check the rest later.
>
> Thanks for the great work!
>
> Stefano
>
I appreciate it! Thanks again for the work on your side reviewing.
I'll address your feedback and rebase onto that other series shortly.
Best,
Bobby
^ permalink raw reply
* RE: [PATCH v3] mshv: Extend create partition ioctl to support cpu features
From: Michael Kelley @ 2025-11-06 23:49 UTC (permalink / raw)
To: Nuno Das Neves, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, wei.liu@kernel.org,
muislam@microsoft.com, easwar.hariharan@linux.microsoft.com
Cc: kys@microsoft.com, haiyangz@microsoft.com, decui@microsoft.com,
longli@microsoft.com, skinsburskii@linux.microsoft.com,
romank@linux.microsoft.com, Jinank Jain
In-Reply-To: <66d77b7a-673f-4339-a4fe-81cadc31b018@linux.microsoft.com>
From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Thursday, November 6, 2025 9:42 AM
>
> On 11/5/2025 8:56 PM, Michael Kelley wrote:
> > From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Wednesday,
> November 5, 2025 11:10 AM
> >>
> >> On 11/5/2025 9:41 AM, Michael Kelley wrote:
> >>> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Tuesday, November 4, 2025 1:47 PM
> >>>>
> >>>> From: Muminul Islam <muislam@microsoft.com>
> >>>>
> >>>> The existing mshv create partition ioctl does not provide a way to
> >>>> specify which cpu features are enabled in the guest. Instead, it
> >>>> attempts to enable all features and those that are not supported are
> >>>> silently disabled by the hypervisor.
> >>>>
> >>>> This was done to reduce unnecessary complexity and is sufficient for
> >>>> many cases. However, new scenarios require fine-grained control over
> >>>> these features.
> >>>>
> >>>> Define a new mshv_create_partition_v2 structure which supports
> >>>> passing the disabled processor and xsave feature bits through to the
> >>>> create partition hypercall directly.
> >>>>
> >>>> The kernel does not introspect the bits in these new fields as they
> >>>> are part of the hypervisor ABI. Require the caller to provide the
> >>>> number of cpu feature banks passed, to support extending the number
> >>>> of banks in future. Disable all banks that are not specified to ensure
> >>>> the behavior is predictable with newer hypervisors.
> >>>>
> >>>> Introduce a new flag MSHV_PT_BIT_CPU_AND_XSAVE_FEATURES which enables
> >>>> the new structure. If unset, the original mshv_create_partition struct
> >>>> is used, with the old behavior of enabling all features.
> >>>>
> >>>> Co-developed-by: Jinank Jain <jinankjain@microsoft.com>
> >>>> Signed-off-by: Jinank Jain <jinankjain@microsoft.com>
> >>>> Signed-off-by: Muminul Islam <muislam@microsoft.com>
> >>>> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
> >>>> ---
> >>>> Changes in v3:
> >>>> - Remove the new cpu features definitions in hvhdk.h, and retain the
> >>>> old behavior of enabling all features for the old struct. For the v2
> >>>> struct, still disable unspecified feature banks, since that makes it
> >>>> robust to future extensions.
> >>>> - Amend comments and commit message to reflect the above
> >>>> - Fix unused variable on arm64 [kernel test robot]
> >>>>
> >>>> Changes in v2:
> >>>> - Fix exposure of CONFIG_X86_64 to uapi [kernel test robot]
> >>>> - Fix compilation issue on arm64 [kernel test robot]
> >>>>
> >>>> ---
> >>>> drivers/hv/mshv_root_main.c | 94 ++++++++++++++++++++++++++++++-------
> >>>> include/uapi/linux/mshv.h | 34 ++++++++++++++
> >>>> 2 files changed, 110 insertions(+), 18 deletions(-)
> >>>>
> >>>> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> >>>> index d542a0143bb8..814465a0912d 100644
> >>>> --- a/drivers/hv/mshv_root_main.c
> >>>> +++ b/drivers/hv/mshv_root_main.c
> >>>> @@ -1900,43 +1900,101 @@ add_partition(struct mshv_partition *partition)
> >>>> return 0;
> >>>> }
> >>>>
> >>>> -static long
> >>>> -mshv_ioctl_create_partition(void __user *user_arg, struct device *module_dev)
> >>>> +static_assert(MSHV_NUM_CPU_FEATURES_BANKS <=
> >>>> + HV_PARTITION_PROCESSOR_FEATURES_BANKS);
> >>>> +
> >>>> +static long mshv_ioctl_process_pt_flags(void __user *user_arg, u64 *pt_flags,
> >>>> + struct hv_partition_creation_properties *cr_props,
> >>>> + union hv_partition_isolation_properties *isol_props)
> >>>> {
> >>>> - struct mshv_create_partition args;
> >>>> - u64 creation_flags;
> >>>> - struct hv_partition_creation_properties creation_properties = {};
> >>>> - union hv_partition_isolation_properties isolation_properties = {};
> >>>> - struct mshv_partition *partition;
> >>>> - struct file *file;
> >>>> - int fd;
> >>>> - long ret;
> >>>> + int i;
> >>>> + struct mshv_create_partition_v2 args;
> >>>> + union hv_partition_processor_features *disabled_procs;
> >>>> + union hv_partition_processor_xsave_features *disabled_xsave;
> >>>>
> >>>> - if (copy_from_user(&args, user_arg, sizeof(args)))
> >>>> + /* First, copy orig struct in case user is on previous versions */
> >>>> + if (copy_from_user(&args, user_arg,
> >>>> + sizeof(struct mshv_create_partition)))
> >>>> return -EFAULT;
> >>>>
> >>>> if ((args.pt_flags & ~MSHV_PT_FLAGS_MASK) ||
> >>>> args.pt_isolation >= MSHV_PT_ISOLATION_COUNT)
> >>>> return -EINVAL;
> >>>>
> >>>> + disabled_procs = &cr_props->disabled_processor_features;
> >>>> + disabled_xsave = &cr_props->disabled_processor_xsave_features;
> >>>> +
> >>>> + /* Check if user provided newer struct with feature fields */
> >>>> + if (args.pt_flags & BIT(MSHV_PT_BIT_CPU_AND_XSAVE_FEATURES)) {
> >>>
> >>> Should probably be "BIT_ULL" instead of "BIT" since args.pt_flags is a long long field,
> >>> though it really doesn't matter as long as the number of flags is <= 32.
> >>>
> >> Noted, thanks
> >>
> >>>> + if (copy_from_user(&args, user_arg, sizeof(args)))
> >>>> + return -EFAULT;
> >>>> +
> >>>> + if (args.pt_num_cpu_fbanks > MSHV_NUM_CPU_FEATURES_BANKS ||
> >>>> + mshv_field_nonzero(args, pt_rsvd) ||
> >>>> + mshv_field_nonzero(args, pt_rsvd1))
> >>>> + return -EINVAL;
> >>>> +
> >>>> +
> >>>> + for (i = 0; i < args.pt_num_cpu_fbanks; i++)
> >>>> + disabled_procs->as_uint64[i] = args.pt_cpu_fbanks[i];
> >>>> +
> >>>> + /* Disable any features left unspecified */
> >>>> + for (; i < HV_PARTITION_PROCESSOR_FEATURES_BANKS; i++)
> >>>> + disabled_procs->as_uint64[i] = -1;
> >>>
> >>> I'm trying to convince myself that disabling unspecified features is the right
> >>> thing to do. In the current hypervisor scenario with 2 banks, if the VMM caller
> >>> specifies only one bank of disable flags, then all the features in the 2nd bank
> >>> are disabled. That's certainly the reverse from the current code which
> >>> always enables all features, and from the hypervisor itself which in the
> >>> hypercall ABI defines the flags as "disable" flags rather than "enable" flags.
> >>>
> >>> Then in a scenario where a new version of the hypervisor shows up with
> >>> support for 3 banks, the old VMM code that only knows about 2 banks
> >>> will cause all features in that 3rd bank to be disabled. Again, that's the
> >>> reverse of the current code.
> >>>
> >>> I guess it depends on how the hypervisor defines any such new features.
> >>> Are they typically defined to be benign if they are enabled by default? Or
> >>> is the polarity the opposite, where the VMM must know about new
> >>> features before they are enabled? The hypercall interface seems to imply
> >>> the former but maybe I'm reading too much into it.
> >>>
> >> The intent is to provide an interface which allows the VMM to control exactly
> >> which features are enabled/disabled. E.g. for live migration of a VM, if the
> >> target machine has more features available and they are enabled inadvertently,
> >> the state may not be restored properly (particularly an issue for xsave).
> >>
> >> So to me it makes sense to disable anything unspecified. In general, enabling
> >> features by default doesn't cause problems, it's only for specific scenarios
> >> like the above. I suppose that's why it's a "disable" mask, though I can't
> >> say I fully understand the reasoning...
> >>
> >>> A code comment about the thinking here would be useful for future readers.
> >>>
> >> Noted. I can repeat the reasoning from the commit message if that is
> >> sufficient:
> >> "
> >> Disable all banks that are not specified to ensure
> >> the behavior is predictable with newer hypervisors.
> >> "
> >
> > Before deciding on the wording of the comment, one more thought
> > occurred to me. union hv_partition_processor_features is currently
> > two 64-bit banks. Bank 0 currently has 63 features plus 1 reserved bit.
> > Bank 1 currently has 22 features, and 42 reserved bits. A new version
> > of the hypervisor could use one or more of those 42 reserved bits for
> > new features.
> >
> > If a VMM is running on a newer hypervisor version that implements
> > features the VMM is unaware of, the intent is that those features
> > should be disabled by default. So is the expectation that when a
> > VMM provides Bank 0 and Bank 1 values, it should set all the
> > reserved bits to 1? (currently the single bit in Bank 0 and the 42 bits
> > in Bank 1) My point is that the default disabling of new features can't
> > be handled entirely by the kernel implementation of the ioctl based
> > on the bank count passed in the argument to the ioctl. The VMM
> > must cooperate as well. And such splitting of the responsibility
> > seems rather messy.
> >
> > I see three cleaner alternatives:
> >
> > 1) Have the argument to the ioctl pass the "max known feature number"
> > instead of the bank count. Then the kernel implementation could set to
> > 1 all feature bits after that max. This alternative makes the kernel
> > fully responsible for doing the default disabling, based on what the
> > VMM tells the kernel it knows about.
> >
> > 2) Define the expected future Bank 2 and Bank 3 fields in
> > struct mshv_create_partition_v2, and require the VMM to set them to
> > all 1's as well as the reserved fields in Bank 0 and Bank 1. This alternative
> > makes the VMM fully responsible for doing the default disabling.
> >
> > 3) As a variant of my #2, invert the polarity of the bits in the pt_cpu_fbanks
> > field of the ioctl argument, so that from the VMM's standpoint they are
> > feature *enable* bits, not feature *disable* bits. The VMM sets bits for
> > the features it knows about and wants to have enabled, which is the
> > much more common pattern. Kernel code would then invert each bank
> > before passing to the hypercall. The Bank 2 and Bank 3 fields would be
> > set to zero by the VMM, and get the same kernel treatment when future
> > hypervisor versions accept the additional banks.
> >
> > Thoughts?
> >
>
> I discussed the future direction of this API with the hypervisor team.
> Surprisingly, it turns out they will add new feature banks but those
> will NOT be exposed at partition creation time. Rather, they will be
> set via the set partition property hypercall as an "early" property.
> That is, after partition creation but before initialization.
>
> This means that we will only need to ever expose 2 banks in this ioctl.
> I think requiring the VMM to provide both banks is reasonable, as well
> as not doing any default disabling in the kernel. i.e. Your suggested
> #2 but with only the 2 banks.
>
> Regarding #3, I want to keep the feature bits as a 'passthrough' field
> that the kernel doesn't touch, and anyway with a known number of banks
> this is unnecessary.
>
> On a related note, discussing the reasoning for the inversion in the
> first place, the hypervisor team told me the primary reason is so that
> new processor security features can be added to the mask and enabled
> by default when set to 0. This logic is a little funny given the
> hypervisor doesn't seem to care whether the reserved bits are 1 or 0
> today, but I guess they are meant to be always 0.
>
> In light of all this, now the reserved fields in the ioctl struct aren't
> serving any purpose. However, unfortunately I don't think I can remove
> them since userspace code already depends on this structure.
>
> I think pt_num_cpu_fbanks has to stay, too, although I think it's better
> to enforce that it is set to 2 by userspace and return -EINVAL otherwise.
>
> Does that all make sense or did I miss something?
Yes, this all makes sense, and I'm good with it. Leaving the reserved
fields in the ioctl structure is OK, as is requiring pt_num_cpu_fbanks
to be 2.
I was going to compare with how HVCALL_SET_PARTITION_PROPERTY is
handled with the potential for Bank 2 and Bank 3 to appear, but I see
that's a pass-thru hypercall whose inputs are constructed entirely in
the user space VMM. So there's no kernel code to compare against,
and the VMM code must "Do The Right Thing".
To summarize, the design of mshv_ioctl_create_partition() will be
to follow the behavior of the underlying hypercall. The hypercall
arguments (and therefore mshv_ioctl_create_partition) are designed
to allow the hypervisor to add new features, and have them be
enabled by default. The kernel code for mshv_ioctl_create_partition()
will pass-thru to the hypercall the Bank 0 and Bank 1 values provided by
the VMM in the ioctl. To achieve the "new features enabled by default"
behavior, the VMM should set the reserved bits in Bank 1 to zero. To
turn off the default enablement, a VMM must know about a feature
and explicitly decide to disable it by setting to 1 its assigned bit in
Bank 0 or Bank 1.
>
> This information about how the banks will be extended came as a surprise
> to me. I only brought it up with the hypervisor folks due to your
> questions here. This is a good thing, thanks for promoting this discussion.
Thx. I'm glad it was useful. Sometimes I wonder if I'm just being
annoying with my comments, but hopefully there are enough real issues
that just being annoying is rare. :-)
Michael
^ permalink raw reply
* [PATCH v2 2/2] mshv: Allow mappings that overlap in uaddr
From: Nuno Das Neves @ 2025-11-06 22:13 UTC (permalink / raw)
To: linux-hyperv, linux-kernel, mhklinux, magnuskulke
Cc: kys, haiyangz, wei.liu, decui, longli, skinsburskii, prapal,
mrathor, muislam, Nuno Das Neves
In-Reply-To: <1762467211-8213-1-git-send-email-nunodasneves@linux.microsoft.com>
From: Magnus Kulke <magnuskulke@linux.microsoft.com>
Currently the MSHV driver rejects mappings that would overlap in
userspace.
Some VMMs require the same memory to be mapped to different parts of
the guest's address space, and so working around this restriction is
difficult.
The hypervisor itself doesn't prohibit mappings that overlap in uaddr,
(really in SPA; system physical addresses), so supporting this in the
driver doesn't require any extra work: only the checks need to be
removed.
Since no userspace code until now has been able to overlap regions in
userspace, relaxing this constraint can't break any existing code.
Signed-off-by: Magnus Kulke <magnuskulke@linux.microsoft.com>
Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
---
drivers/hv/mshv_root_main.c | 8 ++------
include/uapi/linux/mshv.h | 2 +-
2 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
index 25a68912a78d..b1821b18fa09 100644
--- a/drivers/hv/mshv_root_main.c
+++ b/drivers/hv/mshv_root_main.c
@@ -1220,12 +1220,8 @@ static int mshv_partition_create_region(struct mshv_partition *partition,
/* Reject overlapping regions */
hlist_for_each_entry(rg, &partition->pt_mem_regions, hnode) {
- u64 rg_size = rg->nr_pages << HV_HYP_PAGE_SHIFT;
-
- if ((mem->guest_pfn + nr_pages <= rg->start_gfn ||
- rg->start_gfn + rg->nr_pages <= mem->guest_pfn) &&
- (mem->userspace_addr + mem->size <= rg->start_uaddr ||
- rg->start_uaddr + rg_size <= mem->userspace_addr))
+ if (mem->guest_pfn + nr_pages <= rg->start_gfn ||
+ rg->start_gfn + rg->nr_pages <= mem->guest_pfn)
continue;
return -EEXIST;
diff --git a/include/uapi/linux/mshv.h b/include/uapi/linux/mshv.h
index 9091946cba23..b10c8d1cb2ad 100644
--- a/include/uapi/linux/mshv.h
+++ b/include/uapi/linux/mshv.h
@@ -123,7 +123,7 @@ enum {
* @rsvd: MBZ
*
* Map or unmap a region of userspace memory to Guest Physical Addresses (GPA).
- * Mappings can't overlap in GPA space or userspace.
+ * Mappings can't overlap in GPA space.
* To unmap, these fields must match an existing mapping.
*/
struct mshv_user_mem_region {
--
2.34.1
^ permalink raw reply related
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