Re: [PATCH] virtio-vdpa: Fix unchecked call to NULL set_vq_affinity
On Fri, May 12, 2023 at 04:55:38PM -0700, Shannon Nelson wrote:
> On 5/12/23 6:30 AM, Michael S. Tsirkin wrote:
> >
> > On Fri, May 12, 2023 at 12:51:21PM +, 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
> > > > > > Signed-off-by: Dragos Tatulea
> > > > > > ---
> > > > > >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.
>
> Are these queued into a repo that is accessible? I haven't seen activity in
> the vhost.git where I would have expected it. After stumbling over and
> debugging this same problem, I was happy to see it fixed, and I'd like to
> pull from a repo that has the current updates.
>
> Thanks,
> sln
Pushed to next now.
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-vdpa: Fix unchecked call to NULL set_vq_affinity
On 5/12/23 6:30 AM, Michael S. Tsirkin wrote:
On Fri, May 12, 2023 at 12:51:21PM +, 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
Signed-off-by: Dragos Tatulea
---
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.
Are these queued into a repo that is accessible? I haven't seen
activity in the vhost.git where I would have expected it. After
stumbling over and debugging this same problem, I was happy to see it
fixed, and I'd like to pull from a repo that has the current updates.
Thanks,
sln
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-vdpa: Fix unchecked call to NULL set_vq_affinity
On Fri, May 12, 2023 at 12:51:21PM +, 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
> > > > Signed-off-by: Dragos Tatulea
> > > > ---
> > > > 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.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-vdpa: Fix unchecked call to NULL set_vq_affinity
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
> > > Signed-off-by: Dragos Tatulea
> > > ---
> > > 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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-vdpa: Fix unchecked call to NULL set_vq_affinity
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
Signed-off-by: Dragos Tatulea
---
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-vdpa: Fix unchecked call to NULL set_vq_affinity
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
> > Signed-off-by: Dragos Tatulea
> > ---
> > 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
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-vdpa: Fix unchecked call to NULL set_vq_affinity
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
Signed-off-by: Dragos Tatulea
---
Reviewed-by: Feng Liu
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-vdpa: Fix unchecked call to NULL set_vq_affinity
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
Signed-off-by: Dragos Tatulea
---
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-vdpa: Fix unchecked call to NULL set_vq_affinity
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
> > Signed-off-by: Dragos Tatulea
> > ---
> > 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
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-vdpa: Fix unchecked call to NULL set_vq_affinity
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
Signed-off-by: Dragos Tatulea
---
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
