On Tue, Dec 21, 2021 at 5:00 PM Eli Cohen <elic@xxxxxxxxxx> wrote:
>
> On Tue, Dec 21, 2021 at 03:51:47PM +0800, Jason Wang wrote:
> > On Tue, Dec 21, 2021 at 2:17 PM Eli Cohen <elic@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Dec 21, 2021 at 01:51:32PM +0800, Jason Wang wrote:
> > > > On Sun, Dec 19, 2021 at 10:03 PM Eli Cohen <elic@xxxxxxxxxx> wrote:
> > > > >
> > > > > Avoid reading device configuration during feature negotiation. Read
> > > > > device status and verify that VIRTIO_CONFIG_S_FEATURES_OK is set.
> > > > > Otherwise, return -EAGAIN.
> > > > >
> > > > > Signed-off-by: Eli Cohen <elic@xxxxxxxxxx>
> > > > > ---
> > > > > drivers/vdpa/vdpa.c | 7 +++++++
> > > > > 1 file changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > > > > index 42d71d60d5dc..5749cf0a1500 100644
> > > > > --- a/drivers/vdpa/vdpa.c
> > > > > +++ b/drivers/vdpa/vdpa.c
> > > > > @@ -819,8 +819,15 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
> > > > > {
> > > > > u32 device_id;
> > > > > void *hdr;
> > > > > + u8 status;
> > > > > int err;
> > > > >
> > > > > + status = vdev->config->get_status(vdev);
> > > > > + if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> > > > > + NL_SET_ERR_MSG_MOD(extack, "Features negotiation not completed");
> > > > > + return -EAGAIN;
> > > > > + }
> > > > > +
> > > >
> > > > I wonder how we synchronize this with set_status(), set/get_config()?
> > > >
> > >
> > > Not sure why set/get_config() is related to this.
> >
> > I think guest may trigger set_config() while we are at
> > vdpa_dev_config_fill(), this will lead to inconsistent values.
> >
> > > How about each upstream driver will take care to have a lock that
> > > serializes set_status() with get_driver_features() to guarantee that we
> > > always provide valid features?
> >
> > Yes and it looks like we've already had a per device cf_mutex, how
> > about simply use this?
>
> Maybe do it in one place. For example, for vhost, in
> drivers/vhost/vdpa.c:
>
> mutex_lock(&vdpa->cf_mutex);
> ops->set_status(vdpa, status);
> mutex_unlock(&vdpa->cf_mutex);
>
> same for drivers/virtio/virtio_vdpa.c
>
> OVerall, set_status, get_status, set_config, get_config should be
> protected.
Yes, maybe we can introduce new helpers in vdpa.c.
Thanks
>
>
> >
> > Thanks
> >
> > >
> > > > Thanks
> > > >
> > > > > hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
> > > > > VDPA_CMD_DEV_CONFIG_GET);
> > > > > if (!hdr)
> > > > > --
> > > > > 2.34.1
> > > > >
> > > >
> > >
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
