[v2] linux-generic: crypto: properly handle errors in packet copy

Message ID 20170313114408.26283-1-dmitry.ereminsolenikov@linaro.org
State New
Headers show

Commit Message

Dmitry Eremin-Solenikov March 13, 2017, 11:44 a.m.
Add proper handling for errors returned by odp_packet_copy_from_pkt().

Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>

---
 platform/linux-generic/odp_crypto.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
2.11.0

Comments

Dmitry Eremin-Solenikov April 2, 2017, 9:06 p.m. | #1
On 13 March 2017 at 14:44, Dmitry Eremin-Solenikov
<dmitry.ereminsolenikov@linaro.org> wrote:
> Add proper handling for errors returned by odp_packet_copy_from_pkt().

>

> Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>

> ---

>  platform/linux-generic/odp_crypto.c | 8 +++++++-

>  1 file changed, 7 insertions(+), 1 deletion(-)


Any update on this patch?

-- 
With best wishes
Dmitry
Bill Fischofer April 20, 2017, 4:12 p.m. | #2
On Mon, Mar 13, 2017 at 6:44 AM, Dmitry Eremin-Solenikov <
dmitry.ereminsolenikov@linaro.org> wrote:

> Add proper handling for errors returned by odp_packet_copy_from_pkt().

>

> Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>

>


Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>



> ---

>  platform/linux-generic/odp_crypto.c | 8 +++++++-

>  1 file changed, 7 insertions(+), 1 deletion(-)

>

> diff --git a/platform/linux-generic/odp_crypto.c

> b/platform/linux-generic/odp_crypto.c

> index 54b222fd..675b3e25 100644

> --- a/platform/linux-generic/odp_crypto.c

> +++ b/platform/linux-generic/odp_crypto.c

> @@ -886,11 +886,17 @@ odp_crypto_operation(odp_crypto_op_param_t *param,

>         }

>

