Virtualization - Dec 2021 - [PATCH v3 03/10] vdpa: Read device configuration only if FEATURES_OKg

On Tue, Dec 21, 2021 at 5:00 PM Eli Cohen <elic at nvidia.com>
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 at nvidia.com>
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 at
nvidia.com> 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 at nvidia.com>
> > > > > ---
> > > > >  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
> > > > >
> > > >
> > >
> >
>