User space provides the vector as an unsigned int that is checked
early for validity (vfio_set_irqs_validate_and_prepare()).
A later negative check of the provided vector is not necessary.
Remove the negative check and ensure the type used
for the vector is consistent as an unsigned int.
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
drivers/vfio/pci/vfio_pci_intrs.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 6a9c6a143cc3..3f64ccdce69f 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -317,14 +317,14 @@ static int vfio_msi_enable(struct vfio_pci_core_device *vdev, int nvec, bool msi
}
static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
- int vector, int fd, bool msix)
+ unsigned int vector, int fd, bool msix)
{
struct pci_dev *pdev = vdev->pdev;
struct eventfd_ctx *trigger;
int irq, ret;
u16 cmd;
- if (vector < 0 || vector >= vdev->num_ctx)
+ if (vector >= vdev->num_ctx)
return -EINVAL;
irq = pci_irq_vector(pdev, vector);
@@ -399,7 +399,8 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, unsigned start,
unsigned count, int32_t *fds, bool msix)
{
- int i, j, ret = 0;
+ int i, ret = 0;
+ unsigned int j;
if (start >= vdev->num_ctx || start + count > vdev->num_ctx)
return -EINVAL;
@@ -420,7 +421,7 @@ static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, unsigned start,
static void vfio_msi_disable(struct vfio_pci_core_device *vdev, bool msix)
{
struct pci_dev *pdev = vdev->pdev;
- int i;
+ unsigned int i;
u16 cmd;
for (i = 0; i < vdev->num_ctx; i++) {
@@ -542,7 +543,7 @@ static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev,
unsigned index, unsigned start,
unsigned count, uint32_t flags, void *data)
{
- int i;
+ unsigned int i;
bool msix = (index == VFIO_PCI_MSIX_IRQ_INDEX) ? true : false;
if (irq_is(vdev, index) && !count && (flags & VFIO_IRQ_SET_DATA_NONE)) {
--
2.34.1On Tue, 28 Mar 2023 14:53:29 -0700
Reinette Chatre <reinette.chatre@intel.com> wrote:
> User space provides the vector as an unsigned int that is checked
> early for validity (vfio_set_irqs_validate_and_prepare()).
>
> A later negative check of the provided vector is not necessary.
>
> Remove the negative check and ensure the type used
> for the vector is consistent as an unsigned int.
>
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
> drivers/vfio/pci/vfio_pci_intrs.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 6a9c6a143cc3..3f64ccdce69f 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -317,14 +317,14 @@ static int vfio_msi_enable(struct vfio_pci_core_device *vdev, int nvec, bool msi
> }
>
> static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
> - int vector, int fd, bool msix)
> + unsigned int vector, int fd, bool msix)
> {
> struct pci_dev *pdev = vdev->pdev;
> struct eventfd_ctx *trigger;
> int irq, ret;
> u16 cmd;
>
> - if (vector < 0 || vector >= vdev->num_ctx)
> + if (vector >= vdev->num_ctx)
> return -EINVAL;
>
> irq = pci_irq_vector(pdev, vector);
> @@ -399,7 +399,8 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
> static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, unsigned start,
> unsigned count, int32_t *fds, bool msix)
> {
> - int i, j, ret = 0;
> + int i, ret = 0;
> + unsigned int j;
>
> if (start >= vdev->num_ctx || start + count > vdev->num_ctx)
> return -EINVAL;
Unfortunately this turns the unwind portion of the function into an
infinite loop in the common case when @start is zero:
for (--j; j >= (int)start; j--)
vfio_msi_set_vector_signal(vdev, j, -1, msix);
Thanks,
Alex
> @@ -420,7 +421,7 @@ static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, unsigned start,
> static void vfio_msi_disable(struct vfio_pci_core_device *vdev, bool msix)
> {
> struct pci_dev *pdev = vdev->pdev;
> - int i;
> + unsigned int i;
> u16 cmd;
>
> for (i = 0; i < vdev->num_ctx; i++) {
> @@ -542,7 +543,7 @@ static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev,
> unsigned index, unsigned start,
> unsigned count, uint32_t flags, void *data)
> {
> - int i;
> + unsigned int i;
> bool msix = (index == VFIO_PCI_MSIX_IRQ_INDEX) ? true : false;
>
> if (irq_is(vdev, index) && !count && (flags & VFIO_IRQ_SET_DATA_NONE)) {
Hi Alex,
On 3/30/2023 1:26 PM, Alex Williamson wrote:
> On Tue, 28 Mar 2023 14:53:29 -0700
> Reinette Chatre <reinette.chatre@intel.com> wrote:
...
>> @@ -399,7 +399,8 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
>> static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, unsigned start,
>> unsigned count, int32_t *fds, bool msix)
>> {
>> - int i, j, ret = 0;
>> + int i, ret = 0;
>> + unsigned int j;
>>
>> if (start >= vdev->num_ctx || start + count > vdev->num_ctx)
>> return -EINVAL;
>
> Unfortunately this turns the unwind portion of the function into an
> infinite loop in the common case when @start is zero:
>
> for (--j; j >= (int)start; j--)
> vfio_msi_set_vector_signal(vdev, j, -1, msix);
>
>
Thank you very much for catching this. It is not clear to me how you
would prefer to resolve this. Would you prefer that the vector parameter
in vfio_msi_set_vector_signal() continue to be an int and this patch be
dropped and the "if (vector < 0)" check remains (option A)? Or, alternatively,
I see two other possible solutions where the vector parameter in
vfio_msi_set_vector_signal() becomes an unsigned int and the above snippet
could be one of:
option B:
vfio_msi_set_block()
{
int i, j, ret = 0;
...
for (--j; j >= (int)start; j--)
vfio_msi_set_vector_signal(vdev, (unsigned int)j, -1, msix);
}
option C:
vfio_msi_set_block()
{
int i, ret = 0;
unsigned int j;
...
for (--j; j >= start && j < start + count; j--)
vfio_msi_set_vector_signal(vdev, j, -1, msix);
}
What would you prefer?
Reinette
On Thu, 30 Mar 2023 15:32:20 -0700
Reinette Chatre <reinette.chatre@intel.com> wrote:
> Hi Alex,
>
> On 3/30/2023 1:26 PM, Alex Williamson wrote:
> > On Tue, 28 Mar 2023 14:53:29 -0700
> > Reinette Chatre <reinette.chatre@intel.com> wrote:
> ...
>
> >> @@ -399,7 +399,8 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
> >> static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, unsigned start,
> >> unsigned count, int32_t *fds, bool msix)
> >> {
> >> - int i, j, ret = 0;
> >> + int i, ret = 0;
> >> + unsigned int j;
> >>
> >> if (start >= vdev->num_ctx || start + count > vdev->num_ctx)
> >> return -EINVAL;
> >
> > Unfortunately this turns the unwind portion of the function into an
> > infinite loop in the common case when @start is zero:
> >
> > for (--j; j >= (int)start; j--)
> > vfio_msi_set_vector_signal(vdev, j, -1, msix);
> >
> >
>
> Thank you very much for catching this. It is not clear to me how you
> would prefer to resolve this. Would you prefer that the vector parameter
> in vfio_msi_set_vector_signal() continue to be an int and this patch be
> dropped and the "if (vector < 0)" check remains (option A)? Or, alternatively,
> I see two other possible solutions where the vector parameter in
> vfio_msi_set_vector_signal() becomes an unsigned int and the above snippet
> could be one of:
>
> option B:
> vfio_msi_set_block()
> {
> int i, j, ret = 0;
>
> ...
> for (--j; j >= (int)start; j--)
> vfio_msi_set_vector_signal(vdev, (unsigned int)j, -1, msix);
> }
>
> option C:
> vfio_msi_set_block()
> {
> int i, ret = 0;
> unsigned int j;
>
> ...
> for (--j; j >= start && j < start + count; j--)
> vfio_msi_set_vector_signal(vdev, j, -1, msix);
> }
>
> What would you prefer?
Hmm, C is fine, it avoids casting. I think we could also do:
unsigned int i, j;
int ret = 0;
...
for (i = start; i < j; i++)
vfio_msi_set_vector_signal(vdev, i, -1, msix);
Thanks,
Alex
Hi Alex,
On 3/30/2023 3:54 PM, Alex Williamson wrote:
> On Thu, 30 Mar 2023 15:32:20 -0700
> Reinette Chatre <reinette.chatre@intel.com> wrote:
>> On 3/30/2023 1:26 PM, Alex Williamson wrote:
>>> On Tue, 28 Mar 2023 14:53:29 -0700
>>> Reinette Chatre <reinette.chatre@intel.com> wrote:
>> ...
>>
>>>> @@ -399,7 +399,8 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
>>>> static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, unsigned start,
>>>> unsigned count, int32_t *fds, bool msix)
>>>> {
>>>> - int i, j, ret = 0;
>>>> + int i, ret = 0;
>>>> + unsigned int j;
>>>>
>>>> if (start >= vdev->num_ctx || start + count > vdev->num_ctx)
>>>> return -EINVAL;
>>>
>>> Unfortunately this turns the unwind portion of the function into an
>>> infinite loop in the common case when @start is zero:
>>>
>>> for (--j; j >= (int)start; j--)
>>> vfio_msi_set_vector_signal(vdev, j, -1, msix);
>>>
>>>
>>
>> Thank you very much for catching this. It is not clear to me how you
>> would prefer to resolve this. Would you prefer that the vector parameter
>> in vfio_msi_set_vector_signal() continue to be an int and this patch be
>> dropped and the "if (vector < 0)" check remains (option A)? Or, alternatively,
>> I see two other possible solutions where the vector parameter in
>> vfio_msi_set_vector_signal() becomes an unsigned int and the above snippet
>> could be one of:
>>
>> option B:
>> vfio_msi_set_block()
>> {
>> int i, j, ret = 0;
>>
>> ...
>> for (--j; j >= (int)start; j--)
>> vfio_msi_set_vector_signal(vdev, (unsigned int)j, -1, msix);
>> }
>>
>> option C:
>> vfio_msi_set_block()
>> {
>> int i, ret = 0;
>> unsigned int j;
>>
>> ...
>> for (--j; j >= start && j < start + count; j--)
>> vfio_msi_set_vector_signal(vdev, j, -1, msix);
>> }
>>
>> What would you prefer?
>
>
> Hmm, C is fine, it avoids casting. I think we could also do:
>
> unsigned int i, j;
> int ret = 0;
>
> ...
>
> for (i = start; i < j; i++)
> vfio_msi_set_vector_signal(vdev, i, -1, msix);
>
Much better. Will do. Thank you very much.
Reinette
© 2016 - 2026 Red Hat, Inc.