| Message ID | 1528429842-22835-1-git-send-email-jasowang@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > 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. > 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. > > 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. > 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
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@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
From: Jason Wang <jasowang@redhat.com> Date: Fri, 8 Jun 2018 11:50:42 +0800 > 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. Fixing this by safely removing > VHOST_NET_F_VIRTIO_NET_HDR support. There should be very few or even > no userspace can use this. 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. > > Fixes: 4e9fa50c6ccbe ("vhost: move features to core") > Signed-off-by: Jason Wang <jasowang@redhat.com> I don't see this discussion as resolved yet so I'll mark this as Deferred in patchwork. Thanks.
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@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@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 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@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
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))
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. Fixing this by safely removing VHOST_NET_F_VIRTIO_NET_HDR support. There should be very few or even no userspace can use this. 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. Fixes: 4e9fa50c6ccbe ("vhost: move features to core") Signed-off-by: Jason Wang <jasowang@redhat.com> --- drivers/vhost/net.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)