netdev - RE: [PATCH V3 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]

Message-ID: <PH0PR12MB5481FF0AE64F3BB24FF8A869DC829@PH0PR12MB5481.namprd12.prod.outlook.com>
Date:   Fri, 8 Jul 2022 16:23:36 +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 V3 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: Friday, July 8, 2022 2:21 AM
> 
> 
> On 7/2/2022 6:07 AM, Parav Pandit wrote:
> >
> >> From: Zhu Lingshan <lingshan.zhu@...el.com>
> >> Sent: Friday, July 1, 2022 9:28 AM
> >> 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
> >>
> > The fix belongs to user space.
> > When a feature bit _MQ is not negotiated, vdpa kernel space will not add
> attribute VDPA_ATTR_DEV_NET_CFG_MAX_VQP.
> > When such attribute is not returned by kernel, max_vq_pairs should not
> be shown by the iproute2.
> I think userspace tool does not need to care whether MQ is offered or
> negotiated, it just needs to read the number of queues there, so if no MQ, it
> is not "not any queues", there are still 1 queue pair to be a virtio-net device,
> means two queues.
> 
> If not, how can you tell the user there are only 2 queues? The end users may
> don't know this is default. They may misunderstand this as an error or
> defects.
> >
When max_vq_pairs is not shown, it means that device didn’t expose MAX_VQ_PAIRS attribute to its guest users.
(Because _MQ was not negotiated).
It is not error or defect. 
It precisely shows what is exposed.

User space will care when it wants to turn off/on _MQ feature bits and MAX_QP values.

Showing max_vq_pairs of 1 even when _MQ is not negotiated, incorrectly says that max_vq_pairs is exposed to the guest, but it is not offered.

So, please fix the iproute2 to not print max_vq_pairs when it is not returned by the kernel.

> > We have many config space fields that depend on the feature bits and
> some of them do not have any defaults.
> > To keep consistency of existence of config space fields among all, we don't
> want to show default like below.
> >
> > Please fix the iproute2 to not print max_vq_pairs when it is not returned
> by the kernel.
> >
> >> After applying this commit, 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
> >> 1 mtu 1500
> >>
> >> Fixes: a64917bc2e9b (vdpa: Provide interface to read driver features)
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@...el.com>
> >> ---
> >>   drivers/vdpa/vdpa.c | 7 ++++---
> >>   1 file changed, 4 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> >> d76b22b2f7ae..846dd37f3549 100644
> >> --- a/drivers/vdpa/vdpa.c
> >> +++ b/drivers/vdpa/vdpa.c
> >> @@ -806,9 +806,10 @@ static int vdpa_dev_net_mq_config_fill(struct
> >> vdpa_device *vdev,
> >>   	u16 val_u16;
> >>
> >>   	if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0)
> >> -		return 0;
> >> +		val_u16 = 1;
> >> +	else
> >> +		val_u16 = __virtio16_to_cpu(true, config-
> >>> max_virtqueue_pairs);
> >> -	val_u16 = le16_to_cpu(config->max_virtqueue_pairs);
> >>   	return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
> val_u16);
> >> }
> >>
> >> @@ -842,7 +843,7 @@ static int vdpa_dev_net_config_fill(struct
> >> vdpa_device *vdev, struct sk_buff *ms
> >>   			      VDPA_ATTR_PAD))
> >>   		return -EMSGSIZE;
> >>
> >> -	return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver,
> >> &config);
> >> +	return vdpa_dev_net_mq_config_fill(vdev, msg, features_device,
> >> +&config);
> >>   }
> >>
> >>   static int
> >> --
> >> 2.31.1