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 02:32:42 +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>,
        "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


> 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.

> 
> 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

> 
> 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.

> 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.

> +	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.