netdev - RE: [PATCH V2 3/6] vDPA: allow userspace to query features of a vDPA device

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: <PH0PR12MB54816E481633EEB9C24D4D5CDCAA9@PH0PR12MB5481.namprd12.prod.outlook.com>
Date:   Tue, 14 Jun 2022 18:38:16 +0000
From:   Parav Pandit <parav@...dia.com>
To:     "Zhu, Lingshan" <lingshan.zhu@...el.com>,
        "jasowang@...hat.com" <jasowang@...hat.com>,
        "mst@...hat.com" <mst@...hat.com>
CC:     "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "xieyongji@...edance.com" <xieyongji@...edance.com>,
        "gautam.dawar@....com" <gautam.dawar@....com>
Subject: RE: [PATCH V2 3/6] vDPA: allow userspace to query features of a vDPA
 device



> From: Zhu, Lingshan <lingshan.zhu@...el.com>
> Sent: Monday, June 13, 2022 10:53 PM
> 
> 
> On 6/14/2022 4:42 AM, Parav Pandit wrote:
> >
> >> From: Zhu Lingshan <lingshan.zhu@...el.com>
> >> Sent: Monday, June 13, 2022 6:17 AM
> >> device
> >>
> >> This commit adds a new vDPA netlink attribution
> >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
> features
> >> of vDPA devices through this new attr.
> >>
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@...el.com>
> >> ---
> >>   drivers/vdpa/vdpa.c       | 13 +++++++++----
> >>   include/uapi/linux/vdpa.h |  1 +
> >>   2 files changed, 10 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> >> ebf2f363fbe7..9b0e39b2f022 100644
> >> --- a/drivers/vdpa/vdpa.c
> >> +++ b/drivers/vdpa/vdpa.c
> >> @@ -815,7 +815,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)); @@ -
> >> 832,12 +832,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;
> >> +
> >> +	features_device = vdev->config->get_device_features(vdev);
> >> +	if (nla_put_u64_64bit(msg,
> >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,
> >> +features_device,
> >>   			      VDPA_ATTR_PAD))
> >>   		return -EMSGSIZE;
> >>
> >> -	return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> >> +	return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver,
> >> +&config);
> >>   }
> >>
> >>   static int
> >> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> >> index
> >> 25c55cab3d7c..39f1c3d7c112 100644
> >> --- a/include/uapi/linux/vdpa.h
> >> +++ b/include/uapi/linux/vdpa.h
> >> @@ -47,6 +47,7 @@ enum vdpa_attr {
> >>   	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
> >>   	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
> >>   	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
> > I see now what was done incorrectly with commit cd2629f6df1ca.
> >
> > Above was done with wrong name prefix that missed MGMTDEV_. :(
> Please
> > don't add VDPA_ prefix due to one mistake.
> > Please reuse this VDPA_ATTR_DEV_SUPPORTED_FEATURES for device
> attribute as well.
> currently we can reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES to report
> device features for sure, however this could confuse the readers since every
> attr should has its own unique purpose.
VDPA_ATTR_DEV_SUPPORTED_FEATURES is supposed to be VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES
And device specific features is supposed to be named as,
VDPA_ATTR_DEV_SUPPORTED_FEATURES.

But it was not done this way in commit cd2629f6df1ca.
This leads to the finding good name problem now.

Given that this new attribute is going to show same or subset of the features of the management device supported features, it is fine to reuse with exception with explicit comment in the UAPI header file.