netdev - Re: [PATCH 2/2] vDPA: conditionally read fields in virtio-net dev

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 16 Aug 2022 12:18:37 +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>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "xieyongji@...edance.com" <xieyongji@...edance.com>,
        "gautam.dawar@....com" <gautam.dawar@....com>
Subject: Re: [PATCH 2/2] vDPA: conditionally read fields in virtio-net dev



On 8/16/2022 10:32 AM, Parav Pandit wrote:
>> From: Zhu Lingshan <lingshan.zhu@...el.com>
>> Sent: Monday, August 15, 2022 5:27 AM
>>
>> Some fields of virtio-net device config space are conditional on the feature
>> bits, the spec says:
>>
>> "The mac address field always exists
>> (though is only valid if VIRTIO_NET_F_MAC is set)"
>>
>> "max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ or
>> VIRTIO_NET_F_RSS is set"
>>
>> "mtu only exists if VIRTIO_NET_F_MTU is set"
>>
>> so we should read MTU, MAC and MQ in the device config space only when
>> these feature bits are offered.
> Yes.
>
>> For MQ, if both VIRTIO_NET_F_MQ and VIRTIO_NET_F_RSS are not set, the
>> virtio device should have one queue pair as default value, so when userspace
>> querying queue pair numbers, it should return mq=1 than zero.
> No.
> No need to treat mac and max_qps differently.
> It is meaningless to differentiate when field exist/not-exists vs value valid/not valid.
as we discussed before, MQ has a default value 1, to be a functional 
virtio-net device,
while MAC has no default value, if no VIRTIO_NET_F_MAC set, the driver 
should generate
a random MAC.
>
>> For MTU, if VIRTIO_NET_F_MTU is not set, we should not read MTU from
>> the device config sapce.
>> RFC894 <A Standard for the Transmission of IP Datagrams over Ethernet
>> Networks> says:"The minimum length of the data field of a packet sent over
>> an Ethernet is 1500 octets, thus the maximum length of an IP datagram sent
>> over an Ethernet is 1500 octets.  Implementations are encouraged to support
>> full-length packets"
> This line in the RFC 894 of 1984 is wrong.
> Errata already exists for it at [1].
>
> [1] https://www.rfc-editor.org/errata_search.php?rfc=894&rec_status=0
OK, so I think we should return nothing if _F_MTU not set, like handling 
the MAC
>
>> virtio spec says:"The virtio network device is a virtual ethernet card", so the
>> default MTU value should be 1500 for virtio-net.
>>
> Practically I have seen 1500 and highe mtu.
> And this derivation is not good of what should be the default mtu as above errata exists.
>
> And I see the code below why you need to work so hard to define a default value so that _MQ and _MTU can report default values.
>
> There is really no need for this complexity and such a long commit message.
>
> Can we please expose feature bits as-is and report config space field which are valid?
>
> User space will be querying both.
I think MAC and MTU don't have default values, so return nothing if the 
feature bits not set,
for MQ, it is still max_vq_paris == 1 by default.
>
>> For MAC, the spec says:"If the VIRTIO_NET_F_MAC feature bit is set, the
>> configuration space mac entry indicates the “physical” address of the
>> network card, otherwise the driver would typically generate a random local
>> MAC address." So there is no default MAC address if VIRTIO_NET_F_MAC
>> not set.
>>
>> This commits introduces functions vdpa_dev_net_mtu_config_fill() and
>> vdpa_dev_net_mac_config_fill() to fill MTU and MAC.
>> It also fixes vdpa_dev_net_mq_config_fill() to report correct MQ when
>> _F_MQ is not present.
>>
> Multiple changes in single patch are not good idea.
> Its ok to split to smaller patches.
OK, I can try to split it.
>
>> +	if ((features & BIT_ULL(VIRTIO_NET_F_MTU)) == 0)
>> +		val_u16 = 1500;
>> +	else
>> +		val_u16 = __virtio16_to_cpu(true, config->mtu);
>> +
> Need to work hard to find default values and that too turned out had errata.
> There are more fields that doesn’t have default values.
>
> There is no point in kernel doing this guess work, that user space can figure out of what is valid/invalid.
It's not guest work, when guest finds no feature bits set, it can decide 
what to do. These code only for the
user space, just MST ever suggest, if there is a default value, we can 
return it from the kernel, once for all.

Thanks
>