diff mbox series

virtio-vdpa: Fix unchecked call to NULL set_vq_affinity

Message ID	20230504135053.2283816-1-dtatulea@nvidia.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates 216.228.117.160 as permitted sender) receiver=protection.outlook.com; client-ip=216.228.117.160; helo=mail.nvidia.com; pr=C From: Dragos Tatulea <dtatulea@nvidia.com> To: "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>, Xuan Zhuo <xuanzhuo@linux.alibaba.com>, Xie Yongji <xieyongji@bytedance.com> CC: Dragos Tatulea <dtatulea@nvidia.com>, Gal Pressman <gal@nvidia.com>, <virtualization@lists.linux-foundation.org>, <linux-kernel@vger.kernel.org> Subject: [PATCH] virtio-vdpa: Fix unchecked call to NULL set_vq_affinity Date: Thu, 4 May 2023 16:50:52 +0300 Message-ID: <20230504135053.2283816-1-dtatulea@nvidia.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain Precedence: bulk
Series	virtio-vdpa: Fix unchecked call to NULL set_vq_affinity \| virtio-vdpa: Fix unchecked call to NULL set_vq_affinity

Commit Message

Dragos Tatulea May 4, 2023, 1:50 p.m. UTC

  The referenced patch calls set_vq_affinity without checking if the op is
valid. This patch adds the check.

Fixes: 3dad56823b53 ("virtio-vdpa: Support interrupt affinity spreading mechanism")
Reviewed-by: Gal Pressman <gal@nvidia.com>
Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
---
 drivers/virtio/virtio_vdpa.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Feng Liu May 4, 2023, 5:08 p.m. UTC | #1

On 2023-05-04 a.m.9:50, Dragos Tatulea wrote:
> External email: Use caution opening links or attachments
> 
> 
> The referenced patch calls set_vq_affinity without checking if the op is
> valid. This patch adds the check.
> 
> Fixes: 3dad56823b53 ("virtio-vdpa: Support interrupt affinity spreading mechanism")
> Reviewed-by: Gal Pressman <gal@nvidia.com>
> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> ---
>   drivers/virtio/virtio_vdpa.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> index eb6aee8c06b2..989e2d7184ce 100644
> --- a/drivers/virtio/virtio_vdpa.c
> +++ b/drivers/virtio/virtio_vdpa.c
> @@ -385,7 +385,9 @@ static int virtio_vdpa_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
>                          err = PTR_ERR(vqs[i]);
>                          goto err_setup_vq;
>                  }
> -               ops->set_vq_affinity(vdpa, i, &masks[i]);
> +
> +               if (ops->set_vq_affinity)
> +                       ops->set_vq_affinity(vdpa, i, &masks[i]);
if ops->set_vq_affinity is NULL, should give an error code to err, and 
return err

>          }
> 
>          cb.callback = virtio_vdpa_config_cb;
> --
> 2.40.1
>

Dragos Tatulea May 4, 2023, 5:19 p.m. UTC | #2

On Thu, 2023-05-04 at 13:08 -0400, Feng Liu wrote:
> 
> 
> On 2023-05-04 a.m.9:50, Dragos Tatulea wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > The referenced patch calls set_vq_affinity without checking if the op is
> > valid. This patch adds the check.
> > 
> > Fixes: 3dad56823b53 ("virtio-vdpa: Support interrupt affinity spreading
> > mechanism")
> > Reviewed-by: Gal Pressman <gal@nvidia.com>
> > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> > ---
> >   drivers/virtio/virtio_vdpa.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> > index eb6aee8c06b2..989e2d7184ce 100644
> > --- a/drivers/virtio/virtio_vdpa.c
> > +++ b/drivers/virtio/virtio_vdpa.c
> > @@ -385,7 +385,9 @@ static int virtio_vdpa_find_vqs(struct virtio_device
> > *vdev, unsigned int nvqs,
> >                          err = PTR_ERR(vqs[i]);
> >                          goto err_setup_vq;
> >                  }
> > -               ops->set_vq_affinity(vdpa, i, &masks[i]);
> > +
> > +               if (ops->set_vq_affinity)
> > +                       ops->set_vq_affinity(vdpa, i, &masks[i]);
> if ops->set_vq_affinity is NULL, should give an error code to err, and 
> return err
> 
I don't think so: the set_vq_affinity is marked as optional.

> >          }
> > 
> >          cb.callback = virtio_vdpa_config_cb;
> > --
> > 2.40.1
> >

