RHEL-149879: [viostor] Limit num_cpus and max_cpus to MAX_CPU

[benyamin-codez]

Limits the maxmimum CPU count to the global macro constant MAX_CPU.

The bot has identified potential out-of-bounds memory access on systems with > 256 CPUs as ADAPTER_EXTENSION members vq and processing_srbs are limited to MAX_CPU (which is 256) but the num_queues is not.

This was previously managed by casting num_cpus to num_queues as USHORT. PR #1502 removes this cast and reimplements num_queues as ULONG.

This PR proposes an alternate solution to limit the num_queues member by limiting the num_cpus and max_cpus variables to MAX_CPU. This seems the appropriate place to limit CPU counts.

[gemini-code-assist]

Summary of Changes

Hello @benyamin-codez, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the robustness of the viostor driver by implementing a safeguard for CPU count variables. It ensures that the reported active and maximum CPU counts do not exceed the MAX_CPU constant, thereby preventing potential memory corruption issues that could arise on systems with a very high number of processors due to fixed-size data structures. This is a more appropriate place to limit CPU counts according to the author.

Highlights

• CPU Count Limitation: The num_cpus and max_cpus variables are now explicitly limited to the global macro constant MAX_CPU using the min() function during adapter initialization.
• Memory Safety Enhancement: This change prevents potential out-of-bounds memory access on systems with more than 256 CPUs, as ADAPTER_EXTENSION members like vq and processing_srbs are sized according to MAX_CPU (which is 256).
• Alternative Solution: This pull request provides an alternative approach to address the CPU count issue, contrasting with a previous solution in PR [viostor] Use ULONG for num_queues #1502 that modified the num_queues type.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog

• viostor/virtio_stor.c
  • Modified the assignment of num_cpus and max_cpus to include a min() operation with MAX_CPU, ensuring these values do not exceed the defined maximum.

Activity

• The pull request was opened by benyamin-codez to address a potential memory safety issue related to CPU counts.
• It proposes an alternative solution to a problem previously addressed in PR [viostor] Use ULONG for num_queues #1502.
• No specific review comments or discussions have been recorded yet.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[YanVugenfirer]

[Jenkins CI]: Can one of the admins verify this patch?

[benyamin-codez]

@YanVugenfirer

Yan,

Please do let me know if you would prefer I add this one to PR #1502.
It made some sense to me to keep it separate.

Best regards,
Ben

[gemini-code-assist]

Code Review

This pull request correctly addresses a potential out-of-bounds memory access by limiting the num_cpus and max_cpus variables to the MAX_CPU constant. The approach is sound and effectively prevents the issue on systems with more than 256 CPUs. I have one suggestion to improve the implementation to avoid potential side effects related to macro expansion, which can lead to multiple function calls.

[YanVugenfirer]

Hi @benyamin-codez,

I think that assumption of having only 256 CPUs is outdated. https://learn.microsoft.com/en-us/windows-server/get-started/locks-limits?tabs=full-comparison&pivots=windows-server-2025

Your PR with using ULONG for the amount of queue is a good direction. I don't think we need to limit the driver.
Let's check where MAX_CPU used for static arrays and fix the code.

I think that the full solution is:

1. Check in the code where we use MAX_CPU
2. Double check other data structures like DPC array
3. Implement dynamic allocation of everything that was used based on MAX_CPU
4. Implement registry setting to limit amount of queues (to let the users to have a control of max amount of used queues or vCPUs due to different concerns they might have).
5. Repeat the above for virtio-scsi

[benyamin-codez]

@YanVugenfirer

Hi Yan,

iirc, the 256 limit was the previous SeaBIOS default limit due to the xAPIC 256-bit ID bitmap. This then bumped to 288 vCPUs (in 2016 iinm) when using the q35 machine type with IOMMU and EIM enabled (QEMU v2.8). We now have 1024 vCPUs with QEMU version 8.2+, but YMMV as this is configuration dependent.

The legacy vCPU hot-plug/unplug was recently cleaned up (in December last year) and new QEMU versions will now just use the modern interface. The legacy code applied to machine types prior to 2.7 and was limited by:

#define ACPI_CPU_HOTPLUG_ID_LIMIT 256

So if we intend to continue supporting machine versions prior to 2.7, then this will remain a limit with older versions of QEMU that might still be in production. Surely, this would have to be an extreme corner case though...?!?

AFAICT, the reality is that we have no effective means to glean a vCPU limit from QEMU from inside the driver anyway. Given this, and the above-mentioned limit variability, I agreee that it does make sense to remove the limit from the driver. We should be able to populate num_cpus and max_cpus based on the -smp QEMU parameters and then just trust this to be within supported bounds.

