linux-kernel - Re: [PATCH v3 2/6] vdpa: conditionally read STATUS in config space

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: <7b3af822-0075-889c-911c-b79860fd5ce2@oracle.com>
Date:   Sun, 5 Feb 2023 20:53:42 -0800
From:   Si-Wei Liu <si-wei.liu@...cle.com>
To:     Eli Cohen <elic@...dia.com>, mst@...hat.com, jasowang@...hat.com,
        parav@...dia.com
Cc:     virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/6] vdpa: conditionally read STATUS in config space



On 2/5/2023 12:26 AM, Eli Cohen wrote:
>
> On 03/02/2023 7:01, Si-Wei Liu wrote:
>> The spec says:
>>      status only exists if VIRTIO_NET_F_STATUS is set
>>
>> Similar to MAC and MTU, vdpa_dev_net_config_fill() should read
>> STATUS conditionally depending on the feature bits.
>>
>> Signed-off-by: Si-Wei Liu <si-wei.liu@...cle.com>
>> Reviewed-by: Parav Pandit <parav@...dia.com>
>> ---
>>   drivers/vdpa/vdpa.c | 20 +++++++++++++++-----
>>   1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>> index 3a82ca78..21c8aa3 100644
>> --- a/drivers/vdpa/vdpa.c
>> +++ b/drivers/vdpa/vdpa.c
>> @@ -843,18 +843,25 @@ static int vdpa_dev_net_mac_config_fill(struct 
>> sk_buff *msg, u64 features,
>>               sizeof(config->mac), config->mac);
>>   }
>>   +static int vdpa_dev_net_status_config_fill(struct sk_buff *msg, 
>> u64 features,
>> +                       const struct virtio_net_config *config)
>> +{
>> +    u16 val_u16;
>> +
>> +    if ((features & BIT_ULL(VIRTIO_NET_F_STATUS)) == 0)
>> +        return 0;
> Instead of returning 0 here, it would be better to explicitly put 0 in 
> the message field for
>
> VDPA_ATTR_DEV_NET_STATUS
In light of commit 41a2ad927aa2 ("vDPA: conditionally read MTU and MAC 
in dev cfg space"), the userspace must now show the config space field 
presented by the device *as-is*. If the feature bit is not offered by 
device, the relevant field will not be displayed in 'vdpa dev config' 
output. For instance, MAC address won't be shown if the MAC feature is 
not supported/offered by the device (note this has nothing to do with 
negotiated features), even though the vdpa parent may have a non-zero 
MAC address of its own. I think STATUS should not be different from MAC 
and MTU here.

Regards,
-Siwei

>
>> +
>> +    val_u16 = __virtio16_to_cpu(true, config->status);
>> +    return nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16);
>> +}
>> +
>>   static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, 
>> struct sk_buff *msg)
>>   {
>>       struct virtio_net_config config = {};
>>       u64 features_device;
>> -    u16 val_u16;
>>         vdev->config->get_config(vdev, 0, &config, sizeof(config));
>>   -    val_u16 = __virtio16_to_cpu(true, config.status);
>> -    if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
>> -        return -EMSGSIZE;
>> -
>>       features_device = vdev->config->get_device_features(vdev);
>>         if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES, 
>> features_device,
>> @@ -867,6 +874,9 @@ static int vdpa_dev_net_config_fill(struct 
>> vdpa_device *vdev, struct sk_buff *ms
>>       if (vdpa_dev_net_mac_config_fill(msg, features_device, &config))
>>           return -EMSGSIZE;
>>   +    if (vdpa_dev_net_status_config_fill(msg, features_device, 
>> &config))
>> +        return -EMSGSIZE;
>> +
>>       return vdpa_dev_net_mq_config_fill(msg, features_device, &config);
>>   }