Feng Liu May 4, 2023, 5:46 p.m. UTC | #3

On 2023-05-04 p.m.1:19, Dragos Tatulea wrote:
> On Thu, 2023-05-04 at 13:08 -0400, Feng Liu wrote:
>>
>>
>> On 2023-05-04 a.m.9:50, Dragos Tatulea wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> The referenced patch calls set_vq_affinity without checking if the op is
>>> valid. This patch adds the check.
>>>
>>> Fixes: 3dad56823b53 ("virtio-vdpa: Support interrupt affinity spreading
>>> mechanism")
>>> Reviewed-by: Gal Pressman <gal@nvidia.com>
>>> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
>>> ---
>>>    drivers/virtio/virtio_vdpa.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
>>> index eb6aee8c06b2..989e2d7184ce 100644
>>> --- a/drivers/virtio/virtio_vdpa.c
>>> +++ b/drivers/virtio/virtio_vdpa.c
>>> @@ -385,7 +385,9 @@ static int virtio_vdpa_find_vqs(struct virtio_device
>>> *vdev, unsigned int nvqs,
>>>                           err = PTR_ERR(vqs[i]);
>>>                           goto err_setup_vq;
>>>                   }
>>> -               ops->set_vq_affinity(vdpa, i, &masks[i]);
>>> +
>>> +               if (ops->set_vq_affinity)
>>> +                       ops->set_vq_affinity(vdpa, i, &masks[i]);
>> if ops->set_vq_affinity is NULL, should give an error code to err, and
>> return err
>>
> I don't think so: the set_vq_affinity is marked as optional.
> 
Yes, I see
>>>           }
>>>
>>>           cb.callback = virtio_vdpa_config_cb;
>>> --
>>> 2.40.1
>>>
> 
>

Feng Liu May 4, 2023, 5:47 p.m. UTC | #4

On 2023-05-04 a.m.9:50, Dragos Tatulea wrote:
> External email: Use caution opening links or attachments
> 
> 
> The referenced patch calls set_vq_affinity without checking if the op is
> valid. This patch adds the check.
> 
> Fixes: 3dad56823b53 ("virtio-vdpa: Support interrupt affinity spreading mechanism")
> Reviewed-by: Gal Pressman <gal@nvidia.com>
> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> ---
Reviewed-by: Feng Liu <feliu@nvidia.com>

>   drivers/virtio/virtio_vdpa.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> index eb6aee8c06b2..989e2d7184ce 100644
> --- a/drivers/virtio/virtio_vdpa.c
> +++ b/drivers/virtio/virtio_vdpa.c
> @@ -385,7 +385,9 @@ static int virtio_vdpa_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
>                          err = PTR_ERR(vqs[i]);
>                          goto err_setup_vq;
>                  }
> -               ops->set_vq_affinity(vdpa, i, &masks[i]);
> +
> +               if (ops->set_vq_affinity)
> +                       ops->set_vq_affinity(vdpa, i, &masks[i]);
>          }
> 
>          cb.callback = virtio_vdpa_config_cb;
> --
> 2.40.1
>

Michael S. Tsirkin May 4, 2023, 6:51 p.m. UTC | #5

