Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] The patch inadvertently leaves `check_hkcpu` as `false` for non-performance VMbus channels, unintentionally disabling `HK_TYPE_MANAGED_IRQ` isolation checks and allowing their interrupts to be assigned to isolated CPUs. - [High] The fix actively violates the user's CPU isolation configuration by forcing interrupts onto isolated CPUs instead of simply skipping incompatible NUMA nodes. --
commit 36f8ef6b096e353e5114a6a9d0a4e19a5ab2e941 Author: Waiman Long <[email protected]> Drivers: hv: Avoid infinite retry loop in init_vp_index() This commit addresses an infinite retry loop in init_vp_index() by allowing the housekeeping cpumask to be ignored if the allocated cpumask has already been cleared. It introduces a check_hkcpu flag in struct vmbus_channel to control whether the housekeeping CPU check should be performed. Link: https://sashiko.dev/#/message/20260422030903.E1BFCC2BCB0%40smtp.kernel.org [1] > diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c > index a044fd3b3c4e7..30d369ee4bf4a 100644 > --- a/drivers/hv/channel_mgmt.c > +++ b/drivers/hv/channel_mgmt.c > @@ -774,6 +774,7 @@ static void init_vp_index(struct vmbus_channel *channel) > } > > for (i = 1; i <= ncpu + 1; i++) { > + channel->check_hkcpu = true; [Severity: High] Since struct vmbus_channel is allocated with kzalloc(), check_hkcpu starts as false. In init_vp_index(), non-performance channels return early before this loop: if (!perf_chn || ...) { channel->target_cpu = VMBUS_CONNECT_CPU; return; } Does this unintentionally leave check_hkcpu disabled permanently for non-performance channels? If CPU hotplug (hv_synic_cleanup() -> hv_pick_new_cpu()) or sysfs target_cpu_store() later changes the CPU for a non-performance channel, it appears vmbus_channel_set_cpu() will skip the housekeeping check because check_hkcpu is false. Could this allow non-performance VMbus channels to be assigned to isolated CPUs, bypassing CPU isolation protections? > while (true) { > numa_node = next_numa_node_id++; > if (numa_node == nr_node_ids) { > @@ -788,14 +789,21 @@ static void init_vp_index(struct vmbus_channel *channel) > > retry: > cpumask_xor(available_mask, allocated_mask, > cpumask_of_node(numa_node)); > - cpumask_and(available_mask, available_mask, hk_mask); > + if (channel->check_hkcpu) > + cpumask_and(available_mask, available_mask, hk_mask); > > if (cpumask_empty(available_mask)) { > /* > * We have cycled through all the CPUs in the node; > - * reset the allocated map. > + * reset the allocated map. If the allocated map has > + * already been cleared, we will have to ignore the > + * HK_TYPE_MANAGED_IRQ housekeeping cpumask as its use > + * is on a best effort basis, not a must. > */ > - cpumask_clear(allocated_mask); > + if (!cpumask_empty(allocated_mask)) > + cpumask_clear(allocated_mask); > + else > + channel->check_hkcpu = false; > goto retry; > } [Severity: High] Does this logic actively violate CPU isolation configurations by forcing interrupts onto isolated CPUs? When the allocator selects a NUMA node where all CPUs are isolated (none are in hk_mask), clearing check_hkcpu forces the interrupt onto one of those isolated CPUs. Since cpumask_empty(hk_mask) is checked globally before the loop, there is guaranteed to be at least one housekeeping CPU on some other node in the system. Would it be better to simply skip incompatible nodes entirely instead of permanently overriding the housekeeping mask and forcing high-throughput performance channels onto isolated CPUs? [ ... ] > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c > index 5ebdbe24b5a1e..4cb9da8467f29 100644 > --- a/drivers/hv/vmbus_drv.c > +++ b/drivers/hv/vmbus_drv.c > @@ -1745,7 +1745,8 @@ int vmbus_channel_set_cpu(struct vmbus_channel > *channel, u32 target_cpu) > if (target_cpu >= nr_cpumask_bits) > return -EINVAL; > > - if (!cpumask_test_cpu(target_cpu, > housekeeping_cpumask(HK_TYPE_MANAGED_IRQ))) > + if (channel->check_hkcpu && > + !cpumask_test_cpu(target_cpu, > housekeeping_cpumask(HK_TYPE_MANAGED_IRQ))) > return -EINVAL; > > if (!cpu_online(target_cpu)) -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