Dynamic allocation (and cleanup) would be required for the nested processing_srbs[] REQUEST_LIST struct and the nested virtqueue struct (*vq[]). We will need to use the pointer method as we have more than one member in the ADAPTER_EXTENSION struct (the struct hack method only accommodates a single member as the last member), and it is typically the preferred method anyway, especially to retain alignment. AFAICT, the other data structures are already dynamically allocated, e.g. the DPC array is a PSTOR_DPC object managed by Storport.

Regarding registry manipulation, I did this previously in a vioscsi WIP via a bool registry switch to enable the extra queues, and it worked to the extent the queues were allocated. In those experiments I just blindly created MAX_CPU queues and left it for the user to hot-plug vCPUs as they saw fit. Here we can limit to max_cpus detected instead, negating the need for a bool registry switch.

A foreseeable problem is that we will have the wrong KAFFINITY mask for the perfData.ConcurrentChannels, so we might need to do some work on that. An adapter restart would fix this but we might be able to introduce a poll or there might be a Storport PnP event anyway for vCPU hot-plug/unplug. It's worth mentioning that the sys admin has to configure the machine to use the -smp parameters to populate max_cpus, otherwise max_cpus = num_cpus... So some experimentation here will be required.

In any case, I'll need to sort out the registry PR first (#1297 - I am working on that one at the moment) and include a global value only parameter, which is easy enough. I propose we initially implement a limit on max_queues. Later we might want to implement KAFFINITY masking on a per-HBA basis by manipulation of the StorPortInitializePerfOpts() parameters, namely perfData.FirstRedirectionMessageNumber and perfData.ConcurrentChannels.

Repeating for vioscsi should be fairly straightforward.
Some definitions are incorrect but they will be replaced by the dynamic allocation anyway. For example:

// this one:
#define VIRTIO_SCSI_QUEUE_LAST   VIRTIO_SCSI_REQUEST_QUEUE_0 + MAX_CPU
// should be:
#define VIRTIO_SCSI_QUEUE_LAST   MAX_CPU - VIRTIO_SCSI_REQUEST_QUEUE_0

Anyway, do you see any issues with the above approach?

Perhaps more importantly, did you want to merge this PR in the interim to cover the out-of-bounds memory access scenarios, i.e. when num_cpus > MAX_CPU, PR #1502 now exposes since it merged?

Best regards,
Ben

[YanVugenfirer]

@benyamin-codez I don't see big problem with legacy deployments, as in any case the amount of quest must be specified by the host.

Regarding "-smp" issue or a better term will be "CPU Hot plug scenario" - sure, it should be tested.

[benyamin-codez]

Perhaps more importantly, did you want to merge this PR in the interim to cover the out-of-bounds memory access scenarios, i.e. when num_cpus > MAX_CPU, PR #1502 now exposes since it merged?

@YanVugenfirer - I just wanted to make sure this question didn't get missed.

The previous behaviour (prior to PR #1502 merging) would limit the num_queues to num_cpus.

If you want to keep that behaviour for now - as the alternate solution will take some time to prove - I will switch this PR to ready for review with a view to merge it ASAP. I highly recommend this approach to avoid breakage in the wild.

Otherwise I will close this PR early and raise a new PR in due course with the proposed unlimited solution.

[YanVugenfirer]

@benyamin-codez Let's keep the existing behavior.

[benyamin-codez]

@YanVugenfirer

I'm somewhat concerned that a new release without this PR merged will break systems with high vCPU counts.

Can we please run some HCK-CI tests and keep this one moving?

[benyamin-codez]

A further note wrt to the alternate (long-term) solution:

We will still be limited by the MSI-X MessageNumberLimit defined in the INF file.
The limit is presently 2048, per the doco.

As QEMU appears to be presently limited to 1024 vCPUs, ...
...perhaps we should set this to 1025 (1024 queue vectors + one config vector).

The alternative is just to set it to the maximum of 2,048.

I recommend retaining the registry entry as it is useful for testing.

Thoughts?

cc: @YanVugenfirer @kevmw @vrozenfe

[YanVugenfirer]

@benyamin-codez
The amount of queue still can (and is) be limited by the device. So we will still can limit the amount of queues.

In order not to go back to this code in couple of years, I suggest dynamic allocation of all the data structures related to the amount of queues.

[benyamin-codez]

The amount of queue still can (and is) be limited by the device. So we will still can limit the amount of queues.

This is true, although iinm, the QEMU num-queues option is optional and defaults to the number of vCPUs.
This means systems configured to use more than > 256 vCPUs will likely break without manual intervention.

In order not to go back to this code in couple of years, I suggest dynamic allocation of all the data structures related to the amount of queues.

I can probably come up with a first cut by the end of the quarter...