netdev - RE: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F

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]

Date:   Tue, 14 Jun 2022 19:26:10 +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 5/6] vDPA: answer num of queue pairs = 1 to userspace
 when VIRTIO_NET_F_MQ == 0

> From: Zhu, Lingshan <lingshan.zhu@...el.com>
> Sent: Monday, June 13, 2022 10:33 PM
> 
> 
> On 6/14/2022 4:36 AM, Parav Pandit wrote:
> >
> >> From: Zhu Lingshan <lingshan.zhu@...el.com>
> >> Sent: Monday, June 13, 2022 6:17 AM
> >> To: jasowang@...hat.com; mst@...hat.com
> >> Cc: virtualization@...ts.linux-foundation.org;
> >> netdev@...r.kernel.org; Parav Pandit <parav@...dia.com>;
> >> xieyongji@...edance.com; gautam.dawar@....com; Zhu Lingshan
> >> <lingshan.zhu@...el.com>
> >> Subject: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to
> >> userspace when VIRTIO_NET_F_MQ == 0
> >>
> >> If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue
> >> pair, so when userspace querying queue pair numbers, it should return
> >> mq=1 than zero.
> >>
> >> Function vdpa_dev_net_config_fill() fills the attributions of the
> >> vDPA devices, so that it should call vdpa_dev_net_mq_config_fill() so
> >> the parameter in vdpa_dev_net_mq_config_fill() should be
> >> feature_device than feature_driver for the vDPA devices themselves
> >>
> >> Before this change, when MQ = 0, iproute2 output:
> >> $vdpa dev config show vdpa0
> >> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs
> >> 0 mtu 1500
> >>
> > Max_vq_pairs should not be printed when _MQ feature is not negotiated.
> > Existing code that returns 0 is correct following this line of the spec.
> >
> > " The following driver-read-only field, max_virtqueue_pairs only
> > exists if VIRTIO_NET_F_MQ or VIRTIO_- NET_F_RSS is set."
> > The field doesn't exist when _MQ is not there. Hence, it should not be
> printed.
> > Is _RSS offered and is that why you see it?
> >
> > If not a fix in the iproute2/vdpa should be done.
> IMHO, The spec says:
> The following driver-read-only field, max_virtqueue_pairs only exists if
> VIRTIO_NET_F_MQ is *set*
> 
> The spec doesn't say:
> The following driver-read-only field, max_virtqueue_pairs only exists if
> VIRTIO_NET_F_MQ is *negotiated*
> 
> If a device is capable of of multi-queues, this capability does not depend on
> the driver. We are querying the device, not the driver.
> 
> If there is MQ, we print the onboard implemented total number of the
> queue pairs.
> If MQ is not set, we will not read the onboard mq number, because it is not
> there as the spec says.
> But there should be at least one queue pair to be a functional virtio-net, so 1
> is printed.

The commit [1] is supposed to show the device configuration layout as what device _offers_ as mentioned in the subject line of the commit [1] very clearly.

The commit [2] changed the original intent of commit [1] even though commit [2] mentioned "fixes" in the patch without any fixes tag. :(
commit [2] was bug.

The right fix to restore [1] is to report the device features offered using get_device_features() callback instead of get_drivers_features().

Once above fix is applied,
when _MQ is not offered, max_queue_pairs should be treated as 1 by the driver.
We do not have a concept of management driver yet in the specification.
So when _MQ is not offered by the device, either kernel driver can return max_queue_pairs = 1, or the user space caller can see that _MQ is not offered by the device hence treat max_vq_pairs = 1.
(Like how it is described in the spec as - " Identify and initialize the receive and transmission virtqueues, up to N of each kind. If VIRTIO_NET_F_MQ feature bit is negotiated, N=max_virtqueue_pairs, otherwise identify N=1."

So let orchestration layer can certainly derive this N when _MQ feature is not offered, instead of coming from the vdpa management layer.
I agree that this extra if() condition in the user space can be avoided if kernel always provides it.
But better to avoid such assumption because we have more such config space attributes. i.e., they exist only when features are offered.
So to keep it uniform, I prefer we avoid the exception for max_virtqueue_pairs.

Please submit the fix for [2] to call get_device_features() for purpose of returning config space.
Continue to use get_driver_features() to show VDPA_ATTR_DEV_NEGOTIATED_FEATURES.

I didn’t review these patches and it slipped through the cracks. :(

[1] a64917bc2e9b ("vdpa: Introduce query of device config layout")
[2] a64917bc2e9 ("vdpa: Provide interface to read driver features")