On Fri, Jan 14, 2022 at 12:51:55AM -0800, Si-Wei Liu wrote:
>
>
> On 1/12/2022 10:57 PM, Michael S. Tsirkin wrote:
> > On Thu, Jan 13, 2022 at 12:10:50AM -0500, Si-Wei Liu wrote:
> > > Made corresponding change per spec:
> >
> > > The device MUST NOT offer a feature which requires another feature
> > > which was not offered.
> > Says nothing about the driver though, and you seem to be
> > doing things to driver features?
> Yes, it's about validation for driver features, though the spec doesn't have
> clear way how to deal with this situation. I guess this in reality leaves
> quite some space for the implementation. To step back, in recent days with
> latent spec revision for feature negotiation due to endianness and MTU
> validation, what do we expect device to work if the driver is not compliant
> and comes up with invalid features set? To clear a subset of driver features
> unsupported by the device, such that driver may figure out by reading it
> from device config space later on? Or fail the entire features and have
> driver to re-try a different setting? Do you feel its possible for device to
> clear a subset of invalid or unsupported features sent down by the driver,
> which may allow driver continue to work without having to fail the entire
> feature negotiation?
>
> The current userspace implementation in qemu may filter out invalid features
> from driver by clearing a subset and tailor it to fit what host/device can
> offer. I thought it should be safe to follow the existing practice. That way
> guest driver can get know of the effective features during feature
> negotiation, or after features_ok is set (that's what I call by "re-read" of
> features, sorry if I used the wrong term). Did I miss something? I can
> definitely add more explanation for the motivation, remove the reference to
> spec and delete the Fixes tag to avoid confusions. Do you think this would
> work?
>
> Another option would be just return failure for the set_driver_features()
> call when seeing (MQ && !CTRL_VQ). Simple enough and easy to implement.
> Efficient to indicate which individual feature is failing? Probably not,
> driver has to retry a few times using binary search to know.
>
> > pls explain the motivation. which config are you trying to
> > fix what is current and expected behaviour.
> The current mq code for mlx5_vdpa driver is written in the assumption that
> MQ must come together with CTRL_VQ. I would like to point out that right now
> there's nowhere in the host side even QEMU to guarantee this assumption
> would hold. Were there a malicious driver sending down MQ without CTRL_VQ,
> it would compromise various spots such as is_index_valid() and
> is_ctrl_vq_idx(). This doesn't end up with immediate panic or security
> loophole in the host currently, but still the chance for this being taken
> advantage of is not zero, especially when future code change is involved.
> You can say it's code cleanup, but the added check helps harden the crispy
> assumption and assures peace of mind.
I think that right now the right thing to do is to validate untrusted
input and fail invalid operations.
The spec does say "VIRTIO_NET_F_MQ Requires VIRTIO_NET_F_CTRL_VQ".
If there are existing legacy drivers
violating some rules, then we should consider working around that (and
maybe documenting that in the spec in the legacy section).
> >
> > > Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
> >
> > It's all theoretical right? Fixes really means
> > "if you have commit ABC then you should pick this one up".
> > not really appropriate for theoretical fixes.
> Yeah. This was discovered in code review. Didn't see a real issue. I can
> remove the tag.
>
> -Siwei
> >
> > > Signed-off-by: Si-Wei Liu<si-wei.liu@xxxxxxxxxx>
> > > ---
> > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 16 +++++++++++++---
> > > 1 file changed, 13 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index b53603d..46d4deb 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -1897,11 +1897,21 @@ static u64 mlx5_vdpa_get_device_features(struct vdpa_device *vdev)
> > > return ndev->mvdev.mlx_features;
> > > }
> > > -static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 features)
> > > +static int verify_driver_features(struct mlx5_vdpa_dev *mvdev, u64 *features)
> >
> > Good rename actually but document in commit log with an
> > explanation.
> >
> > > {
> > > - if (!(features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
> > > + /* minimum features to expect */
> > > + if (!(*features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
> > > return -EOPNOTSUPP;
> > > + /* Double check features combination sent down by the driver.
> > > + * NACK invalid feature due to the absence of depended feature.
> > Pls rewrite this to make it grammatical. There's no NACK in spec. What
> > does this do? Fails to set FEATURES_OK?
> >
> > > + * Driver is expected to re-read the negotiated features once
> > > + * return from set_driver_features.
> > once return is ungrammatical. What to say here depends on what
> > you mean by this, so I'm not sure.
> >
> >
> > Here's text from spec:
> >
> > \item\label{itm:General Initialization And Device Operation /
> > Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
> > understood by the OS and driver to the device. During this step the
> > driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.
> >
> > \item\label{itm:General Initialization And Device Operation / Device Initialization / Set FEATURES-OK} Set the FEATURES_OK status bit. The driver MUST NOT accept
> > new feature bits after this step.
> >
> > \item\label{itm:General Initialization And Device Operation / Device Initialization / Re-read FEATURES-OK} Re-read \field{device status} to ensure the FEATURES_OK bit is still
> > set: otherwise, the device does not support our subset of features
> > and the device is unusable.
> >
> > \item\label{itm:General Initialization And Device Operation / Device Initialization / Device-specific Setup} Perform device-specific setup, including discovery of virtqueues for the
> > device, optional per-bus setup, reading and possibly writing the
> > device's virtio configuration space, and population of virtqueues.
> >
> > does not seem to talk about re-reading features.
> > What did I miss?
> >
> >
> > > + */
> >
> > This comment confuses more than it clarifies. I would
> > - quote the spec
> > - explain why does code do what it does specifically for these features
> >
> > > + if ((*features & (BIT_ULL(VIRTIO_NET_F_MQ) | BIT_ULL(VIRTIO_NET_F_CTRL_VQ))) ==
> > > + BIT_ULL(VIRTIO_NET_F_MQ))
> > > + *features &= ~BIT_ULL(VIRTIO_NET_F_MQ);
> > > +
> > > return 0;
> > > }
> > > @@ -1977,7 +1987,7 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> > > print_features(mvdev, features, true);
> > > - err = verify_min_features(mvdev, features);
> > > + err = verify_driver_features(mvdev, &features);
> > > if (err)
> > > return err;
> > > --
> > > 1.8.3.1
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
