Linux Virtualization - Jun 2018 - [PATCH net] vhost_net: remove VHOST_NET_F_VIRTIO_NET_HDR support

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.
>
>
>
>> 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?
>
>> 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.
>
>> 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
>
>> Signed-off-by: Jason Wang <jasowang at 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

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.


> > 
> > 
> > 
> > > 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.
> > 
> > > 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.
> > 
> > > Signed-off-by: Jason Wang <jasowang at 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

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 at 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