[dpdk-dev] vhost/crypto: fix missing head correction

Message ID 20180427135233.56019-1-roy.fan.zhang@intel.com (mailing list archive)
State Accepted, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Fan Zhang April 27, 2018, 1:52 p.m. UTC
  Fixes: 473387aaae4a ("vhost/crypto: move to safe GPA translation API")

This patch fixes the missing head descriptor correction for
indirect descriptors.

Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
---
 lib/librte_vhost/vhost_crypto.c | 1 +
 1 file changed, 1 insertion(+)
  

Comments

Maxime Coquelin April 27, 2018, 2:19 p.m. UTC | #1
Hi Fan,

On 04/27/2018 03:52 PM, Fan Zhang wrote:
> Fixes: 473387aaae4a ("vhost/crypto: move to safe GPA translation API")
> 
> This patch fixes the missing head descriptor correction for
> indirect descriptors.
> 
> Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
> ---
>   lib/librte_vhost/vhost_crypto.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_vhost/vhost_crypto.c b/lib/librte_vhost/vhost_crypto.c
> index c38eb3bb5..81fab7e94 100644
> --- a/lib/librte_vhost/vhost_crypto.c
> +++ b/lib/librte_vhost/vhost_crypto.c
> @@ -919,6 +919,7 @@ vhost_crypto_process_one_req(struct vhost_crypto *vcrypto,
>   		if (unlikely(!desc || dlen != head->len))
>   			return -1;
>   		desc_idx = 0;
> +		head = desc;

Thanks for fixing this, and sorry for having introduced the regression.

BTW, my patch is half-complete. Indeed, it ensures no out-of-bound
accesses will be done, but it does not handle legit buffers that are
contiguous in guest physical address space, but discontiguous in the
host VA space. Such case can for example happen when having buffers
larger than the huge page size.

I have implemented the handling of such buffers in virtio-net.c, for 
example:

fb3815cc6 vhost: handle virtually non-contiguous buffers in Rx-mrg
6727f5a73 vhost: handle virtually non-contiguous buffers in Rx
91b7b4080 vhost: handle virtually non-contiguous buffers in Tx
d0c24508e vhost: add support for non-contiguous indirect descs tables

Can you try to work on such handling for next -rc release?


For this patch:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

>   	} else {
>   		desc = head;
>   	}
>
  
Fan Zhang April 27, 2018, 2:22 p.m. UTC | #2
Hi Maxime,

No problem. I will work on it.

Regards,
Fan

> -----Original Message-----

> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]

> Sent: Friday, April 27, 2018 3:19 PM

> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; dev@dpdk.org

> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>

> Subject: Re: [PATCH] vhost/crypto: fix missing head correction

> 

> Hi Fan,

> 

> On 04/27/2018 03:52 PM, Fan Zhang wrote:

> > Fixes: 473387aaae4a ("vhost/crypto: move to safe GPA translation API")

> >

> > This patch fixes the missing head descriptor correction for indirect

> > descriptors.

> >

> > Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>

> > ---

> >   lib/librte_vhost/vhost_crypto.c | 1 +

> >   1 file changed, 1 insertion(+)

> >

> > diff --git a/lib/librte_vhost/vhost_crypto.c

> > b/lib/librte_vhost/vhost_crypto.c index c38eb3bb5..81fab7e94 100644

> > --- a/lib/librte_vhost/vhost_crypto.c

> > +++ b/lib/librte_vhost/vhost_crypto.c

> > @@ -919,6 +919,7 @@ vhost_crypto_process_one_req(struct

> vhost_crypto *vcrypto,

> >   		if (unlikely(!desc || dlen != head->len))

> >   			return -1;

> >   		desc_idx = 0;

> > +		head = desc;

> 

> Thanks for fixing this, and sorry for having introduced the regression.

> 

> BTW, my patch is half-complete. Indeed, it ensures no out-of-bound

> accesses will be done, but it does not handle legit buffers that are contiguous

> in guest physical address space, but discontiguous in the host VA space. Such

> case can for example happen when having buffers larger than the huge page

> size.

> 

> I have implemented the handling of such buffers in virtio-net.c, for

> example:

> 

> fb3815cc6 vhost: handle virtually non-contiguous buffers in Rx-mrg

> 6727f5a73 vhost: handle virtually non-contiguous buffers in Rx

> 91b7b4080 vhost: handle virtually non-contiguous buffers in Tx d0c24508e

> vhost: add support for non-contiguous indirect descs tables

> 

> Can you try to work on such handling for next -rc release?

> 

> 

> For this patch:

> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

> 

> Thanks,

> Maxime

> 

> >   	} else {

> >   		desc = head;

> >   	}

> >
  
Thomas Monjalon April 27, 2018, 5:38 p.m. UTC | #3
27/04/2018 16:19, Maxime Coquelin:
> Hi Fan,
> 
> On 04/27/2018 03:52 PM, Fan Zhang wrote:
> > Fixes: 473387aaae4a ("vhost/crypto: move to safe GPA translation API")
> > 
> > This patch fixes the missing head descriptor correction for
> > indirect descriptors.
> > 
> > Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
> > ---
> >   lib/librte_vhost/vhost_crypto.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/lib/librte_vhost/vhost_crypto.c b/lib/librte_vhost/vhost_crypto.c
> > index c38eb3bb5..81fab7e94 100644
> > --- a/lib/librte_vhost/vhost_crypto.c
> > +++ b/lib/librte_vhost/vhost_crypto.c
> > @@ -919,6 +919,7 @@ vhost_crypto_process_one_req(struct vhost_crypto *vcrypto,
> >   		if (unlikely(!desc || dlen != head->len))
> >   			return -1;
> >   		desc_idx = 0;
> > +		head = desc;
> 
> Thanks for fixing this, and sorry for having introduced the regression.
> 
> BTW, my patch is half-complete. Indeed, it ensures no out-of-bound
> accesses will be done, but it does not handle legit buffers that are
> contiguous in guest physical address space, but discontiguous in the
> host VA space. Such case can for example happen when having buffers
> larger than the huge page size.
> 
> I have implemented the handling of such buffers in virtio-net.c, for 
> example:
> 
> fb3815cc6 vhost: handle virtually non-contiguous buffers in Rx-mrg
> 6727f5a73 vhost: handle virtually non-contiguous buffers in Rx
> 91b7b4080 vhost: handle virtually non-contiguous buffers in Tx
> d0c24508e vhost: add support for non-contiguous indirect descs tables
> 
> Can you try to work on such handling for next -rc release?
> 
> 
> For this patch:
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Applied as hot patch for RC1, thanks
  

Patch

diff --git a/lib/librte_vhost/vhost_crypto.c b/lib/librte_vhost/vhost_crypto.c
index c38eb3bb5..81fab7e94 100644
--- a/lib/librte_vhost/vhost_crypto.c
+++ b/lib/librte_vhost/vhost_crypto.c
@@ -919,6 +919,7 @@  vhost_crypto_process_one_req(struct vhost_crypto *vcrypto,
 		if (unlikely(!desc || dlen != head->len))
 			return -1;
 		desc_idx = 0;
+		head = desc;
 	} else {
 		desc = head;
 	}