Re: [PATCH net] vhost_net: remove VHOST_NET_F_VIRTIO_NET_HDR support

[Michael S. Tsirkin]

On Mon, Jun 11, 2018 at 10:29:36AM +0800, Jason Wang wrote:
> 
> 
> 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

Let's document this, yes. I don't think we should drop
VHOST_NET_F_VIRTIO_NET_HDR now.

> > 
> > > > > Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
> > > > > ---
> > > > >    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