[libvirt PATCH v8 0/3] Ignore EPERM on implicit clearing of VF VLAN ID

[Dmitrii Shcherbakov]

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
<dmitrii.shcherbakov at canonical.com> 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 <mprivozn at redhat.com> 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 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
> >