Re: [Virtio-fs] [PATCH] vhost-user-fs: fix features handling

[Stefan Hajnoczi]

On Mon, Apr 12, 2021 at 02:43:16PM -0400, Vivek Goyal wrote:
> On Sun, Apr 11, 2021 at 09:21:54AM +0300, Anton Kuchin wrote:
> > 
> > On 09/04/2021 18:56, Vivek Goyal wrote:
> > > On Thu, Apr 08, 2021 at 10:55:34PM +0300, Anton Kuchin wrote:
> > > > Make virtio-fs take into account server capabilities.
> > > > 
> > > > Just returning requested features assumes they all of then are 
> > > > implemented
> > > > by server and results in setting unsupported configuration if some of 
> > > > them
> > > > are absent.
> > > > 
> > > > Signed-off-by: Anton Kuchin<antonkuchin@yandex-team.ru>
> > > [CC stefan and qemu-devel.]
> > > 
> > > Can you give more details of what problem exactly you are facing. Or
> > > this fix is about avoiding a future problem where device can refuse
> > > to support a feature qemu is requesting for.
> > 
> > This fixes existing problem that qemu ignores features (un)supported by
> > backend and this works fine only if backend features match features of qemu.
> > Otherwise qemu incorrectly assumes that backend suports all of them and
> > calls vhost_set_features() not taking into account result of previous
> > vhost_get_features() call. This breaks protocol and can crash server or
> > cause incorrect behavior.
> > 
> > This is why I hope it to be accepted in time for 6.0 release.
> > 
> > > IIUC, this patch is preparing a list of features vhost-user-fs device
> > > can support. Then it calls vhost_get_features() which makes sure that
> > > all these features are support by real vhost-user device (hdev->features).
> > > If not, then corresponding feature is reset and remaining features
> > > are returned to caller.
> > When this callback is executed in virtio_bus_device_plugged() list of
> > features that vhost-backend supports has been already obtained earlier by
> > vhost_user_get_features() in vuf_device_realize() and stored in
> > hdev->features.
> 
> > vuf_get_features() should return bitmask of features that
> > are common for vhost backend (hdev->features) and frontend
> > (vdev->host_features).
> 
> But that's not what exactly this patch seems to be doing.
> IIUC, It only resets some of the features from list passed from
> the caller. So whatever has been defined in user_feature_bits[],
> and if these features are not supported by vhost-user backend, then
> that feature will be reset before returning to caller.
> 
> So the question is what are those features which should be in
> user_feature_bits[]? For example, by default libvhost-user
> also supports.
> 
>         /* vhost-user feature bits */
>         1ULL << VHOST_F_LOG_ALL |
>         1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> 
> Should that be in user_feature_bits[] too. So that if a customer
> vhost-user-fs backend does not support VHOST_F_LOG_ALL or
> VHOST_USER_F_PROTOCOL_FEATURES, it is reset.
> 
> IIUC, your current patch is not going to reset these features if
> caller passed you those in vuf_get_features(,requested_features).
> 
> So to me this becomes more of a question that what are those common
> features which both the ends of vhost-user device should support for
> it to work and should be checked in vuf_get_features(). 

VHOST_F_LOG_ALL and VHOST_USER_F_PROTOCOL_FEATURES are controlled by
hw/virtio/vhost.c and hw/virtio/vhost-user.c. These feature bits are
part of the vhost-user protocol and are not involved in guest-visible
VIRTIO feature negotiation. It's confusing because these bits use the
same namespace as VIRTIO features but it is correct to omit it from
user_feature_bits[].

Stefan

signature.asc
Description: PGP signature