* [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
* [PATCH v2 1/2] mshv: Fix create memory region overlap check
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>
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
^ permalink raw reply related
* [PATCH v2 0/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
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
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 v4 1/2] dt-bindings: microsoft: Add vmbus message-connection-id property
From: Hardik Garg @ 2025-11-06 21:36 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: apais, cho, conor+dt, decui, devicetree, haiyangz, hargar,
krzk+dt, kys, linux-hyperv, linux-kernel, robh, ssengar, wei.liu
In-Reply-To: <69ed5b38-830d-46d7-a84b-86787c39df7d@kernel.org>
On 11/4/2025 10:55 PM, Krzysztof Kozlowski wrote:
> On 05/11/2025 02:10, Hardik Garg wrote:
>> Each guest has a private hypervisor mailbox and cannot access any other
>> guest’s communication path. Using an incorrect connection ID does not
>> allow eavesdropping or cause interference — it only results in failed
>> VMBus initialization because the host drops messages sent to an
>> unexpected port. Thus, exposing the correct connection ID to the guest
>> is safe and necessary for correct initialization.
>>
>>> If different values are important for the host, then all guests should
>>> use whatever 0 which will map to different values on host by other means
>>> of your protocol.
>>>
>> Using a fixed value such as 0 for all guests would not work, because the
>> Hyper-V host differentiates between multiple control-plane contexts (for
>> example, VTL0 vs VTL2) using distinct connection IDs. The guest must use
>> the value assigned by the host, as there is no implicit mapping or
>> negotiation protocol to determine it otherwise.
> Sorry, I am not going back to three months old discussion.
I understand your point and I apologize again for the delay in getting
back to you. It took me some time to explore alternative approaches and
verify whether the connection ID could be handled differently, but I’m
now done with that investigation. I’ll make sure to respond in a timely
manner going forward.
>
> Therefore I close this topic for me with: since the actual value does
> not matter for the host - it will discard all messages which are not
> intended to this guest - you can just use value 0 and your hypervisor
> will map to proper port.
The connection ID is managed entirely by the hypervisor and varies
depending on the VM’s configuration (for example, whether the control
plane is in VTL0 or VTL2). There is no existing mapping logic on the
guest side that could translate a constant value like 0 into the correct
port assignment. The host assigns distinct IDs for each configuration,
and the guest must use that specific value to establish the VMBus
control channel correctly.
Could you please confirm if I’m misunderstanding your suggestion about
the mapping mechanism? Without receiving the correct ID from the
hypervisor (via DT or another interface), the guest cannot infer the
right port on its own.
Also, since this thread is quite old, would you prefer that I start a new
thread when I resend the updated patch?
Thanks again for your feedback and patience.
Thanks,
Hardik
^ permalink raw reply
* Re: [PATCH] mshv: Allow mappings that overlap in uaddr
From: Nuno Das Neves @ 2025-11-06 18:40 UTC (permalink / raw)
To: Michael Kelley, 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: <SN6PR02MB41575BE0406D3AB22E1D7DB5D4C2A@SN6PR02MB4157.namprd02.prod.outlook.com>
On 11/6/2025 5:38 AM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Tuesday, November 4, 2025 2:19 PM
>>
>> 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 up until 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 | 19 +------------------
>> include/uapi/linux/mshv.h | 2 +-
>> 2 files changed, 2 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
>> index 814465a0912d..e5da5f2ab6f7 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
>> @@ -1235,9 +1220,7 @@ static int mshv_partition_create_region(struct mshv_partition *partition,
>>
>> /* 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))
>> + mshv_partition_region_by_gfn(partition, mem->guest_pfn + nr_pages - 1))
>> return -EEXIST;
>
> This existing code (and after this patch) checks for overlap by seeing if the
> requested starting and ending GFNs are already in some existing region. But
> is this really sufficient to detect overlap? Consider this example:
>
> 1. Three regions exist covering these GFNs respectively: 100 thru 199,
> 300 thru 399, and 500 thru 599.
> 2. A request is made to create a new region for GFNs 250 thru 449.
>
> This new request would pass the check, but would still overlap. Or is there
> something that prevents this scenario?
>
The logic appears wrong to me. I will create a patch to fix it, and amend this
patch to work with that new logic since it will look a little different. I'll
post the fix + v2 of this patch as a series.
Thanks
Nuno
>>
>> region = vzalloc(sizeof(*region) + sizeof(struct page *) * nr_pages);
>> 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
>
> I've given my Reviewed-by: narrowly for this patch, since it appears to be
> correct for what it does. But if the approach for detecting overlap really
> is faulty, an additional patch is needed that might supersede this one.
>
> Reviewed-by: Michael Kelley <mhklinux@outlook.com>
^ permalink raw reply
* Re: [PATCH v3] mshv: Extend create partition ioctl to support cpu features
From: Nuno Das Neves @ 2025-11-06 17:42 UTC (permalink / raw)
To: Michael Kelley, 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: <SN6PR02MB4157D85AE644F1DE8394BB98D4C2A@SN6PR02MB4157.namprd02.prod.outlook.com>
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?
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.
Nuno
> Michael
^ permalink raw reply
* Re: [PATCH v10 2/2] Drivers: hv: Introduce mshv_vtl driver
From: Naman Jain @ 2025-11-06 17:03 UTC (permalink / raw)
To: Michael Kelley, K . Y . Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, H . Peter Anvin
Cc: linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
x86@kernel.org, Peter Zijlstra, Sean Christopherson,
Paolo Bonzini, Mukesh Rathor, Stanislav Kinsburskii,
Nuno Das Neves, Christoph Hellwig, Saurabh Sengar, ALOK TIWARI
In-Reply-To: <SN6PR02MB41574847FF9B66A3D7321167D4C2A@SN6PR02MB4157.namprd02.prod.outlook.com>
On 11/6/2025 8:24 PM, Michael Kelley wrote:
> From: Naman Jain <namjain@linux.microsoft.com> Sent: Tuesday, October 28, 2025 10:02 PM
>>
>> Provide an interface for Virtual Machine Monitor like OpenVMM and its
>> use as OpenHCL paravisor to control VTL0 (Virtual trust Level).
>> Expose devices and support IOCTLs for features like VTL creation,
>> VTL0 memory management, context switch, making hypercalls,
>> mapping VTL0 address space to VTL2 userspace, getting new VMBus
>> messages and channel events in VTL2 etc.
>>
>> Co-developed-by: Roman Kisel <romank@linux.microsoft.com>
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>> Co-developed-by: Saurabh Sengar <ssengar@linux.microsoft.com>
>> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
>> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
>> ---
>> arch/x86/hyperv/Makefile | 10 +-
>> arch/x86/hyperv/hv_vtl.c | 43 +
>> arch/x86/hyperv/mshv-asm-offsets.c | 37 +
>> arch/x86/hyperv/mshv_vtl_asm.S | 98 ++
>> arch/x86/include/asm/mshyperv.h | 34 +
>> drivers/hv/Kconfig | 26 +-
>> drivers/hv/Makefile | 7 +-
>> drivers/hv/mshv_vtl.h | 25 +
>> drivers/hv/mshv_vtl_main.c | 1392 ++++++++++++++++++++++++++++
>> include/hyperv/hvgdk_mini.h | 106 +++
>> include/uapi/linux/mshv.h | 80 ++
>> 11 files changed, 1855 insertions(+), 3 deletions(-)
>> create mode 100644 arch/x86/hyperv/mshv-asm-offsets.c
>> create mode 100644 arch/x86/hyperv/mshv_vtl_asm.S
>> create mode 100644 drivers/hv/mshv_vtl.h
>> create mode 100644 drivers/hv/mshv_vtl_main.c
>>
>
> I've reviewed and made suggestions on most of this code pretty
> carefully over the past few months and through 10 revisions. This
> version addresses my suggestions and looks good to me. There
> are a few areas, such as the assembly code in mshv_vtl_asm.S and
> the details of the hypervisor ABI for doing VTL Return, that are
> outside my area of expertise so I'm limited to a surface level
> review.
>
> Reviewed-by: Michael Kelley <mhklinux@outlook.com>
Thanks for the review Michael.
Regards
Naman
^ permalink raw reply
* Re: [PATCH v10 1/2] Drivers: hv: Export some symbols for mshv_vtl
From: Naman Jain @ 2025-11-06 17:02 UTC (permalink / raw)
To: Peter Zijlstra
Cc: K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H . Peter Anvin, linux-hyperv, linux-kernel, x86,
Sean Christopherson, Paolo Bonzini, Michael Kelley, Mukesh Rathor,
Stanislav Kinsburskii, Nuno Das Neves, Christoph Hellwig,
Saurabh Sengar, ALOK TIWARI
In-Reply-To: <20251106144919.GT3245006@noisy.programming.kicks-ass.net>
On 11/6/2025 8:19 PM, Peter Zijlstra wrote:
> On Wed, Oct 29, 2025 at 05:01:38AM +0000, Naman Jain wrote:
>> MSHV_VTL driver is going to be introduced, which is supposed to
>> provide interface for Virtual Machine Monitors (VMMs) to control
>> Virtual Trust Level (VTL). Export the symbols needed
>> to make it work (vmbus_isr, hv_context and hv_post_message).
>
> Please consider using EXPORT_SYMBOL_FOR_MODULES()
>
>> +EXPORT_SYMBOL_GPL(hv_context);
>> +EXPORT_SYMBOL_GPL(hv_post_message);
>> +EXPORT_SYMBOL_GPL(vmbus_isr);
Thanks Peter. I can use that in the next version.
Regards,
Naman
^ permalink raw reply
* Re: [PATCH net-next v8 00/14] vsock: add namespace support to vhost-vsock
From: Stefano Garzarella @ 2025-11-06 16:23 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: <aP+rCQih4YMm1OAp@devvm11784.nha0.facebook.com>
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:
>> Hi Bobby,
>>
>> >
>> > Changes in v8:
>> > - Break generic cleanup/refactoring patches into standalone series,
>> > remove those from this series
>>
>> Yep, thanks for splitting the series. I'll review it ASAP since it's a
>> dependency.
>>
>> I was at GSoC mentor summit last week, so I'm bit busy with the backlog, but
>> I'll do my best to review both series this week.
>>
>> Thanks,
>> Stefano
>>
>
>Thanks for the heads up!
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
^ permalink raw reply
* Re: [PATCH net-next v8 07/14] vhost/vsock: add netns support
From: Stefano Garzarella @ 2025-11-06 16:21 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: <20251023-vsock-vmtest-v8-7-dea984d02bb0@meta.com>
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(-)
>
>diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
>index 34adf0cf9124..df6136633cd8 100644
>--- a/drivers/vhost/vsock.c
>+++ b/drivers/vhost/vsock.c
>@@ -46,6 +46,11 @@ static DEFINE_READ_MOSTLY_HASHTABLE(vhost_vsock_hash, 8);
> struct vhost_vsock {
> struct vhost_dev dev;
> struct vhost_virtqueue vqs[2];
>+ struct net *net;
>+ netns_tracker ns_tracker;
>+
>+ /* The ns mode at the time vhost_vsock was created */
>+ enum vsock_net_mode net_mode;
>
> /* Link to global vhost_vsock_hash, writes use vhost_vsock_mutex */
> struct hlist_node hash;
>@@ -67,7 +72,8 @@ static u32 vhost_transport_get_local_cid(void)
> /* Callers that dereference the return value must hold vhost_vsock_mutex or the
> * RCU read lock.
> */
>-static struct vhost_vsock *vhost_vsock_get(u32 guest_cid)
>+static struct vhost_vsock *vhost_vsock_get(u32 guest_cid, struct net *net,
>+ enum vsock_net_mode mode)
> {
> struct vhost_vsock *vsock;
>
>@@ -78,9 +84,9 @@ static struct vhost_vsock *vhost_vsock_get(u32 guest_cid)
> if (other_cid == 0)
> continue;
>
>- if (other_cid == guest_cid)
>+ if (other_cid == guest_cid &&
>+ vsock_net_check_mode(net, mode, vsock->net, vsock->net_mode))
> return vsock;
>-
> }
>
> return NULL;
>@@ -271,14 +277,16 @@ static void vhost_transport_send_pkt_work(struct vhost_work *work)
> static int
> vhost_transport_send_pkt(struct sk_buff *skb)
> {
>+ enum vsock_net_mode mode = virtio_vsock_skb_net_mode(skb);
> struct virtio_vsock_hdr *hdr = virtio_vsock_hdr(skb);
>+ struct net *net = virtio_vsock_skb_net(skb);
> struct vhost_vsock *vsock;
> int len = skb->len;
>
> rcu_read_lock();
>
> /* Find the vhost_vsock according to guest context id */
>- vsock = vhost_vsock_get(le64_to_cpu(hdr->dst_cid));
>+ vsock = vhost_vsock_get(le64_to_cpu(hdr->dst_cid), net, mode);
> if (!vsock) {
> rcu_read_unlock();
> kfree_skb(skb);
>@@ -305,7 +313,8 @@ vhost_transport_cancel_pkt(struct vsock_sock *vsk)
> rcu_read_lock();
>
> /* Find the vhost_vsock according to guest context id */
>- vsock = vhost_vsock_get(vsk->remote_addr.svm_cid);
>+ vsock = vhost_vsock_get(vsk->remote_addr.svm_cid,
>+ sock_net(sk_vsock(vsk)), vsk->net_mode);
> if (!vsock)
> goto out;
>
>@@ -327,7 +336,7 @@ vhost_transport_cancel_pkt(struct vsock_sock *vsk)
> }
>
> static struct sk_buff *
>-vhost_vsock_alloc_skb(struct vhost_virtqueue *vq,
>+vhost_vsock_alloc_skb(struct vhost_vsock *vsock, struct vhost_virtqueue *vq,
> unsigned int out, unsigned int in)
> {
> struct virtio_vsock_hdr *hdr;
>@@ -353,6 +362,9 @@ vhost_vsock_alloc_skb(struct vhost_virtqueue *vq,
> if (!skb)
> return NULL;
>
>+ virtio_vsock_skb_set_net(skb, vsock->net);
>+ virtio_vsock_skb_set_net_mode(skb, vsock->net_mode);
>+
> iov_iter_init(&iov_iter, ITER_SOURCE, vq->iov, out, len);
>
> hdr = virtio_vsock_hdr(skb);
>@@ -462,11 +474,12 @@ static struct virtio_transport vhost_transport = {
>
> static bool vhost_transport_seqpacket_allow(struct vsock_sock *vsk, u32 remote_cid)
> {
>+ struct net *net = sock_net(sk_vsock(vsk));
> struct vhost_vsock *vsock;
> bool seqpacket_allow = false;
>
> rcu_read_lock();
>- vsock = vhost_vsock_get(remote_cid);
>+ vsock = vhost_vsock_get(remote_cid, net, vsk->net_mode);
>
> if (vsock)
> seqpacket_allow = vsock->seqpacket_allow;
>@@ -520,7 +533,7 @@ static void vhost_vsock_handle_tx_kick(struct vhost_work *work)
> break;
> }
>
>- skb = vhost_vsock_alloc_skb(vq, out, in);
>+ skb = vhost_vsock_alloc_skb(vsock, vq, out, in);
> if (!skb) {
> vq_err(vq, "Faulted on pkt\n");
> continue;
>@@ -652,8 +665,10 @@ static void vhost_vsock_free(struct vhost_vsock *vsock)
>
> 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)
>+ 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?
And maybe we should document that also in the virtio_vsock_skb_cb.
The rest LGTM.
Thanks,
Stefano
> kfree(vsock->dev.vqs);
> vhost_vsock_free(vsock);
> return 0;
>@@ -779,7 +803,7 @@ static int vhost_vsock_set_cid(struct vhost_vsock *vsock, u64 guest_cid)
>
> /* Refuse if CID is already in use */
> mutex_lock(&vhost_vsock_mutex);
>- other = vhost_vsock_get(guest_cid);
>+ other = vhost_vsock_get(guest_cid, vsock->net, vsock->net_mode);
> if (other && other != vsock) {
> mutex_unlock(&vhost_vsock_mutex);
> return -EADDRINUSE;
>
>--
>2.47.3
>
^ permalink raw reply
* Re: [PATCH net-next v8 06/14] vsock/virtio: add netns to virtio transport common
From: Stefano Garzarella @ 2025-11-06 16:20 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: <20251023-vsock-vmtest-v8-6-dea984d02bb0@meta.com>
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?
>+ 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?
>+ virtio_vsock_skb_set_net_mode(skb, vsk->net_mode);
>+ }
>+
> return skb;
> out:
> kfree_skb(skb);
>@@ -527,6 +536,7 @@ static int virtio_transport_send_credit_update(struct vsock_sock *vsk)
> struct virtio_vsock_pkt_info info = {
> .op = VIRTIO_VSOCK_OP_CREDIT_UPDATE,
> .vsk = vsk,
>+ .net = sock_net(sk_vsock(vsk)),
> };
>
> return virtio_transport_send_pkt_info(vsk, &info);
>@@ -1067,6 +1077,7 @@ int virtio_transport_connect(struct vsock_sock *vsk)
> struct virtio_vsock_pkt_info info = {
> .op = VIRTIO_VSOCK_OP_REQUEST,
> .vsk = vsk,
>+ .net = sock_net(sk_vsock(vsk)),
> };
>
> return virtio_transport_send_pkt_info(vsk, &info);
>@@ -1082,6 +1093,7 @@ int virtio_transport_shutdown(struct vsock_sock *vsk, int mode)
> (mode & SEND_SHUTDOWN ?
> VIRTIO_VSOCK_SHUTDOWN_SEND : 0),
> .vsk = vsk,
>+ .net = sock_net(sk_vsock(vsk)),
> };
>
> return virtio_transport_send_pkt_info(vsk, &info);
>@@ -1108,6 +1120,7 @@ virtio_transport_stream_enqueue(struct vsock_sock *vsk,
> .msg = msg,
> .pkt_len = len,
> .vsk = vsk,
>+ .net = sock_net(sk_vsock(vsk)),
> };
>
> return virtio_transport_send_pkt_info(vsk, &info);
>@@ -1145,6 +1158,7 @@ static int virtio_transport_reset(struct vsock_sock *vsk,
> .op = VIRTIO_VSOCK_OP_RST,
> .reply = !!skb,
> .vsk = vsk,
>+ .net = sock_net(sk_vsock(vsk)),
> };
>
> /* Send RST only if the original pkt is not a RST pkt */
>@@ -1465,6 +1479,7 @@ virtio_transport_send_response(struct vsock_sock *vsk,
> .remote_port = le32_to_cpu(hdr->src_port),
> .reply = true,
> .vsk = vsk,
>+ .net = sock_net(sk_vsock(vsk)),
> };
>
> 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 :-)
Thanks,
Stefano
> struct sockaddr_vm src, dst;
> struct vsock_sock *vsk;
> struct sock *sk;
>@@ -1606,9 +1623,9 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
> /* The socket must be in connected or bound table
> * otherwise send reset back
> */
>- sk = vsock_find_connected_socket(&src, &dst);
>+ sk = vsock_find_connected_socket_net(&src, &dst, net, net_mode);
> if (!sk) {
>- sk = vsock_find_bound_socket(&dst);
>+ sk = vsock_find_bound_socket_net(&dst, net, net_mode);
> if (!sk) {
> (void)virtio_transport_reset_no_sock(t, skb);
> goto free_pkt;
>
>--
>2.47.3
>
^ permalink raw reply
* Re: [PATCH net-next v8 05/14] vsock/loopback: add netns support
From: Stefano Garzarella @ 2025-11-06 16:18 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: <20251023-vsock-vmtest-v8-5-dea984d02bb0@meta.com>
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?
>Changes in v7:
>- drop for_each_net() init/exit, drop net_rwsem, the pernet registration
> handles this automatically and race-free
>- flush workqueue before destruction, purge pkt list
>- remember net_mode instead of current net mode
>- keep space after INIT_WORK()
>- change vsock_loopback in netns_vsock to ->priv void ptr
>- rename `orig_net_mode` to `net_mode`
>- remove useless comment
>- protect `register_pernet_subsys()` with `net_rwsem`
>- do cleanup before releasing `net_rwsem` when failure happens
>- call `unregister_pernet_subsys()` in `vsock_loopback_exit()`
>- call `vsock_loopback_deinit_vsock()` in `vsock_loopback_exit()`
>
>Changes in v6:
>- init pernet ops for vsock_loopback module
>- vsock_loopback: add space in struct to clarify lock protection
>- do proper cleanup/unregister on vsock_loopback_exit()
>- vsock_loopback: use virtio_vsock_skb_net()
>
>Changes in v5:
>- add callbacks code to avoid reverse dependency
>- add logic for handling vsock_loopback setup for already existing
> namespaces
>---
> include/net/netns/vsock.h | 2 +
> net/vmw_vsock/vsock_loopback.c | 85 ++++++++++++++++++++++++++++++++++++------
> 2 files changed, 75 insertions(+), 12 deletions(-)
>
>diff --git a/include/net/netns/vsock.h b/include/net/netns/vsock.h
>index c9a438ad52f2..9d0d8e2fbc37 100644
>--- a/include/net/netns/vsock.h
>+++ b/include/net/netns/vsock.h
>@@ -16,5 +16,7 @@ struct netns_vsock {
> /* protected by lock */
> enum vsock_net_mode mode;
> bool mode_locked;
>+
>+ void *priv;
> };
> #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?
The rest LGTM!
Thanks,
Stefano
>+ else
>+ vsock = &the_vsock_loopback;
>
> virtio_vsock_skb_queue_tail(&vsock->pkt_queue, skb);
> queue_work(vsock->workqueue, &vsock->pkt_work);
>@@ -134,11 +142,8 @@ static void vsock_loopback_work(struct work_struct *work)
> }
> }
>
>-static int __init vsock_loopback_init(void)
>+static int vsock_loopback_init_vsock(struct vsock_loopback *vsock)
> {
>- struct vsock_loopback *vsock = &the_vsock_loopback;
>- int ret;
>-
> vsock->workqueue = alloc_workqueue("vsock-loopback", WQ_PERCPU, 0);
> if (!vsock->workqueue)
> return -ENOMEM;
>@@ -146,15 +151,73 @@ static int __init vsock_loopback_init(void)
> skb_queue_head_init(&vsock->pkt_queue);
> INIT_WORK(&vsock->pkt_work, vsock_loopback_work);
>
>+ return 0;
>+}
>+
>+static void vsock_loopback_deinit_vsock(struct vsock_loopback *vsock)
>+{
>+ if (vsock->workqueue) {
>+ flush_work(&vsock->pkt_work);
>+ virtio_vsock_skb_queue_purge(&vsock->pkt_queue);
>+ destroy_workqueue(vsock->workqueue);
>+ vsock->workqueue = NULL;
>+ }
>+}
>+
>+static int vsock_loopback_init_net(struct net *net)
>+{
>+ int ret;
>+
>+ net->vsock.priv = kzalloc(sizeof(struct vsock_loopback), GFP_KERNEL);
>+ if (!net->vsock.priv)
>+ return -ENOMEM;
>+
>+ ret = vsock_loopback_init_vsock((struct vsock_loopback *)net->vsock.priv);
>+ if (ret < 0) {
>+ kfree(net->vsock.priv);
>+ net->vsock.priv = NULL;
>+ return ret;
>+ }
>+
>+ return 0;
>+}
>+
>+static void vsock_loopback_exit_net(struct net *net)
>+{
>+ vsock_loopback_deinit_vsock(net->vsock.priv);
>+ kfree(net->vsock.priv);
>+ net->vsock.priv = NULL;
>+}
>+
>+static struct pernet_operations vsock_loopback_net_ops = {
>+ .init = vsock_loopback_init_net,
>+ .exit = vsock_loopback_exit_net,
>+};
>+
>+static int __init vsock_loopback_init(void)
>+{
>+ struct vsock_loopback *vsock = &the_vsock_loopback;
>+ int ret;
>+
>+ ret = vsock_loopback_init_vsock(vsock);
>+ if (ret < 0)
>+ return ret;
>+
>+ ret = register_pernet_subsys(&vsock_loopback_net_ops);
>+ if (ret < 0)
>+ goto out_deinit_vsock;
>+
> ret = vsock_core_register(&loopback_transport.transport,
> VSOCK_TRANSPORT_F_LOCAL);
> if (ret)
>- goto out_wq;
>+ goto out_unregister_pernet_subsys;
>
> return 0;
>
>-out_wq:
>- destroy_workqueue(vsock->workqueue);
>+out_unregister_pernet_subsys:
>+ unregister_pernet_subsys(&vsock_loopback_net_ops);
>+out_deinit_vsock:
>+ vsock_loopback_deinit_vsock(vsock);
> return ret;
> }
>
>@@ -164,11 +227,9 @@ static void __exit vsock_loopback_exit(void)
>
> vsock_core_unregister(&loopback_transport.transport);
>
>- flush_work(&vsock->pkt_work);
>-
>- virtio_vsock_skb_queue_purge(&vsock->pkt_queue);
>+ unregister_pernet_subsys(&vsock_loopback_net_ops);
>
>- destroy_workqueue(vsock->workqueue);
>+ vsock_loopback_deinit_vsock(vsock);
> }
>
> module_init(vsock_loopback_init);
>
>--
>2.47.3
>
^ permalink raw reply
* Re: [PATCH net-next v8 04/14] vsock: add netns to vsock core
From: Stefano Garzarella @ 2025-11-06 16:18 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: <20251023-vsock-vmtest-v8-4-dea984d02bb0@meta.com>
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?
>+
> /* Dimension the RX SKB so that the entire thing fits exactly into
> * a single 4KiB page. This avoids wasting memory due to alloc_skb()
> * rounding up to the next page order and also means that we
>diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>index bce5389ef742..69bb70c3c0fd 100644
>--- a/include/net/af_vsock.h
>+++ b/include/net/af_vsock.h
>@@ -145,7 +145,7 @@ struct vsock_transport {
> int flags);
> int (*seqpacket_enqueue)(struct vsock_sock *vsk, struct msghdr *msg,
> size_t len);
>- bool (*seqpacket_allow)(u32 remote_cid);
>+ bool (*seqpacket_allow)(struct vsock_sock *vsk, u32 remote_cid);
> u32 (*seqpacket_has_data)(struct vsock_sock *vsk);
>
> /* Notification. */
>@@ -218,6 +218,12 @@ void vsock_remove_connected(struct vsock_sock *vsk);
> struct sock *vsock_find_bound_socket(struct sockaddr_vm *addr);
> struct sock *vsock_find_connected_socket(struct sockaddr_vm *src,
> struct sockaddr_vm *dst);
>+struct sock *vsock_find_bound_socket_net(struct sockaddr_vm *addr, struct net *net,
>+ enum vsock_net_mode net_mode);
>+struct sock *vsock_find_connected_socket_net(struct sockaddr_vm *src,
>+ struct sockaddr_vm *dst,
>+ struct net *net,
>+ enum vsock_net_mode net_mode);
> void vsock_remove_sock(struct vsock_sock *vsk);
> void vsock_for_each_connected_socket(struct vsock_transport *transport,
> void (*fn)(struct sock *sk));
>@@ -259,6 +265,12 @@ static inline bool vsock_msgzerocopy_allow(const struct vsock_transport *t)
> return t->msgzerocopy_allow && t->msgzerocopy_allow();
> }
>
>+extern struct net __vsock_global_dummy_net;
>+static inline struct net *vsock_global_dummy_net(void)
>+{
>+ return &__vsock_global_dummy_net;
>+}
>+
> static inline enum vsock_net_mode vsock_net_mode(struct net *net)
> {
> enum vsock_net_mode ret;
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index 4c2db6cca557..656a78810c68 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -83,6 +83,35 @@
> * TCP_ESTABLISHED - connected
> * TCP_CLOSING - disconnecting
> * TCP_LISTEN - listening
>+ *
>+ * - Namespaces in vsock support two different modes configured
>+ * through /proc/sys/net/vsock/ns_mode. The modes are "local" and "global".
>+ * Each mode defines how the namespace interacts with CIDs.
>+ * /proc/sys/net/vsock/ns_mode is write-once, so that it may be configured
>+ * and locked down by a namespace manager. The default is "global". The mode
>+ * is set per-namespace.
>+ *
>+ * The modes affect the allocation and accessibility of CIDs as follows:
>+ *
>+ * - global - access and allocation are all system-wide
>+ * - all CID allocation from global namespaces draw from the same
>+ * system-wide pool
>+ * - if one global namespace has already allocated some CID, another
>+ * global namespace will not be able to allocate the same CID
>+ * - global mode AF_VSOCK sockets can reach any VM or socket in any global
>+ * namespace, they are not contained to only their own namespace
>+ * - AF_VSOCK sockets in a global mode namespace cannot reach VMs or
>+ * sockets in any local mode namespace
>+ * - local - access and allocation are contained within the namespace
>+ * - CID allocation draws only from a private pool local only to the
>+ * namespace, and does not affect the CIDs available for allocation in any
>+ * other namespace (global or local)
>+ * - VMs in a local namespace do not collide with CIDs in any other local
>+ * namespace or any global namespace. For example, if a VM in a local mode
>+ * namespace is given CID 10, then CID 10 is still available for
>+ * allocation in any other namespace, but not in the same namespace
>+ * - AF_VSOCK sockets in a local mode namespace can connect only to VMs or
>+ * other sockets within their own namespace.
> */
>
> #include <linux/compat.h>
>@@ -100,6 +129,7 @@
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/net.h>
>+#include <linux/proc_fs.h>
> #include <linux/poll.h>
> #include <linux/random.h>
> #include <linux/skbuff.h>
>@@ -111,9 +141,18 @@
> #include <linux/workqueue.h>
> #include <net/sock.h>
> #include <net/af_vsock.h>
>+#include <net/netns/vsock.h>
> #include <uapi/linux/vm_sockets.h>
> #include <uapi/asm-generic/ioctls.h>
>
>+#define VSOCK_NET_MODE_STR_GLOBAL "global"
>+#define VSOCK_NET_MODE_STR_LOCAL "local"
>+
>+/* 6 chars for "global", 1 for null-terminator, and 1 more for '\n'.
>+ * The newline is added by proc_dostring() for read operations.
>+ */
>+#define VSOCK_NET_MODE_STR_MAX 8
>+
> static int __vsock_bind(struct sock *sk, struct sockaddr_vm *addr);
> static void vsock_sk_destruct(struct sock *sk);
> static int vsock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb);
>@@ -149,6 +188,15 @@ static const struct vsock_transport *transport_dgram;
> static const struct vsock_transport *transport_local;
> static DEFINE_MUTEX(vsock_register_mutex);
>
>+/* This net is used only for transports that do support namespaces. It is never
>+ * registered with the namespace subsystem and always has
>+ * VSOCK_NET_MODE_GLOBAL. Pass this net to the net lookup functions (e.g.,
>+ * vsock_find_bound_socket_net()) when you want to force global-mode or the
>+ * same behavior as before namespaces were supported.
>+ */
>+struct net __vsock_global_dummy_net;
>+EXPORT_SYMBOL_GPL(__vsock_global_dummy_net);
>+
> /**** UTILS ****/
>
> /* Each bound VSocket is stored in the bind hash table and each connected
>@@ -235,33 +283,44 @@ static void __vsock_remove_connected(struct vsock_sock *vsk)
> sock_put(&vsk->sk);
> }
>
>-static struct sock *__vsock_find_bound_socket(struct sockaddr_vm *addr)
>+static struct sock *__vsock_find_bound_socket_net(struct sockaddr_vm *addr,
>+ struct net *net,
>+ enum vsock_net_mode net_mode)
> {
> struct vsock_sock *vsk;
>
> list_for_each_entry(vsk, vsock_bound_sockets(addr), bound_table) {
>- if (vsock_addr_equals_addr(addr, &vsk->local_addr))
>- return sk_vsock(vsk);
>+ struct sock *sk = sk_vsock(vsk);
>+
>+ if (vsock_addr_equals_addr(addr, &vsk->local_addr) &&
>+ vsock_net_check_mode(sock_net(sk), vsk->net_mode, net, net_mode))
>+ return sk;
>
> if (addr->svm_port == vsk->local_addr.svm_port &&
> (vsk->local_addr.svm_cid == VMADDR_CID_ANY ||
>- addr->svm_cid == VMADDR_CID_ANY))
>- return sk_vsock(vsk);
>+ addr->svm_cid == VMADDR_CID_ANY) &&
>+ vsock_net_check_mode(sock_net(sk), vsk->net_mode, net, net_mode))
>+ return sk;
> }
>
> return NULL;
> }
>
>-static struct sock *__vsock_find_connected_socket(struct sockaddr_vm *src,
>- struct sockaddr_vm *dst)
>+static struct sock *__vsock_find_connected_socket_net(struct sockaddr_vm *src,
>+ struct sockaddr_vm *dst,
>+ struct net *net,
>+ enum vsock_net_mode net_mode)
> {
> struct vsock_sock *vsk;
>
> list_for_each_entry(vsk, vsock_connected_sockets(src, dst),
> connected_table) {
>+ struct sock *sk = sk_vsock(vsk);
>+
> if (vsock_addr_equals_addr(src, &vsk->remote_addr) &&
>- dst->svm_port == vsk->local_addr.svm_port) {
>- return sk_vsock(vsk);
>+ dst->svm_port == vsk->local_addr.svm_port &&
>+ vsock_net_check_mode(sock_net(sk), vsk->net_mode, net, net_mode)) {
>+ return sk;
> }
> }
>
>@@ -304,12 +363,14 @@ void vsock_remove_connected(struct vsock_sock *vsk)
> }
> EXPORT_SYMBOL_GPL(vsock_remove_connected);
>
>-struct sock *vsock_find_bound_socket(struct sockaddr_vm *addr)
>+struct sock *vsock_find_bound_socket_net(struct sockaddr_vm *addr,
>+ struct net *net,
>+ enum vsock_net_mode net_mode)
> {
> struct sock *sk;
>
> spin_lock_bh(&vsock_table_lock);
>- sk = __vsock_find_bound_socket(addr);
>+ sk = __vsock_find_bound_socket_net(addr, net, net_mode);
> if (sk)
> sock_hold(sk);
>
>@@ -317,15 +378,24 @@ struct sock *vsock_find_bound_socket(struct sockaddr_vm *addr)
>
> return sk;
> }
>+EXPORT_SYMBOL_GPL(vsock_find_bound_socket_net);
>+
>+struct sock *vsock_find_bound_socket(struct sockaddr_vm *addr)
>+{
>+ return vsock_find_bound_socket_net(addr, vsock_global_dummy_net(),
>+ VSOCK_NET_MODE_GLOBAL);
>+}
> EXPORT_SYMBOL_GPL(vsock_find_bound_socket);
>
>-struct sock *vsock_find_connected_socket(struct sockaddr_vm *src,
>- struct sockaddr_vm *dst)
>+struct sock *vsock_find_connected_socket_net(struct sockaddr_vm *src,
>+ struct sockaddr_vm *dst,
>+ struct net *net,
>+ enum vsock_net_mode net_mode)
> {
> struct sock *sk;
>
> spin_lock_bh(&vsock_table_lock);
>- sk = __vsock_find_connected_socket(src, dst);
>+ sk = __vsock_find_connected_socket_net(src, dst, net, net_mode);
> if (sk)
> sock_hold(sk);
>
>@@ -333,6 +403,15 @@ struct sock *vsock_find_connected_socket(struct sockaddr_vm *src,
>
> return sk;
> }
>+EXPORT_SYMBOL_GPL(vsock_find_connected_socket_net);
>+
>+struct sock *vsock_find_connected_socket(struct sockaddr_vm *src,
>+ struct sockaddr_vm *dst)
>+{
>+ return vsock_find_connected_socket_net(src, dst,
>+ vsock_global_dummy_net(),
>+ VSOCK_NET_MODE_GLOBAL);
>+}
> EXPORT_SYMBOL_GPL(vsock_find_connected_socket);
>
> void vsock_remove_sock(struct vsock_sock *vsk)
>@@ -528,7 +607,7 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>
> if (sk->sk_type == SOCK_SEQPACKET) {
> if (!new_transport->seqpacket_allow ||
>- !new_transport->seqpacket_allow(remote_cid)) {
>+ !new_transport->seqpacket_allow(vsk, remote_cid)) {
> module_put(new_transport->module);
> return -ESOCKTNOSUPPORT;
> }
>@@ -676,6 +755,7 @@ static void vsock_pending_work(struct work_struct *work)
> static int __vsock_bind_connectible(struct vsock_sock *vsk,
> struct sockaddr_vm *addr)
> {
>+ struct net *net = sock_net(sk_vsock(vsk));
> static u32 port;
> struct sockaddr_vm new_addr;
>
>@@ -695,7 +775,8 @@ static int __vsock_bind_connectible(struct vsock_sock *vsk,
>
> new_addr.svm_port = port++;
>
>- if (!__vsock_find_bound_socket(&new_addr)) {
>+ if (!__vsock_find_bound_socket_net(&new_addr, net,
>+ vsk->net_mode)) {
> found = true;
> break;
> }
>@@ -712,7 +793,8 @@ static int __vsock_bind_connectible(struct vsock_sock *vsk,
> return -EACCES;
> }
>
>- if (__vsock_find_bound_socket(&new_addr))
>+ if (__vsock_find_bound_socket_net(&new_addr, net,
>+ vsk->net_mode))
> return -EADDRINUSE;
> }
>
>@@ -836,6 +918,8 @@ static struct sock *__vsock_create(struct net *net,
> vsk->buffer_max_size = VSOCK_DEFAULT_BUFFER_MAX_SIZE;
> }
>
>+ vsk->net_mode = vsock_net_mode(net);
>+
> return sk;
> }
>
>@@ -2636,6 +2720,142 @@ static struct miscdevice vsock_device = {
> .fops = &vsock_device_ops,
> };
>
>+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 ?
The rest LGTM!
Stefano
>+ if (*lenp >= sizeof(data))
>+ return -EINVAL;
>+
>+ if (!strncmp(data, VSOCK_NET_MODE_STR_GLOBAL, sizeof(data)))
>+ mode = VSOCK_NET_MODE_GLOBAL;
>+ else if (!strncmp(data, VSOCK_NET_MODE_STR_LOCAL, sizeof(data)))
>+ mode = VSOCK_NET_MODE_LOCAL;
>+ else
>+ return -EINVAL;
>+
>+ if (!vsock_net_write_mode(net, mode))
>+ return -EPERM;
>+ }
>+
>+ return 0;
>+}
>+
>+static struct ctl_table vsock_table[] = {
>+ {
>+ .procname = "ns_mode",
>+ .data = &init_net.vsock.mode,
>+ .maxlen = VSOCK_NET_MODE_STR_MAX,
>+ .mode = 0644,
>+ .proc_handler = vsock_net_mode_string
>+ },
>+};
>+
>+static int __net_init vsock_sysctl_register(struct net *net)
>+{
>+ struct ctl_table *table;
>+
>+ if (net_eq(net, &init_net)) {
>+ table = vsock_table;
>+ } else {
>+ table = kmemdup(vsock_table, sizeof(vsock_table), GFP_KERNEL);
>+ if (!table)
>+ goto err_alloc;
>+
>+ table[0].data = &net->vsock.mode;
>+ }
>+
>+ net->vsock.sysctl_hdr = register_net_sysctl_sz(net, "net/vsock", table,
>+ ARRAY_SIZE(vsock_table));
>+ if (!net->vsock.sysctl_hdr)
>+ goto err_reg;
>+
>+ return 0;
>+
>+err_reg:
>+ if (!net_eq(net, &init_net))
>+ kfree(table);
>+err_alloc:
>+ return -ENOMEM;
>+}
>+
>+static void vsock_sysctl_unregister(struct net *net)
>+{
>+ const struct ctl_table *table;
>+
>+ table = net->vsock.sysctl_hdr->ctl_table_arg;
>+ unregister_net_sysctl_table(net->vsock.sysctl_hdr);
>+ if (!net_eq(net, &init_net))
>+ kfree(table);
>+}
>+
>+static void vsock_net_init(struct net *net)
>+{
>+ spin_lock_init(&net->vsock.lock);
>+ net->vsock.mode = VSOCK_NET_MODE_GLOBAL;
>+}
>+
>+static __net_init int vsock_sysctl_init_net(struct net *net)
>+{
>+ vsock_net_init(net);
>+
>+ if (vsock_sysctl_register(net))
>+ return -ENOMEM;
>+
>+ return 0;
>+}
>+
>+static __net_exit void vsock_sysctl_exit_net(struct net *net)
>+{
>+ vsock_sysctl_unregister(net);
>+}
>+
>+static struct pernet_operations vsock_sysctl_ops __net_initdata = {
>+ .init = vsock_sysctl_init_net,
>+ .exit = vsock_sysctl_exit_net,
>+};
>+
> static int __init vsock_init(void)
> {
> int err = 0;
>@@ -2663,10 +2883,19 @@ static int __init vsock_init(void)
> goto err_unregister_proto;
> }
>
>+ if (register_pernet_subsys(&vsock_sysctl_ops)) {
>+ err = -ENOMEM;
>+ goto err_unregister_sock;
>+ }
>+
>+ vsock_net_init(&init_net);
>+ vsock_net_init(vsock_global_dummy_net());
> vsock_bpf_build_proto();
>
> return 0;
>
>+err_unregister_sock:
>+ sock_unregister(AF_VSOCK);
> err_unregister_proto:
> proto_unregister(&vsock_proto);
> err_deregister_misc:
>@@ -2680,6 +2909,7 @@ static void __exit vsock_exit(void)
> misc_deregister(&vsock_device);
> sock_unregister(AF_VSOCK);
> proto_unregister(&vsock_proto);
>+ unregister_pernet_subsys(&vsock_sysctl_ops);
> }
>
> const struct vsock_transport *vsock_core_get_transport(struct vsock_sock *vsk)
>diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
>index 8c867023a2e5..6abec6b9b5bc 100644
>--- a/net/vmw_vsock/virtio_transport.c
>+++ b/net/vmw_vsock/virtio_transport.c
>@@ -316,11 +316,10 @@ static void virtio_vsock_rx_fill(struct virtio_vsock *vsock)
> vq = vsock->vqs[VSOCK_VQ_RX];
>
> do {
>- skb = virtio_vsock_alloc_linear_skb(total_len, GFP_KERNEL);
>+ skb = virtio_vsock_alloc_rx_skb(total_len, GFP_KERNEL);
> if (!skb)
> break;
>
>- memset(skb->head, 0, VIRTIO_VSOCK_SKB_HEADROOM);
> sg_init_one(&pkt, virtio_vsock_hdr(skb), total_len);
> p = &pkt;
> ret = virtqueue_add_sgs(vq, &p, 0, 1, skb, GFP_KERNEL);
>@@ -536,7 +535,7 @@ static bool virtio_transport_msgzerocopy_allow(void)
> return true;
> }
>
>-static bool virtio_transport_seqpacket_allow(u32 remote_cid);
>+static bool virtio_transport_seqpacket_allow(struct vsock_sock *vsk, u32 remote_cid);
>
> static struct virtio_transport virtio_transport = {
> .transport = {
>@@ -593,7 +592,7 @@ static struct virtio_transport virtio_transport = {
> .can_msgzerocopy = virtio_transport_can_msgzerocopy,
> };
>
>-static bool virtio_transport_seqpacket_allow(u32 remote_cid)
>+static bool virtio_transport_seqpacket_allow(struct vsock_sock *vsk, u32 remote_cid)
> {
> struct virtio_vsock *vsock;
> bool seqpacket_allow;
>diff --git a/net/vmw_vsock/vsock_loopback.c b/net/vmw_vsock/vsock_loopback.c
>index bc2ff918b315..a8f218f0c5a3 100644
>--- a/net/vmw_vsock/vsock_loopback.c
>+++ b/net/vmw_vsock/vsock_loopback.c
>@@ -46,7 +46,7 @@ static int vsock_loopback_cancel_pkt(struct vsock_sock *vsk)
> return 0;
> }
>
>-static bool vsock_loopback_seqpacket_allow(u32 remote_cid);
>+static bool vsock_loopback_seqpacket_allow(struct vsock_sock *vsk, u32 remote_cid);
> static bool vsock_loopback_msgzerocopy_allow(void)
> {
> return true;
>@@ -106,7 +106,7 @@ static struct virtio_transport loopback_transport = {
> .send_pkt = vsock_loopback_send_pkt,
> };
>
>-static bool vsock_loopback_seqpacket_allow(u32 remote_cid)
>+static bool vsock_loopback_seqpacket_allow(struct vsock_sock *vsk, u32 remote_cid)
> {
> return true;
> }
>
>--
>2.47.3
>
^ permalink raw reply
* Re: [PATCH net-next v8 03/14] vsock: add netns to vsock skb cb
From: Stefano Garzarella @ 2025-11-06 16:17 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: <20251023-vsock-vmtest-v8-3-dea984d02bb0@meta.com>
On Thu, Oct 23, 2025 at 11:27:42AM -0700, Bobby Eshleman wrote:
>From: Bobby Eshleman <bobbyeshleman@meta.com>
>
>Add a net pointer and net_mode to the vsock skb and helpers for
>getting/setting them. When skbs are received the transport needs a way
>to tell the vsock layer and/or virtio common layer which namespace and
>what namespace mode the packet belongs to. This will be used by those
>upper layers for finding the correct socket object. This patch stashes
>these fields in the skb control buffer.
>
>This extends virtio_vsock_skb_cb to 24 bytes:
>
>struct virtio_vsock_skb_cb {
> struct net * net; /* 0 8 */
> enum vsock_net_mode net_mode; /* 8 4 */
> u32 offset; /* 12 4 */
> bool reply; /* 16 1 */
> bool tap_delivered; /* 17 1 */
>
> /* size: 24, cachelines: 1, members: 5 */
> /* padding: 6 */
> /* last cacheline: 24 bytes */
>};
>
>Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com>
>---
>Changes in v7:
>- rename `orig_net_mode` to `net_mode`
>- update commit message with a more complete explanation of changes
>
>Changes in v5:
>- some diff context change due to rebase to current net-next
>---
> include/linux/virtio_vsock.h | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>
>diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>index 87cf4dcac78a..7f334a32133c 100644
>--- a/include/linux/virtio_vsock.h
>+++ b/include/linux/virtio_vsock.h
>@@ -10,6 +10,8 @@
> #define VIRTIO_VSOCK_SKB_HEADROOM (sizeof(struct virtio_vsock_hdr))
>
> struct virtio_vsock_skb_cb {
>+ struct net *net;
>+ enum vsock_net_mode net_mode;
> u32 offset;
> bool reply;
> bool tap_delivered;
>@@ -130,6 +132,27 @@ static inline size_t virtio_vsock_skb_len(struct sk_buff *skb)
> return (size_t)(skb_end_pointer(skb) - skb->head);
> }
>
>+static inline struct net *virtio_vsock_skb_net(struct sk_buff *skb)
>+{
>+ return VIRTIO_VSOCK_SKB_CB(skb)->net;
>+}
>+
>+static inline void virtio_vsock_skb_set_net(struct sk_buff *skb, struct net *net)
>+{
>+ VIRTIO_VSOCK_SKB_CB(skb)->net = net;
>+}
>+
>+static inline enum vsock_net_mode virtio_vsock_skb_net_mode(struct sk_buff *skb)
>+{
>+ return VIRTIO_VSOCK_SKB_CB(skb)->net_mode;
>+}
>+
>+static inline void virtio_vsock_skb_set_net_mode(struct sk_buff *skb,
>+ enum vsock_net_mode net_mode)
>+{
>+ VIRTIO_VSOCK_SKB_CB(skb)->net_mode = net_mode;
>+}
>+
> /* Dimension the RX SKB so that the entire thing fits exactly into
> * a single 4KiB page. This avoids wasting memory due to alloc_skb()
> * rounding up to the next page order and also means that we
>
>--
>2.47.3
>
^ permalink raw reply
* Re: [PATCH net-next v8 02/14] vsock/virtio: pack struct virtio_vsock_skb_cb
From: Stefano Garzarella @ 2025-11-06 16:16 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: <20251023-vsock-vmtest-v8-2-dea984d02bb0@meta.com>
On Thu, Oct 23, 2025 at 11:27:41AM -0700, Bobby Eshleman wrote:
>From: Bobby Eshleman <bobbyeshleman@meta.com>
>
>Reduce holes in struct virtio_vsock_skb_cb. As this struct continues to
>grow, we want to keep it trimmed down so it doesn't exceed the size of
>skb->cb (currently 48 bytes). Eliminating the 2 byte hole provides an
>additional two bytes for new fields at the end of the structure. It does
>not shrink the total size, however.
>
>Future work could include combining fields like reply and tap_delivered
>into a single bitfield, but currently doing so will not make the total
>struct size smaller (although, would extend the tail-end padding area by
>one byte).
>
>Before this patch:
>
>struct virtio_vsock_skb_cb {
> bool reply; /* 0 1 */
> bool tap_delivered; /* 1 1 */
>
> /* XXX 2 bytes hole, try to pack */
>
> u32 offset; /* 4 4 */
>
> /* size: 8, cachelines: 1, members: 3 */
> /* sum members: 6, holes: 1, sum holes: 2 */
> /* last cacheline: 8 bytes */
>};
>;
>
>After this patch:
>
>struct virtio_vsock_skb_cb {
> u32 offset; /* 0 4 */
> bool reply; /* 4 1 */
> bool tap_delivered; /* 5 1 */
>
> /* size: 8, cachelines: 1, members: 3 */
> /* padding: 2 */
> /* last cacheline: 8 bytes */
>};
>
>Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com>
>---
> include/linux/virtio_vsock.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Yeah, thanks for that!
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>
>diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>index 0c67543a45c8..87cf4dcac78a 100644
>--- a/include/linux/virtio_vsock.h
>+++ b/include/linux/virtio_vsock.h
>@@ -10,9 +10,9 @@
> #define VIRTIO_VSOCK_SKB_HEADROOM (sizeof(struct virtio_vsock_hdr))
>
> struct virtio_vsock_skb_cb {
>+ u32 offset;
> bool reply;
> bool tap_delivered;
>- u32 offset;
> };
>
> #define VIRTIO_VSOCK_SKB_CB(skb) ((struct virtio_vsock_skb_cb *)((skb)->cb))
>
>--
>2.47.3
>
^ permalink raw reply
* Re: [PATCH net-next v8 01/14] vsock: a per-net vsock NS mode state
From: Stefano Garzarella @ 2025-11-06 16:16 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: <20251023-vsock-vmtest-v8-1-dea984d02bb0@meta.com>
On Thu, Oct 23, 2025 at 11:27:40AM -0700, Bobby Eshleman wrote:
>From: Bobby Eshleman <bobbyeshleman@meta.com>
>
>Add the per-net vsock NS mode state. This only adds the structure for
>holding the mode and some of the functions for setting/getting and
>checking the mode, but does not integrate the functionality yet.
>
>A "net_mode" field is added to vsock_sock to store the mode of the
>namespace when the vsock_sock was created. In order to evaluate
>namespace mode rules we need to know both a) which namespace the
>endpoints are in, and b) what mode that namespace had when the endpoints
>were created. This allows us to handle the changing of modes from global
>to local *after* a socket has been created by remembering that the mode
>was global when the socket was created. If we were to use the current
>net's mode instead, then the lookup would fail and the socket would
>break.
>
>Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com>
>---
>Changes in v7:
>- clarify vsock_net_check_mode() comments
>- change to `orig_net_mode == VSOCK_NET_MODE_GLOBAL && orig_net_mode == vsk->orig_net_mode`
>- remove extraneous explanation of `orig_net_mode`
>- rename `written` to `mode_locked`
>- rename `vsock_hdr` to `sysctl_hdr`
>- change `orig_net_mode` to `net_mode`
>- make vsock_net_check_mode() more generic by taking just net pointers
> and modes, instead of a vsock_sock ptr, for reuse by transports
> (e.g., vhost_vsock)
>
>Changes in v6:
>- add orig_net_mode to store mode at creation time which will be used to
> avoid breakage when namespace changes mode during socket/VM lifespan
>
>Changes in v5:
>- use /proc/sys/net/vsock/ns_mode instead of /proc/net/vsock_ns_mode
>- change from net->vsock.ns_mode to net->vsock.mode
>- change vsock_net_set_mode() to vsock_net_write_mode()
>- vsock_net_write_mode() returns bool for write success to avoid
> need to use vsock_net_mode_can_set()
>- remove vsock_net_mode_can_set()
>---
> MAINTAINERS | 1 +
> include/net/af_vsock.h | 56 +++++++++++++++++++++++++++++++++++++++++++++
> include/net/net_namespace.h | 4 ++++
> include/net/netns/vsock.h | 20 ++++++++++++++++
> 4 files changed, 81 insertions(+)
>
>diff --git a/MAINTAINERS b/MAINTAINERS
>index ea72b3bd2248..dd765bbf79ab 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -27070,6 +27070,7 @@ L: netdev@vger.kernel.org
> S: Maintained
> F: drivers/vhost/vsock.c
> F: include/linux/virtio_vsock.h
>+F: include/net/netns/vsock.h
> F: include/uapi/linux/virtio_vsock.h
> F: net/vmw_vsock/virtio_transport.c
> F: net/vmw_vsock/virtio_transport_common.c
>diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>index d40e978126e3..bce5389ef742 100644
>--- a/include/net/af_vsock.h
>+++ b/include/net/af_vsock.h
>@@ -10,6 +10,7 @@
>
> #include <linux/kernel.h>
> #include <linux/workqueue.h>
>+#include <net/netns/vsock.h>
> #include <net/sock.h>
> #include <uapi/linux/vm_sockets.h>
>
>@@ -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;
}
Thanks,
Stefano
>+ spin_unlock_bh(&net->vsock.lock);
>+ return ret;
>+}
>+
>+static inline bool vsock_net_write_mode(struct net *net, u8 mode)
>+{
>+ bool ret;
>+
>+ spin_lock_bh(&net->vsock.lock);
>+
>+ if (net->vsock.mode_locked) {
>+ ret = false;
>+ goto skip;
>+ }
>+
>+ net->vsock.mode = mode;
>+ net->vsock.mode_locked = true;
>+ ret = true;
>+
>+skip:
>+ spin_unlock_bh(&net->vsock.lock);
>+ return ret;
>+}
>+
>+/* Return true if two namespaces and modes pass the mode rules. Otherwise,
>+ * return false.
>+ *
>+ * ns0 and ns1 are the namespaces being checked.
>+ * mode0 and mode1 are the vsock namespace modes of ns0 and ns1.
>+ *
>+ * Read more about modes in the comment header of net/vmw_vsock/af_vsock.c.
>+ */
>+static inline bool vsock_net_check_mode(struct net *ns0, enum vsock_net_mode mode0,
>+ struct net *ns1, enum vsock_net_mode mode1)
>+{
>+ /* Any vsocks within the same network namespace are always reachable,
>+ * regardless of the mode.
>+ */
>+ if (net_eq(ns0, ns1))
>+ return true;
>+
>+ /*
>+ * If the network namespaces differ, vsocks are only reachable if both
>+ * were created in VSOCK_NET_MODE_GLOBAL mode.
>+ */
>+ return mode0 == VSOCK_NET_MODE_GLOBAL && mode0 == mode1;
>+}
> #endif /* __AF_VSOCK_H__ */
>diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
>index cb664f6e3558..66d3de1d935f 100644
>--- a/include/net/net_namespace.h
>+++ b/include/net/net_namespace.h
>@@ -37,6 +37,7 @@
> #include <net/netns/smc.h>
> #include <net/netns/bpf.h>
> #include <net/netns/mctp.h>
>+#include <net/netns/vsock.h>
> #include <net/net_trackers.h>
> #include <linux/ns_common.h>
> #include <linux/idr.h>
>@@ -196,6 +197,9 @@ struct net {
> /* Move to a better place when the config guard is removed. */
> struct mutex rtnl_mutex;
> #endif
>+#if IS_ENABLED(CONFIG_VSOCKETS)
>+ struct netns_vsock vsock;
>+#endif
> } __randomize_layout;
>
> #include <linux/seq_file_net.h>
>diff --git a/include/net/netns/vsock.h b/include/net/netns/vsock.h
>new file mode 100644
>index 000000000000..c9a438ad52f2
>--- /dev/null
>+++ b/include/net/netns/vsock.h
>@@ -0,0 +1,20 @@
>+/* SPDX-License-Identifier: GPL-2.0 */
>+#ifndef __NET_NET_NAMESPACE_VSOCK_H
>+#define __NET_NET_NAMESPACE_VSOCK_H
>+
>+#include <linux/types.h>
>+
>+enum vsock_net_mode {
>+ VSOCK_NET_MODE_GLOBAL,
>+ VSOCK_NET_MODE_LOCAL,
>+};
>+
>+struct netns_vsock {
>+ struct ctl_table_header *sysctl_hdr;
>+ spinlock_t lock;
>+
>+ /* protected by lock */
>+ enum vsock_net_mode mode;
>+ bool mode_locked;
>+};
>+#endif /* __NET_NET_NAMESPACE_VSOCK_H */
>
>--
>2.47.3
>
^ permalink raw reply
* Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute
From: ally heev @ 2025-11-06 16:05 UTC (permalink / raw)
To: Alexander Lobakin
Cc: Tony Nguyen, Przemek Kitszel, 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: <575bfdb1-8fc4-4147-8af7-33c40e619b66@intel.com>
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`.
>
> 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.
Here though, the definitions are still at the top right? Maybe I could
just sort them
>
> > +
> > if (!params)
> > return -ENOMEM;
> >
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> > index cbb5fa30f5a0ec778c1ee30470da3ca21cc1af24..368138715cd55cd1dadc686931cdda51c7a5130d 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> > @@ -1012,7 +1012,6 @@ static int idpf_send_get_caps_msg(struct idpf_adapter *adapter)
> > */
> > static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
> > {
> > - struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree);
> > struct idpf_vc_xn_params xn_params = {
> > .vc_op = VIRTCHNL2_OP_GET_LAN_MEMORY_REGIONS,
> > .recv_buf.iov_len = IDPF_CTLQ_MAX_BUF_LEN,
> > @@ -1023,7 +1022,9 @@ static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
> > ssize_t reply_sz;
> > int err = 0;
> >
> > - rcvd_regions = kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
> > + struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree) =
> > + kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
> > +
> > if (!rcvd_regions)
> > return -ENOMEM;
>
> Same here, @rcvd_regions is initialized before the very first return, no
> idea how one can provoke an UB here.
>
> >
> >
> > ---
> > base-commit: c9cfc122f03711a5124b4aafab3211cf4d35a2ac
> > change-id: 20251105-aheev-uninitialized-free-attr-net-ethernet-7d106e4ab3f7
> >
> > Best regards,
>
> Thanks,
> Olek
Regards,
Ally
^ permalink raw reply
* RE: [PATCH v10 2/2] Drivers: hv: Introduce mshv_vtl driver
From: Michael Kelley @ 2025-11-06 14:54 UTC (permalink / raw)
To: Naman Jain, K . Y . Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, H . Peter Anvin
Cc: linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
x86@kernel.org, Peter Zijlstra, Sean Christopherson,
Paolo Bonzini, Mukesh Rathor, Stanislav Kinsburskii,
Nuno Das Neves, Christoph Hellwig, Saurabh Sengar, ALOK TIWARI
In-Reply-To: <20251029050139.46545-3-namjain@linux.microsoft.com>
From: Naman Jain <namjain@linux.microsoft.com> Sent: Tuesday, October 28, 2025 10:02 PM
>
> Provide an interface for Virtual Machine Monitor like OpenVMM and its
> use as OpenHCL paravisor to control VTL0 (Virtual trust Level).
> Expose devices and support IOCTLs for features like VTL creation,
> VTL0 memory management, context switch, making hypercalls,
> mapping VTL0 address space to VTL2 userspace, getting new VMBus
> messages and channel events in VTL2 etc.
>
> Co-developed-by: Roman Kisel <romank@linux.microsoft.com>
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> Co-developed-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
> ---
> arch/x86/hyperv/Makefile | 10 +-
> arch/x86/hyperv/hv_vtl.c | 43 +
> arch/x86/hyperv/mshv-asm-offsets.c | 37 +
> arch/x86/hyperv/mshv_vtl_asm.S | 98 ++
> arch/x86/include/asm/mshyperv.h | 34 +
> drivers/hv/Kconfig | 26 +-
> drivers/hv/Makefile | 7 +-
> drivers/hv/mshv_vtl.h | 25 +
> drivers/hv/mshv_vtl_main.c | 1392 ++++++++++++++++++++++++++++
> include/hyperv/hvgdk_mini.h | 106 +++
> include/uapi/linux/mshv.h | 80 ++
> 11 files changed, 1855 insertions(+), 3 deletions(-)
> create mode 100644 arch/x86/hyperv/mshv-asm-offsets.c
> create mode 100644 arch/x86/hyperv/mshv_vtl_asm.S
> create mode 100644 drivers/hv/mshv_vtl.h
> create mode 100644 drivers/hv/mshv_vtl_main.c
>
I've reviewed and made suggestions on most of this code pretty
carefully over the past few months and through 10 revisions. This
version addresses my suggestions and looks good to me. There
are a few areas, such as the assembly code in mshv_vtl_asm.S and
the details of the hypervisor ABI for doing VTL Return, that are
outside my area of expertise so I'm limited to a surface level
review.
Reviewed-by: Michael Kelley <mhklinux@outlook.com>
^ permalink raw reply
* Re: [PATCH v10 1/2] Drivers: hv: Export some symbols for mshv_vtl
From: Peter Zijlstra @ 2025-11-06 14:49 UTC (permalink / raw)
To: Naman Jain
Cc: K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H . Peter Anvin, linux-hyperv, linux-kernel, x86,
Sean Christopherson, Paolo Bonzini, Michael Kelley, Mukesh Rathor,
Stanislav Kinsburskii, Nuno Das Neves, Christoph Hellwig,
Saurabh Sengar, ALOK TIWARI
In-Reply-To: <20251029050139.46545-2-namjain@linux.microsoft.com>
On Wed, Oct 29, 2025 at 05:01:38AM +0000, Naman Jain wrote:
> MSHV_VTL driver is going to be introduced, which is supposed to
> provide interface for Virtual Machine Monitors (VMMs) to control
> Virtual Trust Level (VTL). Export the symbols needed
> to make it work (vmbus_isr, hv_context and hv_post_message).
Please consider using EXPORT_SYMBOL_FOR_MODULES()
> +EXPORT_SYMBOL_GPL(hv_context);
> +EXPORT_SYMBOL_GPL(hv_post_message);
> +EXPORT_SYMBOL_GPL(vmbus_isr);
^ permalink raw reply
* RE: [PATCH v10 1/2] Drivers: hv: Export some symbols for mshv_vtl
From: Michael Kelley @ 2025-11-06 14:45 UTC (permalink / raw)
To: Naman Jain, K . Y . Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, H . Peter Anvin
Cc: linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
x86@kernel.org, Peter Zijlstra, Sean Christopherson,
Paolo Bonzini, Mukesh Rathor, Stanislav Kinsburskii,
Nuno Das Neves, Christoph Hellwig, Saurabh Sengar, ALOK TIWARI
In-Reply-To: <20251029050139.46545-2-namjain@linux.microsoft.com>
From: Naman Jain <namjain@linux.microsoft.com> Sent: Tuesday, October 28, 2025 10:02 PM
>
> MSHV_VTL driver is going to be introduced, which is supposed to
> provide interface for Virtual Machine Monitors (VMMs) to control
> Virtual Trust Level (VTL). Export the symbols needed
> to make it work (vmbus_isr, hv_context and hv_post_message).
>
> Co-developed-by: Roman Kisel <romank@linux.microsoft.com>
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> Co-developed-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202506110544.q0NDMQVc-lkp@intel.com/
> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
Reviewed-by: Michael Kelley <mhklinux@outlook.com>
> ---
> drivers/hv/hv.c | 3 +++
> drivers/hv/hyperv_vmbus.h | 1 +
> drivers/hv/vmbus_drv.c | 4 +++-
> 3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index 936c5f310df6..c924e3294b8b 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -18,6 +18,7 @@
> #include <linux/clockchips.h>
> #include <linux/delay.h>
> #include <linux/interrupt.h>
> +#include <linux/export.h>
> #include <clocksource/hyperv_timer.h>
> #include <asm/mshyperv.h>
> #include <linux/set_memory.h>
> @@ -25,6 +26,7 @@
>
> /* The one and only */
> struct hv_context hv_context;
> +EXPORT_SYMBOL_GPL(hv_context);
>
> /*
> * hv_init - Main initialization routine.
> @@ -104,6 +106,7 @@ int hv_post_message(union hv_connection_id connection_id,
>
> return hv_result(status);
> }
> +EXPORT_SYMBOL_GPL(hv_post_message);
>
> static int hv_alloc_page(void **page, bool decrypt, const char *note)
> {
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index f7fc2630c054..b2862e0a317a 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -33,6 +33,7 @@
> */
> #define HV_UTIL_NEGO_TIMEOUT 55
>
> +void vmbus_isr(void);
>
> /* Definitions for the monitored notification facility */
> union hv_monitor_trigger_group {
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 0dc4692b411a..c9ad17ed562d 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -36,6 +36,7 @@
> #include <linux/syscore_ops.h>
> #include <linux/dma-map-ops.h>
> #include <linux/pci.h>
> +#include <linux/export.h>
> #include <clocksource/hyperv_timer.h>
> #include <asm/mshyperv.h>
> #include "hyperv_vmbus.h"
> @@ -1349,7 +1350,7 @@ static void vmbus_message_sched(struct
> hv_per_cpu_context *hv_cpu, void *message
> }
> }
>
> -static void vmbus_isr(void)
> +void vmbus_isr(void)
> {
> struct hv_per_cpu_context *hv_cpu
> = this_cpu_ptr(hv_context.cpu_context);
> @@ -1362,6 +1363,7 @@ static void vmbus_isr(void)
>
> add_interrupt_randomness(vmbus_interrupt);
> }
> +EXPORT_SYMBOL_GPL(vmbus_isr);
>
> static irqreturn_t vmbus_percpu_isr(int irq, void *dev_id)
> {
> --
> 2.43.0
^ permalink raw reply
* Re: [Intel-wired-lan] [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute
From: Alexander Lobakin @ 2025-11-06 14:07 UTC (permalink / raw)
To: Ally Heev
Cc: Tony Nguyen, Przemek Kitszel, 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: <20251106-aheev-uninitialized-free-attr-net-ethernet-v3-1-ef2220f4f476@gmail.com>
From: Ally Heev <allyheev@gmail.com>
Date: Thu, 06 Nov 2025 17:25:48 +0530
> Uninitialized pointers with `__free` attribute can cause undefined
> behavior as the memory assigned randomly to the pointer is freed
> automatically when the pointer goes out of scope.
>
> It is better to initialize and assign pointers with `__free`
> attribute in one statement to ensure proper scope-based cleanup.
>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/aPiG_F5EBQUjZqsl@stanley.mountain/
> Signed-off-by: Ally Heev <allyheev@gmail.com>
> ---
> Changes in v3:
> - fixed style issues
> - Link to v2: https://lore.kernel.org/r/20251106-aheev-uninitialized-free-attr-net-ethernet-v2-1-048da0c5d6b6@gmail.com
>
> Changes in v2:
> - fixed non-pointer initialization to NULL
> - NOTE: drop v1
> - Link to v1: https://lore.kernel.org/r/20251105-aheev-uninitialized-free-attr-net-ethernet-v1-1-f6ea84bbd750@gmail.com
> ---
> drivers/net/ethernet/intel/ice/ice_flow.c | 5 +++--
> drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 5 +++--
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> 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`.
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?
> +
> if (!params)
> return -ENOMEM;
>
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> index cbb5fa30f5a0ec778c1ee30470da3ca21cc1af24..368138715cd55cd1dadc686931cdda51c7a5130d 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> @@ -1012,7 +1012,6 @@ static int idpf_send_get_caps_msg(struct idpf_adapter *adapter)
> */
> static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
> {
> - struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree);
> struct idpf_vc_xn_params xn_params = {
> .vc_op = VIRTCHNL2_OP_GET_LAN_MEMORY_REGIONS,
> .recv_buf.iov_len = IDPF_CTLQ_MAX_BUF_LEN,
> @@ -1023,7 +1022,9 @@ static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
> ssize_t reply_sz;
> int err = 0;
>
> - rcvd_regions = kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
> + struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree) =
> + kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
> +
> if (!rcvd_regions)
> return -ENOMEM;
Same here, @rcvd_regions is initialized before the very first return, no
idea how one can provoke an UB here.
>
>
> ---
> base-commit: c9cfc122f03711a5124b4aafab3211cf4d35a2ac
> change-id: 20251105-aheev-uninitialized-free-attr-net-ethernet-7d106e4ab3f7
>
> Best regards,
Thanks,
Olek
^ permalink raw reply
* RE: [PATCH] mshv: Allow mappings that overlap in uaddr
From: Michael Kelley @ 2025-11-06 13:38 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: <1762294728-21721-1-git-send-email-nunodasneves@linux.microsoft.com>
From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Tuesday, November 4, 2025 2:19 PM
>
> 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 up until 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 | 19 +------------------
> include/uapi/linux/mshv.h | 2 +-
> 2 files changed, 2 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index 814465a0912d..e5da5f2ab6f7 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
> @@ -1235,9 +1220,7 @@ static int mshv_partition_create_region(struct mshv_partition *partition,
>
> /* 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))
> + mshv_partition_region_by_gfn(partition, mem->guest_pfn + nr_pages - 1))
> return -EEXIST;
This existing code (and after this patch) checks for overlap by seeing if the
requested starting and ending GFNs are already in some existing region. But
is this really sufficient to detect overlap? Consider this example:
1. Three regions exist covering these GFNs respectively: 100 thru 199,
300 thru 399, and 500 thru 599.
2. A request is made to create a new region for GFNs 250 thru 449.
This new request would pass the check, but would still overlap. Or is there
something that prevents this scenario?
>
> region = vzalloc(sizeof(*region) + sizeof(struct page *) * nr_pages);
> 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
I've given my Reviewed-by: narrowly for this patch, since it appears to be
correct for what it does. But if the approach for detecting overlap really
is faulty, an additional patch is needed that might supersede this one.
Reviewed-by: Michael Kelley <mhklinux@outlook.com>
^ permalink raw reply
* Re: [PATCH net-next v2] net: mana: Handle SKB if TX SGEs exceed hardware limit
From: Eric Dumazet @ 2025-11-06 13:17 UTC (permalink / raw)
To: Aditya Garg
Cc: Jakub Kicinski, kys, haiyangz, wei.liu, decui, andrew+netdev,
davem, pabeni, longli, kotaranov, horms, shradhagupta, ssengar,
ernis, dipayanroy, shirazsaleem, linux-hyperv, netdev,
linux-kernel, linux-rdma, gargaditya
In-Reply-To: <bb692420-25f9-4d6e-a68b-dd83c8f4be10@linux.microsoft.com>
On Thu, Nov 6, 2025 at 5:01 AM Aditya Garg
<gargaditya@linux.microsoft.com> wrote:
>
> On 06-11-2025 05:47, Jakub Kicinski wrote:
> > On Wed, 5 Nov 2025 22:10:23 +0530 Aditya Garg wrote:
> >>>> if (err) {
> >>>> (void)skb_dequeue_tail(&txq->pending_skbs);
> >>>> + mana_unmap_skb(skb, apc);
> >>>> netdev_warn(ndev, "Failed to post TX OOB: %d\n", err);
> >>>
> >>> You have a print right here and in the callee. This condition must
> >>> (almost) never happen in practice. It's likely fine to just drop
> >>> the packet.
> >>
> >> The logs placed in callee doesn't covers all the failure scenarios,
> >> hence I feel to have this log here with proper status. Maybe I can
> >> remove the log in the callee?
> >
> > I think my point was that since there are logs (per packet!) when the
> > condition is hit -- if it did in fact hit with any noticeable frequency
> > your users would have complained. So handling the condition gracefully
> > and returning BUSY is likely just unnecessary complexity in practice.
> >
>
> In this, we are returning tx_busy when the error reason is -ENOSPC, for
> all other errors, skb is dropped.
> Is it okay requeue only for -ENOSPC cases or should we drop the skb?
I would avoid NETDEV_TX_BUSY like the plague.
Most drivers get it wrong (including mana)
Documentation/networking/driver.rst
Please drop the packet.
>
> > The logs themselves I don't care all that much about. Sure, having two
> > lines for one error is a bit unclean.
> >
> >>> Either way -- this should be a separate patch.
> >>>
> >> Are you suggesting a separate patch altogether or two patch in the same
> >> series?
> >
> > The changes feel related enough to make them a series, but either way
> > is fine.
>
> Regards,
> Aditya
>
^ permalink raw reply
* Re: [PATCH net-next v2] net: mana: Handle SKB if TX SGEs exceed hardware limit
From: Aditya Garg @ 2025-11-06 13:00 UTC (permalink / raw)
To: Jakub Kicinski
Cc: kys, haiyangz, wei.liu, decui, andrew+netdev, davem, edumazet,
pabeni, longli, kotaranov, horms, shradhagupta, ssengar, ernis,
dipayanroy, shirazsaleem, linux-hyperv, netdev, linux-kernel,
linux-rdma, gargaditya
In-Reply-To: <20251105161754.4b9a1363@kernel.org>
On 06-11-2025 05:47, Jakub Kicinski wrote:
> On Wed, 5 Nov 2025 22:10:23 +0530 Aditya Garg wrote:
>>>> if (err) {
>>>> (void)skb_dequeue_tail(&txq->pending_skbs);
>>>> + mana_unmap_skb(skb, apc);
>>>> netdev_warn(ndev, "Failed to post TX OOB: %d\n", err);
>>>
>>> You have a print right here and in the callee. This condition must
>>> (almost) never happen in practice. It's likely fine to just drop
>>> the packet.
>>
>> The logs placed in callee doesn't covers all the failure scenarios,
>> hence I feel to have this log here with proper status. Maybe I can
>> remove the log in the callee?
>
> I think my point was that since there are logs (per packet!) when the
> condition is hit -- if it did in fact hit with any noticeable frequency
> your users would have complained. So handling the condition gracefully
> and returning BUSY is likely just unnecessary complexity in practice.
>
In this, we are returning tx_busy when the error reason is -ENOSPC, for
all other errors, skb is dropped.
Is it okay requeue only for -ENOSPC cases or should we drop the skb?
> The logs themselves I don't care all that much about. Sure, having two
> lines for one error is a bit unclean.
>
>>> Either way -- this should be a separate patch.
>>>
>> Are you suggesting a separate patch altogether or two patch in the same
>> series?
>
> The changes feel related enough to make them a series, but either way
> is fine.
Regards,
Aditya
^ permalink raw reply
* [PATCH v3] net: ethernet: fix uninitialized pointers with free attribute
From: Ally Heev @ 2025-11-06 11:55 UTC (permalink / raw)
To: Tony Nguyen, Przemek Kitszel, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui
Cc: Aleksandr Loktionov, intel-wired-lan, netdev, linux-kernel,
linux-hyperv, Dan Carpenter, Ally Heev
Uninitialized pointers with `__free` attribute can cause undefined
behavior as the memory assigned randomly to the pointer is freed
automatically when the pointer goes out of scope.
It is better to initialize and assign pointers with `__free`
attribute in one statement to ensure proper scope-based cleanup.
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/aPiG_F5EBQUjZqsl@stanley.mountain/
Signed-off-by: Ally Heev <allyheev@gmail.com>
---
Changes in v3:
- fixed style issues
- Link to v2: https://lore.kernel.org/r/20251106-aheev-uninitialized-free-attr-net-ethernet-v2-1-048da0c5d6b6@gmail.com
Changes in v2:
- fixed non-pointer initialization to NULL
- NOTE: drop v1
- Link to v1: https://lore.kernel.org/r/20251105-aheev-uninitialized-free-attr-net-ethernet-v1-1-f6ea84bbd750@gmail.com
---
drivers/net/ethernet/intel/ice/ice_flow.c | 5 +++--
drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 5 +++--
2 files changed, 6 insertions(+), 4 deletions(-)
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);
+
if (!params)
return -ENOMEM;
diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index cbb5fa30f5a0ec778c1ee30470da3ca21cc1af24..368138715cd55cd1dadc686931cdda51c7a5130d 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
@@ -1012,7 +1012,6 @@ static int idpf_send_get_caps_msg(struct idpf_adapter *adapter)
*/
static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
{
- struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree);
struct idpf_vc_xn_params xn_params = {
.vc_op = VIRTCHNL2_OP_GET_LAN_MEMORY_REGIONS,
.recv_buf.iov_len = IDPF_CTLQ_MAX_BUF_LEN,
@@ -1023,7 +1022,9 @@ static int idpf_send_get_lan_memory_regions(struct idpf_adapter *adapter)
ssize_t reply_sz;
int err = 0;
- rcvd_regions = kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
+ struct virtchnl2_get_lan_memory_regions *rcvd_regions __free(kfree) =
+ kzalloc(IDPF_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
+
if (!rcvd_regions)
return -ENOMEM;
---
base-commit: c9cfc122f03711a5124b4aafab3211cf4d35a2ac
change-id: 20251105-aheev-uninitialized-free-attr-net-ethernet-7d106e4ab3f7
Best regards,
--
Ally Heev <allyheev@gmail.com>
^ 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