Messages in this thread | | | Subject | Re: [PATCH net] vhost_net: remove VHOST_NET_F_VIRTIO_NET_HDR support | From | Jason Wang <> | Date | Mon, 11 Jun 2018 10:29:36 +0800 |
| |
On 2018年06月11日 10:12, Michael S. Tsirkin wrote: > On Fri, Jun 08, 2018 at 01:07:09PM +0800, Jason Wang wrote: >> >> On 2018年06月08日 12:46, Michael S. Tsirkin wrote: >>> On Fri, Jun 08, 2018 at 11:50:42AM +0800, Jason Wang wrote: >>>> This feature bit is duplicated with VIRTIO_F_ANY_LAYOUT, this means if >>>> a userpsace want to enable VRITIO_F_ANY_LAYOUT, >>>> VHOST_NET_F_VIRTIO_NET_HDR will be implied too. This is wrong and will >>>> break networking. >>> What breaks networking exactly? VHOST_NET supported ANY_LAYOUT >>> from day one. For this reason it does not need to know about >>> VRITIO_F_ANY_LAYOUT and we reused the bit for other purposes. >> It's the knowledge of vhost_net code it self but not userspace. For >> userspace, it should depends on the value of returned by VHOST_GET_FEATURES. >> So when userspace can set_features with ANY_LAYOUT, vhost may think it wants >> VHOST_NET_F_VIRTIO_NET_HDR. > Yes but that's the admittedly ugly API that we have now. > userspace is supposed to know VRITIO_F_ANY_LAYOUT does > not make sense for vhost.
Ok.
> > > >>> >>> >>>> Fixing this by safely removing >>>> VHOST_NET_F_VIRTIO_NET_HDR support. There should be very few or even >>>> no userspace can use this. >>> Quite possibly, but it is hard to be sure. It seems safer to >>> maintain it unless there's an actual reason something's broken. >> I think not since the feature is negotiated not mandatory? > That doesn't mean much. > >>>> Further cleanups could be done for >>>> -net-next for safety. >>>> >>>> In the future, we need a vhost dedicated feature set/get ioctl() >>>> instead of reusing virtio ones. >>> Not just in the future, we might want to switch iommu >>> to a sane structure without the 64 bit padding bug >>> right now. >> Yes, I hit this bug when introducing V2 of msg IOTLB message. > Sounds good, so if you like, reserve a bit for > VHOST_NET_F_VIRTIO_NET_HDR in the new ioctl mask and > do not enable it there.
Ok, and maybe VHOST_F_LOG_ALL.
> >>>> Fixes: 4e9fa50c6ccbe ("vhost: move features to core") >>> This tag makes no sense here IMHO. Looks like people are using some tool >>> that just looks at the earliest version where patch won't apply. The >>> commit in question just moved some code around. >> Looks not, before this commit, vhost_net won't return ANY_LAYOUT. >> >> Thanks > Well ANY_LAYOUT just happens to be same as VHOST_NET_F_VIRTIO_NET_HDR > and that has been set since forever.
So do you still want this patch? If not we need to document that ANY_LAYOUT could not be passed through SET_FEATURES somewhere.
Thanks
> >>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>>> --- >>>> drivers/vhost/net.c | 15 +++++---------- >>>> 1 file changed, 5 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c >>>> index 986058a..83eef52 100644 >>>> --- a/drivers/vhost/net.c >>>> +++ b/drivers/vhost/net.c >>>> @@ -69,7 +69,6 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;" >>>> enum { >>>> VHOST_NET_FEATURES = VHOST_FEATURES | >>>> - (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) | >>>> (1ULL << VIRTIO_NET_F_MRG_RXBUF) | >>>> (1ULL << VIRTIO_F_IOMMU_PLATFORM) >>>> }; >>>> @@ -1255,15 +1254,11 @@ static int vhost_net_set_features(struct vhost_net *n, u64 features) >>>> (1ULL << VIRTIO_F_VERSION_1))) ? >>>> sizeof(struct virtio_net_hdr_mrg_rxbuf) : >>>> sizeof(struct virtio_net_hdr); >>>> - if (features & (1 << VHOST_NET_F_VIRTIO_NET_HDR)) { >>>> - /* vhost provides vnet_hdr */ >>>> - vhost_hlen = hdr_len; >>>> - sock_hlen = 0; >>>> - } else { >>>> - /* socket provides vnet_hdr */ >>>> - vhost_hlen = 0; >>>> - sock_hlen = hdr_len; >>>> - } >>>> + >>>> + /* socket provides vnet_hdr */ >>>> + vhost_hlen = 0; >>>> + sock_hlen = hdr_len; >>>> + >>>> mutex_lock(&n->dev.mutex); >>>> if ((features & (1 << VHOST_F_LOG_ALL)) && >>>> !vhost_log_access_ok(&n->dev)) >>>> -- >>>> 2.7.4
| |