On Thu, May 04, 2023 at 01:08:54PM -0400, Feng Liu wrote:
> 
> 
> On 2023-05-04 a.m.9:50, Dragos Tatulea wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > The referenced patch calls set_vq_affinity without checking if the op is
> > valid. This patch adds the check.
> > 
> > Fixes: 3dad56823b53 ("virtio-vdpa: Support interrupt affinity spreading mechanism")
> > Reviewed-by: Gal Pressman <gal@nvidia.com>
> > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> > ---
> >   drivers/virtio/virtio_vdpa.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> > index eb6aee8c06b2..989e2d7184ce 100644
> > --- a/drivers/virtio/virtio_vdpa.c
> > +++ b/drivers/virtio/virtio_vdpa.c
> > @@ -385,7 +385,9 @@ static int virtio_vdpa_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
> >                          err = PTR_ERR(vqs[i]);
> >                          goto err_setup_vq;
> >                  }
> > -               ops->set_vq_affinity(vdpa, i, &masks[i]);
> > +
> > +               if (ops->set_vq_affinity)
> > +                       ops->set_vq_affinity(vdpa, i, &masks[i]);
> if ops->set_vq_affinity is NULL, should give an error code to err, and
> return err

Given we ignore return code, hardly seems like a critical thing to do.
Is it really important? affinity is an optimization isn't it?

> >          }
> > 
> >          cb.callback = virtio_vdpa_config_cb;
> > --
> > 2.40.1
> >

Feng Liu May 4, 2023, 10:47 p.m. UTC | #6

On 2023-05-04 p.m.2:51, Michael S. Tsirkin wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, May 04, 2023 at 01:08:54PM -0400, Feng Liu wrote:
>>
>>
>> On 2023-05-04 a.m.9:50, Dragos Tatulea wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> The referenced patch calls set_vq_affinity without checking if the op is
>>> valid. This patch adds the check.
>>>
>>> Fixes: 3dad56823b53 ("virtio-vdpa: Support interrupt affinity spreading mechanism")
>>> Reviewed-by: Gal Pressman <gal@nvidia.com>
>>> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
>>> ---
>>>    drivers/virtio/virtio_vdpa.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
>>> index eb6aee8c06b2..989e2d7184ce 100644
>>> --- a/drivers/virtio/virtio_vdpa.c
>>> +++ b/drivers/virtio/virtio_vdpa.c
>>> @@ -385,7 +385,9 @@ static int virtio_vdpa_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
>>>                           err = PTR_ERR(vqs[i]);
>>>                           goto err_setup_vq;
>>>                   }
>>> -               ops->set_vq_affinity(vdpa, i, &masks[i]);
>>> +
>>> +               if (ops->set_vq_affinity)
>>> +                       ops->set_vq_affinity(vdpa, i, &masks[i]);
>> if ops->set_vq_affinity is NULL, should give an error code to err, and
>> return err
> 
> Given we ignore return code, hardly seems like a critical thing to do.
> Is it really important? affinity is an optimization isn't it?
> 
Yes, it is an optimization, got it.

>>>           }
>>>
>>>           cb.callback = virtio_vdpa_config_cb;
>>> --
>>> 2.40.1
>>>
>

Dragos Tatulea May 12, 2023, 12:51 p.m. UTC | #7

