Re: [PATCH 2/3] vdpa/mlx5: set_features should nack MQ if no CTRL_VQ

["Michael S. Tsirkin" <mst@redhat.com>]

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@oracle.com>
> > > ---
> > >   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@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization