[vioscsi] Fix potential typecast overflow error casting to num_queues#1513
[vioscsi] Fix potential typecast overflow error casting to num_queues#1513benyamin-codez wants to merge 1 commit intovirtio-win:masterfrom
Conversation
Fixes a potential typecast overflow error casting to 'num_queues' caused when 'num_queues' was updated to a ULONG from USHORT. The objects being cast are the ULONG variable 'num_cpus' in VioScsiFindAdapter(), and the ULONG struct member 'msix_vectors' from ADAPTER_EXTENSION accessed in VioScsiHwInitialize(). Removal of the cast is all that is necessary to avoid any potential typecast overflow error. It is noteworthy that the overflow error could presently only occur if MAX_CPU was ever increased to 65,536, which is very unlikely. Signed-off-by: benyamin-codez <115509179+benyamin-codez@users.noreply.github.com>
Summary of ChangesHello @benyamin-codez, I'm Gemini Code Assist1! 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 addresses a potential typecast overflow issue within the Highlights
🧠 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. Using Gemini Code AssistThe 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
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 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
|
|
[Jenkins CI]: Can one of the admins verify this patch? |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a potential integer overflow by removing unnecessary casts to USHORT when assigning to num_queues, which is now a ULONG. The changes are correct in addressing the overflow. However, I found a logical issue in one of the modified locations. When adjusting the number of queues based on available MSI-X vectors, the logic is flawed and could lead to an incorrect number of queues being configured, potentially causing issues. I've provided a suggestion to fix this logic. Other than that, the change is good.
| if (adaptExt->num_queues > 1 && ((adaptExt->num_queues + 3) > adaptExt->msix_vectors)) | ||
| { | ||
| adaptExt->num_queues = (USHORT)adaptExt->msix_vectors; | ||
| adaptExt->num_queues = adaptExt->msix_vectors; |
There was a problem hiding this comment.
While removing the cast is correct to prevent a potential overflow, the logic for adjusting num_queues seems incorrect. If the condition (adaptExt->num_queues + 3) > adaptExt->msix_vectors is met, it means there are not enough MSI-X vectors for the requested number of queues. The number of queues should be reduced, but the current code sets num_queues to msix_vectors, which could potentially increase num_queues and worsen the vector deficit.
The number of queues should be reduced to a value that satisfies new_num_queues + 3 <= msix_vectors. A possible correction is to set num_queues to the maximum possible value, which would be msix_vectors - 3.
This should also handle the case where msix_vectors is too small to support even one queue with this scheme (i.e., msix_vectors < 4), by falling back to a single queue. The subsequent driver logic would then need to handle that scenario, possibly by using a single shared interrupt.
adaptExt->num_queues = (adaptExt->msix_vectors >= 4) ? (adaptExt->msix_vectors - 3) : 1;There was a problem hiding this comment.
Will address this in a separate PR...
YanVugenfirer
left a comment
YanVugenfirer
left a comment
There was a problem hiding this comment.
The PR looks OK.
But I would like to repeat myself in virtio-scsi as well:
" if (adaptExt->num_queues > 1 && ((adaptExt->num_queues + 3) > adaptExt->msix_vectors))"
I don't think we need to distinguish 1 and several queues scenarios for the calculations when MSIx is present.
Basically (1 queue + control queues < msix_vectors) scenario is also problematic.
So if you have energy for PRs - please direct it to the impactful ones.
|
ok to test |
Thanks Yan. My thoughts were it makes sense to get this properly solved for |
|
Do you know what happened with the The
|
@benyamin-codez |
3 participants
Fixes a potential typecast overflow error casting to
num_queuescaused whennum_queueswas updated to a ULONG from USHORT. The objects being cast are the ULONG variablenum_cpusinVioScsiFindAdapter(), and the ULONG struct membermsix_vectorsfromADAPTER_EXTENSIONaccessed inVioScsiHwInitialize(). Removal of the cast is all that is necessary to avoid any potential typecast overflow error. It is noteworthy that the overflow error could presently only occur ifMAX_CPUwas ever increased to 65,536, which is very unlikely.