| Message ID | 20170313114408.26283-1-dmitry.ereminsolenikov@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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 > >
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 >> >>
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
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
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;
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