>         if (param->pkt != param->out_pkt) {

> -               (void)odp_packet_copy_from_pkt(param->out_pkt,

> +               int ret;

> +

> +               ret = odp_packet_copy_from_pkt(param->out_pkt,

>                                                0,

>                                                param->pkt,

>                                                0,

>                                                odp_packet_len(param->pkt));

> +               if (odp_unlikely(ret < 0)) {

> +                       ODP_DBG("Copy failed.\n");

> +                       return -1;

> +               }

>                 _odp_packet_copy_md_to_packet(param->pkt, param->out_pkt);

>                 odp_packet_free(param->pkt);

>                 param->pkt = ODP_PACKET_INVALID;

> --

> 2.11.0

>

>
Maxim Uvarov April 20, 2017, 8:25 p.m. | #3
On 04/20/17 19:12, Bill Fischofer wrote:
> On Mon, Mar 13, 2017 at 6:44 AM, Dmitry Eremin-Solenikov <

> dmitry.ereminsolenikov@linaro.org> wrote:

> 

>> Add proper handling for errors returned by odp_packet_copy_from_pkt().

>>

>> Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>

>>

> 

> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>

> 

> 

>> ---

>>  platform/linux-generic/odp_crypto.c | 8 +++++++-

>>  1 file changed, 7 insertions(+), 1 deletion(-)

>>

>> diff --git a/platform/linux-generic/odp_crypto.c

>> b/platform/linux-generic/odp_crypto.c

>> index 54b222fd..675b3e25 100644

>> --- a/platform/linux-generic/odp_crypto.c

>> +++ b/platform/linux-generic/odp_crypto.c

>> @@ -886,11 +886,17 @@ odp_crypto_operation(odp_crypto_op_param_t *param,

>>         }

>>

>>         if (param->pkt != param->out_pkt) {

>> -               (void)odp_packet_copy_from_pkt(param->out_pkt,

>> +               int ret;

>> +

>> +               ret = odp_packet_copy_from_pkt(param->out_pkt,

>>                                                0,

>>                                                param->pkt,

>>                                                0,

>>                                                odp_packet_len(param->pkt));

>> +               if (odp_unlikely(ret < 0)) {

>> +                       ODP_DBG("Copy failed.\n");



if you are here than packet param->out_pkt was allocated, it needs to be
freed and set in invalid to prevent packet leaks.

it looks like also other non zero returns in that function needs the
same change.

Regards,
Maxim.


>> +                       return -1;

>> +               }

>>                 _odp_packet_copy_md_to_packet(param->pkt, param->out_pkt);

>>                 odp_packet_free(param->pkt);

>>                 param->pkt = ODP_PACKET_INVALID;

>> --

>> 2.11.0

>>

>>
Dmitry Eremin-Solenikov April 22, 2017, 4:20 p.m. | #4
On 20.04.2017 23:25, Maxim Uvarov wrote:
> On 04/20/17 19:12, Bill Fischofer wrote:

>> On Mon, Mar 13, 2017 at 6:44 AM, Dmitry Eremin-Solenikov <

>> dmitry.ereminsolenikov@linaro.org> wrote:

>>

>>> Add proper handling for errors returned by odp_packet_copy_from_pkt().

>>>

>>> Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>

>>>

>>

>> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>

>>

>>

>>> ---

>>>  platform/linux-generic/odp_crypto.c | 8 +++++++-

>>>  1 file changed, 7 insertions(+), 1 deletion(-)

>>>

>>> diff --git a/platform/linux-generic/odp_crypto.c

>>> b/platform/linux-generic/odp_crypto.c

>>> index 54b222fd..675b3e25 100644

>>> --- a/platform/linux-generic/odp_crypto.c

>>> +++ b/platform/linux-generic/odp_crypto.c

>>> @@ -886,11 +886,17 @@ odp_crypto_operation(odp_crypto_op_param_t *param,

>>>         }

>>>

>>>         if (param->pkt != param->out_pkt) {

>>> -               (void)odp_packet_copy_from_pkt(param->out_pkt,

>>> +               int ret;

>>> +

>>> +               ret = odp_packet_copy_from_pkt(param->out_pkt,

>>>                                                0,

>>>                                                param->pkt,

>>>                                                0,

>>>                                                odp_packet_len(param->pkt));

>>> +               if (odp_unlikely(ret < 0)) {

>>> +                       ODP_DBG("Copy failed.\n");

> 

> 

> if you are here than packet param->out_pkt was allocated, it needs to be

> freed and set in invalid to prevent packet leaks.

> 

> it looks like also other non zero returns in that function needs the

> same change.


There is connected interesting question, which should be though wrt. all
'packet-consuming' functions. Should such functions always consume and
free incoming packet? IOW:

 - odp_crypto_operation() returned -1. Should the app free inbound
packet afterwards?

 - odp_ipsec_*() returned -1. Should inbound packets be freed by an app?

Would it be sane for the app to assume that it should re-read inbound
packet/packet array from params(!) and free all non-INVALID packets (or
requeque them, etc).


-- 
With best wishes
Dmitry
Peltonen, Janne (Nokia - FI/Espoo) April 24, 2017, 12:58 p.m. | #5
Hi,

> 

> There is connected interesting question, which should be though wrt. all

> 'packet-consuming' functions. Should such functions always consume and

> free incoming packet? IOW:

> 

>  - odp_crypto_operation() returned -1. Should the app free inbound

> packet afterwards?


The API spec is not crystal clear about it but the way I read it is
that on failure the crypto operation does nothing to the input packets.
So the input packets continue to be owned by the application which needs
take care of them (maybe free them).

> 

>  - odp_ipsec_*() returned -1. Should inbound packets be freed by an app?

> 


I think the IPsec API is slightly more clear since it says that a positive
return value indicates the number of packets consumed by the operation.
So on error none of the input packets were consumed. In that case they
need to be freed or otherwise taken care of by the application which
still owns them.

> Would it be sane for the app to assume that it should re-read inbound

> packet/packet array from params(!) and free all non-INVALID packets (or

> requeque them, etc).


When the IPsec processing function returns a negative status, none
of the input packets were consumed. ODP does not update the operation
parameter struct and the input packet table in there, but there is also
no need to check the status packet-by-packet in that case. Per-packet
errors can occur only when the operation as a whole succeeds. Per-packet
errors are then indicated in the operation result struct.

	Janne

Patch

diff --git a/platform/linux-generic/odp_crypto.c b/platform/linux-generic/odp_crypto.c
index 54b222fd..675b3e25 100644
--- a/platform/linux-generic/odp_crypto.c
+++ b/platform/linux-generic/odp_crypto.c
@@ -886,11 +886,17 @@  odp_crypto_operation(odp_crypto_op_param_t *param,
 	}
 
 	if (param->pkt != param->out_pkt) {
-		(void)odp_packet_copy_from_pkt(param->out_pkt,
+		int ret;
+
+		ret = odp_packet_copy_from_pkt(param->out_pkt,
 					       0,
 					       param->pkt,
 					       0,
 					       odp_packet_len(param->pkt));
+		if (odp_unlikely(ret < 0)) {
+			ODP_DBG("Copy failed.\n");
+			return -1;
+		}
 		_odp_packet_copy_md_to_packet(param->pkt, param->out_pkt);
 		odp_packet_free(param->pkt);
 		param->pkt = ODP_PACKET_INVALID;