On Thu, 2023-05-04 at 14:51 -0400, Michael S. Tsirkin wrote:
> On Thu, May 04, 2023 at 01:08:54PM -0400, Feng Liu wrote:
> > 
> > 
> > On 2023-05-04 a.m.9:50, Dragos Tatulea wrote:
> > > External email: Use caution opening links or attachments
> > > 
> > > 
> > > The referenced patch calls set_vq_affinity without checking if the op is
> > > valid. This patch adds the check.
> > > 
> > > Fixes: 3dad56823b53 ("virtio-vdpa: Support interrupt affinity spreading
> > > mechanism")
> > > Reviewed-by: Gal Pressman <gal@nvidia.com>
> > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> > > ---
> > >   drivers/virtio/virtio_vdpa.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> > > index eb6aee8c06b2..989e2d7184ce 100644
> > > --- a/drivers/virtio/virtio_vdpa.c
> > > +++ b/drivers/virtio/virtio_vdpa.c
> > > @@ -385,7 +385,9 @@ static int virtio_vdpa_find_vqs(struct virtio_device
> > > *vdev, unsigned int nvqs,
> > >                          err = PTR_ERR(vqs[i]);
> > >                          goto err_setup_vq;
> > >                  }
> > > -               ops->set_vq_affinity(vdpa, i, &masks[i]);
> > > +
> > > +               if (ops->set_vq_affinity)
> > > +                       ops->set_vq_affinity(vdpa, i, &masks[i]);
> > if ops->set_vq_affinity is NULL, should give an error code to err, and
> > return err
> 
> Given we ignore return code, hardly seems like a critical thing to do.
> Is it really important? affinity is an optimization isn't it?
> 
> > > 
set_vq_affinity is optional so it's not an error if the op is not implemented.

Is there anything else that needs to be done for this fix?

Thanks,
Dragos

Michael S. Tsirkin May 12, 2023, 1:30 p.m. UTC | #8

On Fri, May 12, 2023 at 12:51:21PM +0000, Dragos Tatulea wrote:
> On Thu, 2023-05-04 at 14:51 -0400, Michael S. Tsirkin wrote:
> > On Thu, May 04, 2023 at 01:08:54PM -0400, Feng Liu wrote:
> > > 
> > > 
> > > On 2023-05-04 a.m.9:50, Dragos Tatulea wrote:
> > > > External email: Use caution opening links or attachments
> > > > 
> > > > 
> > > > The referenced patch calls set_vq_affinity without checking if the op is
> > > > valid. This patch adds the check.
> > > > 
> > > > Fixes: 3dad56823b53 ("virtio-vdpa: Support interrupt affinity spreading
> > > > mechanism")
> > > > Reviewed-by: Gal Pressman <gal@nvidia.com>
> > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> > > > ---
> > > >   drivers/virtio/virtio_vdpa.c | 4 +++-
> > > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> > > > index eb6aee8c06b2..989e2d7184ce 100644
> > > > --- a/drivers/virtio/virtio_vdpa.c
> > > > +++ b/drivers/virtio/virtio_vdpa.c
> > > > @@ -385,7 +385,9 @@ static int virtio_vdpa_find_vqs(struct virtio_device
> > > > *vdev, unsigned int nvqs,
> > > >                          err = PTR_ERR(vqs[i]);
> > > >                          goto err_setup_vq;
> > > >                  }
> > > > -               ops->set_vq_affinity(vdpa, i, &masks[i]);
> > > > +
> > > > +               if (ops->set_vq_affinity)
> > > > +                       ops->set_vq_affinity(vdpa, i, &masks[i]);
> > > if ops->set_vq_affinity is NULL, should give an error code to err, and
> > > return err
> > 
> > Given we ignore return code, hardly seems like a critical thing to do.
> > Is it really important? affinity is an optimization isn't it?
> > 
> > > > 
> set_vq_affinity is optional so it's not an error if the op is not implemented.
> 
> Is there anything else that needs to be done for this fix?
> 
> Thanks,
> Dragos
> 

no, it's queued already.

diff mbox series

Patch

diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
index eb6aee8c06b2..989e2d7184ce 100644
--- a/drivers/virtio/virtio_vdpa.c
+++ b/drivers/virtio/virtio_vdpa.c
@@ -385,7 +385,9 @@  static int virtio_vdpa_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
 			err = PTR_ERR(vqs[i]);
 			goto err_setup_vq;
 		}
-		ops->set_vq_affinity(vdpa, i, &masks[i]);
+
+		if (ops->set_vq_affinity)
+			ops->set_vq_affinity(vdpa, i, &masks[i]);
 	}
 
 	cb.callback = virtio_vdpa_config_cb;