netdev - RE: [PATCH 3/6] vDPA/ifcvf: support userspace to query device feature bits

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <DM8PR12MB5400F0F46CD4B0749A60FE8BABDE9@DM8PR12MB5400.namprd12.prod.outlook.com>
Date:   Thu, 2 Jun 2022 07:46:26 +0000
From:   Eli Cohen <elic@...dia.com>
To:     Jason Wang <jasowang@...hat.com>,
        Zhu Lingshan <lingshan.zhu@...el.com>
CC:     mst <mst@...hat.com>,
        virtualization <virtualization@...ts.linux-foundation.org>,
        netdev <netdev@...r.kernel.org>, Parav Pandit <parav@...dia.com>
Subject: RE: [PATCH 3/6] vDPA/ifcvf: support userspace to query device feature
 bits



> -----Original Message-----
> From: Jason Wang <jasowang@...hat.com>
> Sent: Thursday, June 2, 2022 10:32 AM
> To: Zhu Lingshan <lingshan.zhu@...el.com>
> Cc: mst <mst@...hat.com>; virtualization <virtualization@...ts.linux-foundation.org>; netdev <netdev@...r.kernel.org>; Eli Cohen
> <elic@...dia.com>; Parav Pandit <parav@...dia.com>
> Subject: Re: [PATCH 3/6] vDPA/ifcvf: support userspace to query device feature bits
> 
> On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@...el.com> wrote:
> >
> > This commit supports userspace to query device feature bits
> > by filling the relevant netlink attribute.
> >
> > There are two types of netlink attributes:
> > VDPA_ATTR_DEV_XXXX work for virtio devices config space, and
> > VDPA_ATTR_MGMTDEV_XXXX work for the management devices.
> >
> > This commit fixes a misuse of VDPA_ATTR_DEV_SUPPORTED_FEATURES,
> > this attr is for a virtio device, not management devices.
> >
> > Thus VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES is introduced for
> > reporting management device features, and VDPA_ATTR_DEV_SUPPORTED_FEATURES
> > for virtio devices feature bits.
> >
> > Signed-off-by: Zhu Lingshan <lingshan.zhu@...el.com>
> > ---
> >  drivers/vdpa/vdpa.c       | 15 ++++++++++-----
> >  include/uapi/linux/vdpa.h |  1 +
> >  2 files changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > index 2b75c00b1005..c820dd2b0307 100644
> > --- a/drivers/vdpa/vdpa.c
> > +++ b/drivers/vdpa/vdpa.c
> > @@ -508,7 +508,7 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
> >                 err = -EMSGSIZE;
> >                 goto msg_err;
> >         }
> > -       if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
> > +       if (nla_put_u64_64bit(msg, VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES,
> 
> Adding Eli and Parav.
> 
> If I understand correctly, we can't provision virtio features right
> now. This means, the vDPA instance should have the same features as
> its parent (management device).

A management device should be read as "a management device capable of
instantiating a virtio device". Thus, I see no reason to introduce another attribute.
VDPA_ATTR_DEV_SUPPORTED_FEATURES is the capability of the management
device to spawn a virtio device with certain features.

> 
> And it seems to me if we can do things like this, we need first allow
> the features to be provisioned. (And this change breaks uABI)
> 
> Thanks
> 
> 
> >                               mdev->supported_features, VDPA_ATTR_PAD)) {
> >                 err = -EMSGSIZE;
> >                 goto msg_err;
> > @@ -827,7 +827,7 @@ static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
> >  static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg)
> >  {
> >         struct virtio_net_config config = {};
> > -       u64 features;
> > +       u64 features_device, features_driver;
> >         u16 val_u16;
> >
> >         vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config));
> > @@ -844,12 +844,17 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> >         if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
> >                 return -EMSGSIZE;
> >
> > -       features = vdev->config->get_driver_features(vdev);
> > -       if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
> > +       features_driver = vdev->config->get_driver_features(vdev);
> > +       if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver,
> >                               VDPA_ATTR_PAD))
> >                 return -EMSGSIZE;
> >
> > -       return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> > +       features_device = vdev->config->get_device_features(vdev);
> > +       if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES, features_device,
> > +                             VDPA_ATTR_PAD))
> > +               return -EMSGSIZE;
> > +
> > +       return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, &config);
> >  }
> >
> >  static int
> > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > index 1061d8d2d09d..70a3672c288f 100644
> > --- a/include/uapi/linux/vdpa.h
> > +++ b/include/uapi/linux/vdpa.h
> > @@ -30,6 +30,7 @@ enum vdpa_attr {
> >         VDPA_ATTR_MGMTDEV_BUS_NAME,             /* string */
> >         VDPA_ATTR_MGMTDEV_DEV_NAME,             /* string */
> >         VDPA_ATTR_MGMTDEV_SUPPORTED_CLASSES,    /* u64 */
> > +       VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES,   /* u64 */
> >
> >         VDPA_ATTR_DEV_NAME,                     /* string */
> >         VDPA_ATTR_DEV_ID,                       /* u32 */
> > --
> > 2.31.1
> >