mbox series

[0/4] Some fixes for vhost zero copy

Message ID 20190222024209.30879-1-tiwei.bie@intel.com (mailing list archive)
Headers
Series Some fixes for vhost zero copy |

Message

Tiwei Bie Feb. 22, 2019, 2:42 a.m. UTC 
  Tiwei Bie (4):
  vhost: restore mbuf first when freeing zmbuf
  vhost: fix potential use-after-free for zero copy mbuf
  vhost: fix potential use-after-free for memory region
  doc: improve vhost zero copy guide

 doc/guides/prog_guide/vhost_lib.rst |  3 +++
 lib/librte_vhost/vhost.h            | 34 +++++++++++++++++++++++
 lib/librte_vhost/vhost_user.c       | 42 ++++++++++++++++++++++-------
 lib/librte_vhost/virtio_net.c       | 34 -----------------------
 4 files changed, 70 insertions(+), 43 deletions(-)

Comments

Maxime Coquelin Feb. 26, 2019, 2:46 p.m. UTC | #1 
On 2/22/19 3:42 AM, Tiwei Bie wrote:
> Tiwei Bie (4):
>    vhost: restore mbuf first when freeing zmbuf
>    vhost: fix potential use-after-free for zero copy mbuf
>    vhost: fix potential use-after-free for memory region
>    doc: improve vhost zero copy guide
> 
>   doc/guides/prog_guide/vhost_lib.rst |  3 +++
>   lib/librte_vhost/vhost.h            | 34 +++++++++++++++++++++++
>   lib/librte_vhost/vhost_user.c       | 42 ++++++++++++++++++++++-------
>   lib/librte_vhost/virtio_net.c       | 34 -----------------------
>   4 files changed, 70 insertions(+), 43 deletions(-)
> 

Looking at the spec, I think we may need also to drain zmbufs in the
VHOST_USER_SET_VRING_ENABLE for the disable case:

""
If VHOST_USER_F_PROTOCOL_FEATURES has been negotiated, the ring is 
initialized
in a disabled state. Client must not pass data to/from the backend until 
ring is enabled by
VHOST_USER_SET_VRING_ENABLE with parameter 1, or after it has been 
disabled by
VHOST_USER_SET_VRING_ENABLE with parameter 0.

Each ring is initialized in a stopped state, client must not process it 
until
ring is started, or *after it has been stopped*.
""

Do you take care of this or I send a patch on top?

Thanks,
Maxime
Tiwei Bie Feb. 27, 2019, 1:52 a.m. UTC | #2 
On Tue, Feb 26, 2019 at 03:46:41PM +0100, Maxime Coquelin wrote:
> On 2/22/19 3:42 AM, Tiwei Bie wrote:
> > Tiwei Bie (4):
> >    vhost: restore mbuf first when freeing zmbuf
> >    vhost: fix potential use-after-free for zero copy mbuf
> >    vhost: fix potential use-after-free for memory region
> >    doc: improve vhost zero copy guide
> > 
> >   doc/guides/prog_guide/vhost_lib.rst |  3 +++
> >   lib/librte_vhost/vhost.h            | 34 +++++++++++++++++++++++
> >   lib/librte_vhost/vhost_user.c       | 42 ++++++++++++++++++++++-------
> >   lib/librte_vhost/virtio_net.c       | 34 -----------------------
> >   4 files changed, 70 insertions(+), 43 deletions(-)
> > 
> 
> Looking at the spec, I think we may need also to drain zmbufs in the
> VHOST_USER_SET_VRING_ENABLE for the disable case:
> 
> ""
> If VHOST_USER_F_PROTOCOL_FEATURES has been negotiated, the ring is
> initialized
> in a disabled state. Client must not pass data to/from the backend until
> ring is enabled by
> VHOST_USER_SET_VRING_ENABLE with parameter 1, or after it has been disabled
> by
> VHOST_USER_SET_VRING_ENABLE with parameter 0.
> 
> Each ring is initialized in a stopped state, client must not process it
> until
> ring is started, or *after it has been stopped*.
> ""
> 
> Do you take care of this or I send a patch on top?

Agree. Please feel free to send any patch on top.

Thanks!
Tiwei

> 
> Thanks,
> Maxime
Maxime Coquelin Feb. 27, 2019, 8:31 a.m. UTC | #3 
On 2/22/19 3:42 AM, Tiwei Bie wrote:
> Tiwei Bie (4):
>    vhost: restore mbuf first when freeing zmbuf
>    vhost: fix potential use-after-free for zero copy mbuf
>    vhost: fix potential use-after-free for memory region
>    doc: improve vhost zero copy guide
> 
>   doc/guides/prog_guide/vhost_lib.rst |  3 +++
>   lib/librte_vhost/vhost.h            | 34 +++++++++++++++++++++++
>   lib/librte_vhost/vhost_user.c       | 42 ++++++++++++++++++++++-------
>   lib/librte_vhost/virtio_net.c       | 34 -----------------------
>   4 files changed, 70 insertions(+), 43 deletions(-)
> 

Applied to dpdk-next-virtio/master.

Thanks,
Maxime
Maxime Coquelin Feb. 27, 2019, 8:32 a.m. UTC | #4 
On 2/27/19 2:52 AM, Tiwei Bie wrote:
> On Tue, Feb 26, 2019 at 03:46:41PM +0100, Maxime Coquelin wrote:
>> On 2/22/19 3:42 AM, Tiwei Bie wrote:
>>> Tiwei Bie (4):
>>>     vhost: restore mbuf first when freeing zmbuf
>>>     vhost: fix potential use-after-free for zero copy mbuf
>>>     vhost: fix potential use-after-free for memory region
>>>     doc: improve vhost zero copy guide
>>>
>>>    doc/guides/prog_guide/vhost_lib.rst |  3 +++
>>>    lib/librte_vhost/vhost.h            | 34 +++++++++++++++++++++++
>>>    lib/librte_vhost/vhost_user.c       | 42 ++++++++++++++++++++++-------
>>>    lib/librte_vhost/virtio_net.c       | 34 -----------------------
>>>    4 files changed, 70 insertions(+), 43 deletions(-)
>>>
>>
>> Looking at the spec, I think we may need also to drain zmbufs in the
>> VHOST_USER_SET_VRING_ENABLE for the disable case:
>>
>> ""
>> If VHOST_USER_F_PROTOCOL_FEATURES has been negotiated, the ring is
>> initialized
>> in a disabled state. Client must not pass data to/from the backend until
>> ring is enabled by
>> VHOST_USER_SET_VRING_ENABLE with parameter 1, or after it has been disabled
>> by
>> VHOST_USER_SET_VRING_ENABLE with parameter 0.
>>
>> Each ring is initialized in a stopped state, client must not process it
>> until
>> ring is started, or *after it has been stopped*.
>> ""
>>
>> Do you take care of this or I send a patch on top?
> 
> Agree. Please feel free to send any patch on top.

Good, I'll do the patch now.

Maxime

> Thanks!
> Tiwei
> 
>>
>> Thanks,
>> Maxime