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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 1 Aug 2022 15:53:37 -0700 From: Si-Wei Liu <si-wei.liu@...cle.com> To: Jason Wang <jasowang@...hat.com>, "Zhu, Lingshan" <lingshan.zhu@...el.com>, Parav Pandit <parav@...dia.com>, "mst@...hat.com" <mst@...hat.com>, Eli Cohen <elic@...dia.com> Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "xieyongji@...edance.com" <xieyongji@...edance.com>, "gautam.dawar@....com" <gautam.dawar@....com>, "virtualization@...ts.linux-foundation.org" <virtualization@...ts.linux-foundation.org> Subject: Re: [PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space On 7/31/2022 9:44 PM, Jason Wang wrote: > > 在 2022/7/30 04:55, Si-Wei Liu 写道: >> >> >> On 7/28/2022 7:04 PM, Zhu, Lingshan wrote: >>> >>> >>> On 7/29/2022 5:48 AM, Si-Wei Liu wrote: >>>> >>>> >>>> On 7/27/2022 7:43 PM, Zhu, Lingshan wrote: >>>>> >>>>> >>>>> On 7/28/2022 8:56 AM, Si-Wei Liu wrote: >>>>>> >>>>>> >>>>>> On 7/27/2022 4:47 AM, Zhu, Lingshan wrote: >>>>>>> >>>>>>> >>>>>>> On 7/27/2022 5:43 PM, Si-Wei Liu wrote: >>>>>>>> Sorry to chime in late in the game. For some reason I couldn't >>>>>>>> get to most emails for this discussion (I only subscribed to >>>>>>>> the virtualization list), while I was taking off amongst the >>>>>>>> past few weeks. >>>>>>>> >>>>>>>> It looks to me this patch is incomplete. Noted down the way in >>>>>>>> vdpa_dev_net_config_fill(), we have the following: >>>>>>>> features = vdev->config->get_driver_features(vdev); >>>>>>>> if (nla_put_u64_64bit(msg, >>>>>>>> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features, >>>>>>>> VDPA_ATTR_PAD)) >>>>>>>> return -EMSGSIZE; >>>>>>>> >>>>>>>> Making call to .get_driver_features() doesn't make sense when >>>>>>>> feature negotiation isn't complete. Neither should present >>>>>>>> negotiated_features to userspace before negotiation is done. >>>>>>>> >>>>>>>> Similarly, max_vqp through vdpa_dev_net_mq_config_fill() >>>>>>>> probably should not show before negotiation is done - it >>>>>>>> depends on driver features negotiated. >>>>>>> I have another patch in this series introduces device_features >>>>>>> and will report device_features to the userspace even features >>>>>>> negotiation not done. Because the spec says we should allow >>>>>>> driver access the config space before FEATURES_OK. >>>>>> The config space can be accessed by guest before features_ok >>>>>> doesn't necessarily mean the value is valid. You may want to >>>>>> double check with Michael for what he quoted earlier: >>>>> that's why I proposed to fix these issues, e.g., if no _F_MAC, >>>>> vDPA kernel should not return a mac to the userspace, there is not >>>>> a default value for mac. >>>> Then please show us the code, as I can only comment based on your >>>> latest (v4) patch and it was not there.. To be honest, I don't >>>> understand the motivation and the use cases you have, is it for >>>> debugging/monitoring or there's really a use case for live >>>> migration? For the former, you can do a direct dump on all config >>>> space fields regardless of endianess and feature negotiation >>>> without having to worry about validity (meaningful to present to >>>> admin user). To me these are conflict asks that is impossible to >>>> mix in exact one command. >>> This bug just has been revealed two days, and you will see the patch >>> soon. >>> >>> There are something to clarify: >>> 1) we need to read the device features, or how can you pick a proper >>> LM destination > > > So it's probably not very efficient to use this, the manager layer > should have the knowledge about the compatibility before doing > migration other than try-and-fail. > > And it's the task of the management to gather the nodes whose devices > could be live migrated to each other as something like "cluster" which > we've already used in the case of cpuflags. > > 1) during node bootstrap, the capability of each node and devices was > reported to management layer > 2) management layer decide the cluster and make sure the migration can > only done among the nodes insides the cluster > 3) before migration, the vDPA needs to be provisioned on the destination > > >>> 2) vdpa dev config show can show both device features and driver >>> features, there just need a patch for iproute2 >>> 3) To process information like MQ, we don't just dump the config >>> space, MST has explained before >> So, it's for live migration... Then why not export those config >> parameters specified for vdpa creation (as well as device feature >> bits) to the output of "vdpa dev show" command? That's where device >> side config lives and is static across vdpa's life cycle. "vdpa dev >> config show" is mostly for dynamic driver side config, and the >> validity is subject to feature negotiation. I suppose this should >> suit your need of LM, e.g. > > > I think so. > > >> >> $ vdpa dev add name vdpa1 mgmtdev pci/0000:41:04.2 max_vqp 7 mtu 2000 >> $ vdpa dev show vdpa1 >> vdpa1: type network mgmtdev pci/0000:41:04.2 vendor_id 5555 max_vqs >> 15 max_vq_size 256 >> max_vqp 7 mtu 2000 >> dev_features CSUM GUEST_CSUM MTU HOST_TSO4 HOST_TSO6 STATUS CTRL_VQ >> MQ CTRL_MAC_ADDR VERSION_1 RING_PACKED > > > Note that the mgmt should know this destination have those > capability/features before the provisioning. Yes, mgmt software should have to check the above from source. > > >> >> For it to work, you'd want to pass "struct vdpa_dev_set_config" to >> _vdpa_register_device() during registration, and get it saved there >> in "struct vdpa_device". Then in vdpa_dev_fill() show each field >> conditionally subject to "struct vdpa_dev_set_config.mask". >> >> Thanks, >> -Siwei > > > Thanks > > >>> >>> Thanks >>> Zhu Lingshan >>>> >>>>>>> Nope: >>>>>>> >>>>>>> 2.5.1 Driver Requirements: Device Configuration Space >>>>>>> >>>>>>> ... >>>>>>> >>>>>>> For optional configuration space fields, the driver MUST check >>>>>>> that the corresponding feature is offered >>>>>>> before accessing that part of the configuration space. >>>>>> >>>>>> and how many driver bugs taking wrong assumption of the validity >>>>>> of config space field without features_ok. I am not sure what use >>>>>> case you want to expose config resister values for before >>>>>> features_ok, if it's mostly for live migration I guess it's >>>>>> probably heading a wrong direction. >>>>>> >>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Last but not the least, this "vdpa dev config" command was not >>>>>>>> designed to display the real config space register values in >>>>>>>> the first place. Quoting the vdpa-dev(8) man page: >>>>>>>> >>>>>>>>> vdpa dev config show - Show configuration of specific device >>>>>>>>> or all devices. >>>>>>>>> DEV - specifies the vdpa device to show its configuration. If >>>>>>>>> this argument is omitted all devices configuration is listed. >>>>>>>> It doesn't say anything about configuration space or register >>>>>>>> values in config space. As long as it can convey the config >>>>>>>> attribute when instantiating vDPA device instance, and more >>>>>>>> importantly, the config can be easily imported from or exported >>>>>>>> to userspace tools when trying to reconstruct vdpa instance >>>>>>>> intact on destination host for live migration, IMHO in my >>>>>>>> personal interpretation it doesn't matter what the config space >>>>>>>> may present. It may be worth while adding a new debug command >>>>>>>> to expose the real register value, but that's another story. >>>>>>> I am not sure getting your points. vDPA now reports device >>>>>>> feature bits(device_features) and negotiated feature >>>>>>> bits(driver_features), and yes, the drivers features can be a >>>>>>> subset of the device features; and the vDPA device features can >>>>>>> be a subset of the management device features. >>>>>> What I said is after unblocking the conditional check, you'd have >>>>>> to handle the case for each of the vdpa attribute when feature >>>>>> negotiation is not yet done: basically the register values you >>>>>> got from config space via the vdpa_get_config_unlocked() call is >>>>>> not considered to be valid before features_ok (per-spec). >>>>>> Although in some case you may get sane value, such behavior is >>>>>> generally undefined. If you desire to show just the >>>>>> device_features alone without any config space field, which the >>>>>> device had advertised *before feature negotiation is complete*, >>>>>> that'll be fine. But looks to me this is not how patch has been >>>>>> implemented. Probably need some more work? >>>>> They are driver_features(negotiated) and the device_features(which >>>>> comes with the device), and the config space fields that depend on >>>>> them. In this series, we report both to the userspace. >>>> I fail to understand what you want to present from your >>>> description. May be worth showing some example outputs that at >>>> least include the following cases: 1) when device offers features >>>> but not yet acknowledge by guest 2) when guest acknowledged >>>> features and device is yet to accept 3) after guest feature >>>> negotiation is completed (agreed upon between guest and device). >>> Only two feature sets: 1) what the device has. (2) what is negotiated >>>> >>>> Thanks, >>>> -Siwei >>>>>> >>>>>> Regards, >>>>>> -Siwei >>>>>> >>>>>>>> >>>>>>>> Having said, please consider to drop the Fixes tag, as appears >>>>>>>> to me you're proposing a new feature rather than fixing a real >>>>>>>> issue. >>>>>>> it's a new feature to report the device feature bits than only >>>>>>> negotiated features, however this patch is a must, or it will >>>>>>> block the device feature bits reporting. but I agree, the fix >>>>>>> tag is not a must. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> -Siwei >>>>>>>> >>>>>>>> On 7/1/2022 3:12 PM, Parav Pandit via Virtualization wrote: >>>>>>>>>> From: Zhu Lingshan<lingshan.zhu@...el.com> >>>>>>>>>> Sent: Friday, July 1, 2022 9:28 AM >>>>>>>>>> >>>>>>>>>> Users may want to query the config space of a vDPA device, to >>>>>>>>>> choose a >>>>>>>>>> appropriate one for a certain guest. This means the users >>>>>>>>>> need to read the >>>>>>>>>> config space before FEATURES_OK, and the existence of config >>>>>>>>>> space >>>>>>>>>> contents does not depend on FEATURES_OK. >>>>>>>>>> >>>>>>>>>> The spec says: >>>>>>>>>> The device MUST allow reading of any device-specific >>>>>>>>>> configuration field >>>>>>>>>> before FEATURES_OK is set by the driver. This includes fields >>>>>>>>>> which are >>>>>>>>>> conditional on feature bits, as long as those feature bits >>>>>>>>>> are offered by the >>>>>>>>>> device. >>>>>>>>>> >>>>>>>>>> Fixes: 30ef7a8ac8a07 (vdpa: Read device configuration only if >>>>>>>>>> FEATURES_OK) >>>>>>>>> Fix is fine, but fixes tag needs correction described below. >>>>>>>>> >>>>>>>>> Above commit id is 13 letters should be 12. >>>>>>>>> And >>>>>>>>> It should be in format >>>>>>>>> Fixes: 30ef7a8ac8a0 ("vdpa: Read device configuration only if >>>>>>>>> FEATURES_OK") >>>>>>>>> >>>>>>>>> Please use checkpatch.pl script before posting the patches to >>>>>>>>> catch these errors. >>>>>>>>> There is a bot that looks at the fixes tag and identifies the >>>>>>>>> right kernel version to apply this fix. >>>>>>>>> >>>>>>>>>> Signed-off-by: Zhu Lingshan<lingshan.zhu@...el.com> >>>>>>>>>> --- >>>>>>>>>> drivers/vdpa/vdpa.c | 8 -------- >>>>>>>>>> 1 file changed, 8 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index >>>>>>>>>> 9b0e39b2f022..d76b22b2f7ae 100644 >>>>>>>>>> --- a/drivers/vdpa/vdpa.c >>>>>>>>>> +++ b/drivers/vdpa/vdpa.c >>>>>>>>>> @@ -851,17 +851,9 @@ vdpa_dev_config_fill(struct vdpa_device >>>>>>>>>> *vdev, >>>>>>>>>> struct sk_buff *msg, u32 portid, { >>>>>>>>>> u32 device_id; >>>>>>>>>> void *hdr; >>>>>>>>>> - u8 status; >>>>>>>>>> int err; >>>>>>>>>> >>>>>>>>>> down_read(&vdev->cf_lock); >>>>>>>>>> - status = vdev->config->get_status(vdev); >>>>>>>>>> - if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) { >>>>>>>>>> - NL_SET_ERR_MSG_MOD(extack, "Features negotiation not >>>>>>>>>> completed"); >>>>>>>>>> - err = -EAGAIN; >>>>>>>>>> - goto out; >>>>>>>>>> - } >>>>>>>>>> - >>>>>>>>>> hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, >>>>>>>>>> flags, >>>>>>>>>> VDPA_CMD_DEV_CONFIG_GET); >>>>>>>>>> if (!hdr) { >>>>>>>>>> -- >>>>>>>>>> 2.31.1 >>>>>>>>> _______________________________________________ >>>>>>>>> Virtualization mailing list >>>>>>>>> Virtualization@...ts.linux-foundation.org >>>>>>>>> https://urldefense.com/v3/__https://lists.linuxfoundation.org/mailman/listinfo/virtualization__;!!ACWV5N9M2RV99hQ!NzOv5Ew_Z2CP-zHyD7RsUoStLZ54KpB21QyuZ8L63YVPLEGDEwvcOSDlIGxQPHY-DMkOa9sKKZdBSaNknMU$ >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
Powered by blists - more mailing lists