Re: [libvirt PATCH v8 0/3] Ignore EPERM on implicit clearing of VF VLAN ID
On 2/3/22 10:52 AM, Michal Prívozník wrote: On 2/3/22 15:45, Laine Stump wrote: On 2/2/22 1:04 PM, Michal Prívozník wrote: Laine, any thoughts? I somehow thought this had already been pushed, so I was surprised when it showed up again :-/ I think the only issue I had before was that I thought the new unit tests were more testing the test setup than the actual code, but Dan convinced me otherwise. So I'm fine with this change. Cool, pushed now. A newly discovered (but pre-existing, so non-consequential to these patches) problem associated with vlans is that we don't differentiate between "set vlan 0" and "don't set any vlan", which I hadn't even considered before, until this BZ came up: https://bugzilla.redhat.com/2035726 (The reporter is trying to use to "unset" the vlan tag for an interface) Ah, but the bug report is for openvswitch port profile, so that's doubly unrelated :-) Yes and now. The original report was about openvswitch ports, but then the reporter also tried things with hostdev SRIOV devices and macvtap passthrough devices (with varying results). I'm sure that whatever is done to fix it will be deeply intertwined and complicated by the current topic (since the whole point of these patches is that smartnics want/need to just leave the entire vlan tag thing alone) :-) Additionally, Dmitrii's change might make it possible to easily/simply fix the case of updating vlan tag for existing macvtap passthrough and hostdev SRIOV VFs (since we can now set the vlan tag without doing anything else). So for once there is an unintended *good* side effect!
Re: [libvirt PATCH v8 0/3] Ignore EPERM on implicit clearing of VF VLAN ID
Thanks again for the feedback - much appreciated! Best Regards, Dmitrii Shcherbakov LP/MM/oftc: dmitriis On Thu, Feb 3, 2022 at 6:52 PM Michal Prívozník wrote: > > On 2/3/22 15:45, Laine Stump wrote: > > On 2/2/22 1:04 PM, Michal Prívozník wrote: > >> > >> Laine, any thoughts? > > > > I somehow thought this had already been pushed, so I was surprised when > > it showed up again :-/ > > > > I think the only issue I had before was that I thought the new unit > > tests were more testing the test setup than the actual code, but Dan > > convinced me otherwise. > > > > So I'm fine with this change. > > > > Cool, pushed now. > > > A newly discovered (but pre-existing, so non-consequential to these > > patches) problem associated with vlans is that we don't differentiate > > between "set vlan 0" and "don't set any vlan", which I hadn't even > > considered before, until this BZ came up: > > > > https://bugzilla.redhat.com/2035726 > > > > (The reporter is trying to use to "unset" the vlan tag > > for an interface) > > > > Ah, but the bug report is for openvswitch port profile, so that's doubly > unrelated :-) > > Michal >
Re: [libvirt PATCH v8 0/3] Ignore EPERM on implicit clearing of VF VLAN ID
On 2/3/22 15:45, Laine Stump wrote: > On 2/2/22 1:04 PM, Michal Prívozník wrote: >> >> Laine, any thoughts? > > I somehow thought this had already been pushed, so I was surprised when > it showed up again :-/ > > I think the only issue I had before was that I thought the new unit > tests were more testing the test setup than the actual code, but Dan > convinced me otherwise. > > So I'm fine with this change. > Cool, pushed now. > A newly discovered (but pre-existing, so non-consequential to these > patches) problem associated with vlans is that we don't differentiate > between "set vlan 0" and "don't set any vlan", which I hadn't even > considered before, until this BZ came up: > > https://bugzilla.redhat.com/2035726 > > (The reporter is trying to use to "unset" the vlan tag > for an interface) > Ah, but the bug report is for openvswitch port profile, so that's doubly unrelated :-) Michal
Re: [libvirt PATCH v8 0/3] Ignore EPERM on implicit clearing of VF VLAN ID
On 2/2/22 1:04 PM, Michal Prívozník wrote: Laine, any thoughts? I somehow thought this had already been pushed, so I was surprised when it showed up again :-/ I think the only issue I had before was that I thought the new unit tests were more testing the test setup than the actual code, but Dan convinced me otherwise. So I'm fine with this change. A newly discovered (but pre-existing, so non-consequential to these patches) problem associated with vlans is that we don't differentiate between "set vlan 0" and "don't set any vlan", which I hadn't even considered before, until this BZ came up: https://bugzilla.redhat.com/2035726 (The reporter is trying to use to "unset" the vlan tag for an interface)
Re: [libvirt PATCH v8 0/3] Ignore EPERM on implicit clearing of VF VLAN ID
Squashed & auto-tested here: https://gitlab.com/dmitriis/libvirt/-/commits/2021-ignore-eperm-for-vid0 https://gitlab.com/dmitriis/libvirt/-/pipelines/462532823 Best Regards, Dmitrii Shcherbakov LP/MM/oftc: dmitriis On Wed, Feb 2, 2022 at 11:10 PM Dmitrii Shcherbakov wrote: > > Hi Michal, > > No problem, thanks for coming back to re-review it, I also acknowledge > that it was late in the year and during the holiday season so things > got slower. > > > and if you agree, I'd squash them in respective commits and merge. > > I looked at the fixup commits - I agree with the changes and with > squashing them so it's a +1 from me, thanks a lot for doing this! > > Best Regards, > Dmitrii Shcherbakov > LP/MM/oftc: dmitriis > > > On Wed, Feb 2, 2022 at 9:04 PM Michal Prívozník wrote: > > > > On 2/1/22 09:28, Dmitrii Shcherbakov wrote: > > > SmartNIC DPUs may not expose some privileged eswitch operations > > > to the hypervisor hosts. For example, this happens with Bluefield > > > devices running in the ECPF (default) mode [1] for security reasons. While > > > VF MAC address programming is possible via an RTM_SETLINK operation, > > > trying to set a VLAN ID in the same operation may fail with EPERM. > > > > > > Specifically for the mlx5 driver this behavior was altered in the Linux > > > kernel upstream [2] to avoid getting EPERM when trying to program VLAN 0. > > > This may also get backported to older downstream kernels (e.g. [3]). > > > However, Libvirt could potentially handle this case gracefully without > > > needing a specific kernel version or depending on a specific driver fix. > > > > > > In the kernel a relevant call chain may look like > > > > > > do_setlink -> do_setvfinfo -> dev->netdev_ops->set_vf_vlan > > > > > > which calls a driver-specific function like [4] eventually. > > > > > > The equivalent ip link commands below provide an illustration: > > > > > > 1. This will work without an error: > > > > > > sudo ip link set enp130s0f0 vf 2 mac de:ad:be:ef:ca:fe > > > > > > 2. Setting (or clearing) a VLAN will fail with EPERM: > > > > > > sudo ip link set enp130s0f0 vf 2 vlan 0 > > > RTNETLINK answers: Operation not permitted > > > > > > 3. This is what Libvirt attempts to do today when trying to clear a > > >VF VLAN at the same time as programming a VF MAC: > > > > > > sudo ip link set enp130s0f0 vf 2 vlan 0 mac de:ad:be:ef:ca:fe > > > RTNETLINK answers: Operation not permitted > > > > > > If setting an explicit VLAN ID results in an EPERM, clearing a VLAN > > > (setting a VLAN ID to 0) can be handled gracefully by ignoring the > > > EPERM error with the rationale being that if we cannot set this state > > > in the first place, we cannot clear it either. > > > > > > Thus, virNetDevSetVfConfig is split into two distinct functions. If > > > clearing a VLAN ID fails with EPERM when clearing is implicit, the > > > error is simply ignored. For explicit clearing EPERM is still a > > > fatal error. > > > > > > Both new functions rely virNetDevSendVfSetLinkRequest that implements > > > common functionality related to formatting a request, sending it and > > > handling error conditions and returns 0 or an error since in both cases > > > the payload is either NLMSG_DONE (no error) or NLMSG_ERROR where an > > > error message is needed by the caller to handle known cases > > > appropriately. This function allows the conditional code to be unit > > > tested. > > > > > > An alternative to this could be providing a higher level control plane > > > mechanism that would provide metadata about a device being remotely > > > managed in which case Libvirt would avoid trying to set or clear a > > > VLAN ID. This would be more complicated since other software (like Nova > > > in the OpenStack case) would have to annotate every guest device with an > > > attribute indicating whether a device is remotely managed or not based > > > on operator provided configuration so that Libvirt can act on this and > > > avoid VLAN programming. > > > > > > https://gitlab.com/dmitriis/libvirt/-/pipelines/460293963 > > > > > > v8 change: > > > > > > * Rebased on top of the latest changes to Libvirt; > > > * Added relevant upstream Linux and downstream kernel references to > > > this cover letter. > > > > > > [1] > > > https://docs.mellanox.com/display/BlueFieldSWv35111601/Modes+of+Operation#ModesofOperation-SmartNICmode > > > [2] > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7846665d3504812acaebf920d1141851379a7f37 > > > [3] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1957753 > > > [4] > > > https://github.com/torvalds/linux/blob/v5.15/drivers/net/ethernet/mellanox/mlx5/core/esw/legacy.c#L427-L434 > > > > > > Dmitrii Shcherbakov (3): > > > Set VF MAC and VLAN ID in two different operations > > > Allow VF vlanid to be passed as a pointer > > > Ignore EPERM on implicit clearing of VF VLAN ID > > > > > > NEWS.rst| 14 ++ > > > src/hypervisor/virhostdev.c |
Re: [libvirt PATCH v8 0/3] Ignore EPERM on implicit clearing of VF VLAN ID
Hi Michal, No problem, thanks for coming back to re-review it, I also acknowledge that it was late in the year and during the holiday season so things got slower. > and if you agree, I'd squash them in respective commits and merge. I looked at the fixup commits - I agree with the changes and with squashing them so it's a +1 from me, thanks a lot for doing this! Best Regards, Dmitrii Shcherbakov LP/MM/oftc: dmitriis On Wed, Feb 2, 2022 at 9:04 PM Michal Prívozník wrote: > > On 2/1/22 09:28, Dmitrii Shcherbakov wrote: > > SmartNIC DPUs may not expose some privileged eswitch operations > > to the hypervisor hosts. For example, this happens with Bluefield > > devices running in the ECPF (default) mode [1] for security reasons. While > > VF MAC address programming is possible via an RTM_SETLINK operation, > > trying to set a VLAN ID in the same operation may fail with EPERM. > > > > Specifically for the mlx5 driver this behavior was altered in the Linux > > kernel upstream [2] to avoid getting EPERM when trying to program VLAN 0. > > This may also get backported to older downstream kernels (e.g. [3]). > > However, Libvirt could potentially handle this case gracefully without > > needing a specific kernel version or depending on a specific driver fix. > > > > In the kernel a relevant call chain may look like > > > > do_setlink -> do_setvfinfo -> dev->netdev_ops->set_vf_vlan > > > > which calls a driver-specific function like [4] eventually. > > > > The equivalent ip link commands below provide an illustration: > > > > 1. This will work without an error: > > > > sudo ip link set enp130s0f0 vf 2 mac de:ad:be:ef:ca:fe > > > > 2. Setting (or clearing) a VLAN will fail with EPERM: > > > > sudo ip link set enp130s0f0 vf 2 vlan 0 > > RTNETLINK answers: Operation not permitted > > > > 3. This is what Libvirt attempts to do today when trying to clear a > >VF VLAN at the same time as programming a VF MAC: > > > > sudo ip link set enp130s0f0 vf 2 vlan 0 mac de:ad:be:ef:ca:fe > > RTNETLINK answers: Operation not permitted > > > > If setting an explicit VLAN ID results in an EPERM, clearing a VLAN > > (setting a VLAN ID to 0) can be handled gracefully by ignoring the > > EPERM error with the rationale being that if we cannot set this state > > in the first place, we cannot clear it either. > > > > Thus, virNetDevSetVfConfig is split into two distinct functions. If > > clearing a VLAN ID fails with EPERM when clearing is implicit, the > > error is simply ignored. For explicit clearing EPERM is still a > > fatal error. > > > > Both new functions rely virNetDevSendVfSetLinkRequest that implements > > common functionality related to formatting a request, sending it and > > handling error conditions and returns 0 or an error since in both cases > > the payload is either NLMSG_DONE (no error) or NLMSG_ERROR where an > > error message is needed by the caller to handle known cases > > appropriately. This function allows the conditional code to be unit tested. > > > > An alternative to this could be providing a higher level control plane > > mechanism that would provide metadata about a device being remotely > > managed in which case Libvirt would avoid trying to set or clear a > > VLAN ID. This would be more complicated since other software (like Nova > > in the OpenStack case) would have to annotate every guest device with an > > attribute indicating whether a device is remotely managed or not based > > on operator provided configuration so that Libvirt can act on this and > > avoid VLAN programming. > > > > https://gitlab.com/dmitriis/libvirt/-/pipelines/460293963 > > > > v8 change: > > > > * Rebased on top of the latest changes to Libvirt; > > * Added relevant upstream Linux and downstream kernel references to > > this cover letter. > > > > [1] > > https://docs.mellanox.com/display/BlueFieldSWv35111601/Modes+of+Operation#ModesofOperation-SmartNICmode > > [2] > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7846665d3504812acaebf920d1141851379a7f37 > > [3] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1957753 > > [4] > > https://github.com/torvalds/linux/blob/v5.15/drivers/net/ethernet/mellanox/mlx5/core/esw/legacy.c#L427-L434 > > > > Dmitrii Shcherbakov (3): > > Set VF MAC and VLAN ID in two different operations > > Allow VF vlanid to be passed as a pointer > > Ignore EPERM on implicit clearing of VF VLAN ID > > > > NEWS.rst| 14 ++ > > src/hypervisor/virhostdev.c | 4 +- > > src/libvirt_private.syms| 7 + > > src/util/virnetdev.c| 256 +--- > > src/util/virnetdevpriv.h| 44 +++ > > tests/virnetdevtest.c | 249 ++- > > 6 files changed, 496 insertions(+), 78 deletions(-) > > create mode 100644 src/util/virnetdevpriv.h > > > > Sorry for allowing this go to v8 without proper review. To make it up to > you, here's my suggestion: problems I've raised in my
Re: [libvirt PATCH v8 0/3] Ignore EPERM on implicit clearing of VF VLAN ID
On 2/1/22 09:28, Dmitrii Shcherbakov wrote: > SmartNIC DPUs may not expose some privileged eswitch operations > to the hypervisor hosts. For example, this happens with Bluefield > devices running in the ECPF (default) mode [1] for security reasons. While > VF MAC address programming is possible via an RTM_SETLINK operation, > trying to set a VLAN ID in the same operation may fail with EPERM. > > Specifically for the mlx5 driver this behavior was altered in the Linux > kernel upstream [2] to avoid getting EPERM when trying to program VLAN 0. > This may also get backported to older downstream kernels (e.g. [3]). > However, Libvirt could potentially handle this case gracefully without > needing a specific kernel version or depending on a specific driver fix. > > In the kernel a relevant call chain may look like > > do_setlink -> do_setvfinfo -> dev->netdev_ops->set_vf_vlan > > which calls a driver-specific function like [4] eventually. > > The equivalent ip link commands below provide an illustration: > > 1. This will work without an error: > > sudo ip link set enp130s0f0 vf 2 mac de:ad:be:ef:ca:fe > > 2. Setting (or clearing) a VLAN will fail with EPERM: > > sudo ip link set enp130s0f0 vf 2 vlan 0 > RTNETLINK answers: Operation not permitted > > 3. This is what Libvirt attempts to do today when trying to clear a >VF VLAN at the same time as programming a VF MAC: > > sudo ip link set enp130s0f0 vf 2 vlan 0 mac de:ad:be:ef:ca:fe > RTNETLINK answers: Operation not permitted > > If setting an explicit VLAN ID results in an EPERM, clearing a VLAN > (setting a VLAN ID to 0) can be handled gracefully by ignoring the > EPERM error with the rationale being that if we cannot set this state > in the first place, we cannot clear it either. > > Thus, virNetDevSetVfConfig is split into two distinct functions. If > clearing a VLAN ID fails with EPERM when clearing is implicit, the > error is simply ignored. For explicit clearing EPERM is still a > fatal error. > > Both new functions rely virNetDevSendVfSetLinkRequest that implements > common functionality related to formatting a request, sending it and > handling error conditions and returns 0 or an error since in both cases > the payload is either NLMSG_DONE (no error) or NLMSG_ERROR where an > error message is needed by the caller to handle known cases > appropriately. This function allows the conditional code to be unit tested. > > An alternative to this could be providing a higher level control plane > mechanism that would provide metadata about a device being remotely > managed in which case Libvirt would avoid trying to set or clear a > VLAN ID. This would be more complicated since other software (like Nova > in the OpenStack case) would have to annotate every guest device with an > attribute indicating whether a device is remotely managed or not based > on operator provided configuration so that Libvirt can act on this and > avoid VLAN programming. > > https://gitlab.com/dmitriis/libvirt/-/pipelines/460293963 > > v8 change: > > * Rebased on top of the latest changes to Libvirt; > * Added relevant upstream Linux and downstream kernel references to > this cover letter. > > [1] > https://docs.mellanox.com/display/BlueFieldSWv35111601/Modes+of+Operation#ModesofOperation-SmartNICmode > [2] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7846665d3504812acaebf920d1141851379a7f37 > [3] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1957753 > [4] > https://github.com/torvalds/linux/blob/v5.15/drivers/net/ethernet/mellanox/mlx5/core/esw/legacy.c#L427-L434 > > Dmitrii Shcherbakov (3): > Set VF MAC and VLAN ID in two different operations > Allow VF vlanid to be passed as a pointer > Ignore EPERM on implicit clearing of VF VLAN ID > > NEWS.rst| 14 ++ > src/hypervisor/virhostdev.c | 4 +- > src/libvirt_private.syms| 7 + > src/util/virnetdev.c| 256 +--- > src/util/virnetdevpriv.h| 44 +++ > tests/virnetdevtest.c | 249 ++- > 6 files changed, 496 insertions(+), 78 deletions(-) > create mode 100644 src/util/virnetdevpriv.h > Sorry for allowing this go to v8 without proper review. To make it up to you, here's my suggestion: problems I've raised in my review are easy to solve. I've uploaded the 'fixup' commits onto my gilab here: https://gitlab.com/MichalPrivoznik/libvirt/-/commits/vf_vlan_id/ and if you agree, I'd squash them in respective commits and merge. Laine, any thoughts? Michal