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] [day] [month] [year] [list]

Message-ID: <b2c9df82-ad49-faad-c855-851de5c88a4c@intel.com>
Date:   Wed, 15 Jun 2022 14:54:47 +0800
From:   "Zhu, Lingshan" <lingshan.zhu@...el.com>
To:     Parav Pandit <parav@...dia.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



On 6/15/2022 2:38 AM, Parav Pandit wrote:
>
>> 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.
Yes, that's the point and what I did in my V1 series, but you see if  we 
change original VDPA_ATTR_DEV_SUPPORTED_FEATURES to 
VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES, uAPI has been changed, so iproute2 
may work improperly.
> 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.
Currently I don't see any bugs can be caused by reuse the attr 
VDPA_ATTR_DEV_SUPPORTED_FEATURES.
However I really prefer to do defensive coding, introduce a new attr is 
not a big burden, its a common practice using a dedicated attr for an 
unique type of data, this can help us avoid potential bugs, and less 
confusion for other contributors.

VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES is still better than a ATTR2, but maybe it is not pretty enough, any better namings are more than welcome!!!!!