[[RFCv2] 2/4] api: ipsec: add default queue for outbound events

Message ID 20170427115150.19452-2-dmitry.ereminsolenikov@linaro.org
State New
Headers show

Commit Message

Dmitry Eremin-Solenikov April 27, 2017, 11:51 a.m.
If an application has passed invalid SA in async mode, there is no way
to report it back to application except using default queue (which does
not exist at this moment).

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

---
 include/odp/api/spec/ipsec.h | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
2.11.0

Comments

Bill Fischofer April 27, 2017, 10:23 p.m. | #1
On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov <
dmitry.ereminsolenikov@linaro.org> wrote:

> If an application has passed invalid SA in async mode, there is no way

> to report it back to application except using default queue (which does

> not exist at this moment).

>

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

> ---

>  include/odp/api/spec/ipsec.h | 7 +++++++

>  1 file changed, 7 insertions(+)

>

> diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h

> index 4f746f67..7f43e81c 100644

> --- a/include/odp/api/spec/ipsec.h

> +++ b/include/odp/api/spec/ipsec.h

> @@ -190,6 +190,13 @@ typedef struct odp_ipsec_inbound_config_t {

>   * Configuration options for IPSEC outbound processing

>   */

>  typedef struct odp_ipsec_outbound_config_t {

> +       /** Default destination queue for IPSEC events

> +        *

> +        *  When passed invalid SA in the asynchronous mode,

> +        *  resulting IPSEC events are enqueued into this queue.

> +        */

> +       odp_queue_t default_queue;

>


Is an invalid SA going to be passed for any legitimate reason or is this
always a programming error? If the latter then this seems unnecessary as
this case falls under the "results are undefined" category. If the former,
then why can't the call simply be failed before being accepted for async
processing? The caller is "owed" an odp_ipsec_result_t only if the async
call is successful.


> +

>         /** Flags to control L3/L4 checksum insertion as part of outbound

>          *  packet processing. Packet must have set with valid L3/L4

> offsets.

>          *  Checksum configuration is ignored for packets that checksum

> cannot

> --

> 2.11.0

>

>
Dmitry Eremin-Solenikov April 27, 2017, 11:19 p.m. | #2
On 28.04.2017 01:23, Bill Fischofer wrote:
> 

> 

> On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov

> <dmitry.ereminsolenikov@linaro.org

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

> 

>     If an application has passed invalid SA in async mode, there is no way

>     to report it back to application except using default queue (which does

>     not exist at this moment).

> 

>     Signed-off-by: Dmitry Eremin-Solenikov

>     <dmitry.ereminsolenikov@linaro.org

>     <mailto:dmitry.ereminsolenikov@linaro.org>>

>     ---

>      include/odp/api/spec/ipsec.h | 7 +++++++

>      1 file changed, 7 insertions(+)

> 

>     diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h

>     index 4f746f67..7f43e81c 100644

>     --- a/include/odp/api/spec/ipsec.h

>     +++ b/include/odp/api/spec/ipsec.h

>     @@ -190,6 +190,13 @@ typedef struct odp_ipsec_inbound_config_t {

>       * Configuration options for IPSEC outbound processing

>       */

>      typedef struct odp_ipsec_outbound_config_t {

>     +       /** Default destination queue for IPSEC events

>     +        *

>     +        *  When passed invalid SA in the asynchronous mode,

>     +        *  resulting IPSEC events are enqueued into this queue.

>     +        */

>     +       odp_queue_t default_queue;

> 

> 

> Is an invalid SA going to be passed for any legitimate reason or is this

> always a programming error? If the latter then this seems unnecessary as

> this case falls under the "results are undefined" category. If the

> former, then why can't the call simply be failed before being accepted

> for async processing? The caller is "owed" an odp_ipsec_result_t only if

> the async call is successful.


Unfortunately returning an error or 'processing stopped here' does not
allow application to determine the cause of that error/processing stop.
So, it well might resubmit the packet later. Thus it might be better to
accept such packet and return an error through event.

-- 
With best wishes
Dmitry
Bill Fischofer April 28, 2017, 12:33 a.m. | #3
On Thu, Apr 27, 2017 at 6:19 PM, Dmitry Eremin-Solenikov <
dmitry.ereminsolenikov@linaro.org> wrote:

> On 28.04.2017 01:23, Bill Fischofer wrote:

> >

> >

> > On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov

> > <dmitry.ereminsolenikov@linaro.org

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

> >

> >     If an application has passed invalid SA in async mode, there is no

> way

> >     to report it back to application except using default queue (which

> does

> >     not exist at this moment).

> >

> >     Signed-off-by: Dmitry Eremin-Solenikov

> >     <dmitry.ereminsolenikov@linaro.org

> >     <mailto:dmitry.ereminsolenikov@linaro.org>>

> >     ---

> >      include/odp/api/spec/ipsec.h | 7 +++++++

> >      1 file changed, 7 insertions(+)

> >

> >     diff --git a/include/odp/api/spec/ipsec.h

> b/include/odp/api/spec/ipsec.h

> >     index 4f746f67..7f43e81c 100644

> >     --- a/include/odp/api/spec/ipsec.h

> >     +++ b/include/odp/api/spec/ipsec.h

> >     @@ -190,6 +190,13 @@ typedef struct odp_ipsec_inbound_config_t {

> >       * Configuration options for IPSEC outbound processing

> >       */

> >      typedef struct odp_ipsec_outbound_config_t {

> >     +       /** Default destination queue for IPSEC events

> >     +        *

> >     +        *  When passed invalid SA in the asynchronous mode,

> >     +        *  resulting IPSEC events are enqueued into this queue.

> >     +        */

> >     +       odp_queue_t default_queue;

> >

> >

> > Is an invalid SA going to be passed for any legitimate reason or is this

> > always a programming error? If the latter then this seems unnecessary as

> > this case falls under the "results are undefined" category. If the

> > former, then why can't the call simply be failed before being accepted

> > for async processing? The caller is "owed" an odp_ipsec_result_t only if

> > the async call is successful.

>

> Unfortunately returning an error or 'processing stopped here' does not

> allow application to determine the cause of that error/processing stop.

> So, it well might resubmit the packet later. Thus it might be better to

> accept such packet and return an error through event.

>


Communicating error cause is what odp_errno is designed for. If you're
saying an application might loop on an error, that's certainly a
possibility but not something that ODP can control nor would that be unique
to this API.


>

> --

> With best wishes

> Dmitry

>
Dmitry Eremin-Solenikov April 28, 2017, 12:44 a.m. | #4
On 28.04.2017 03:33, Bill Fischofer wrote:
> 

> 

> On Thu, Apr 27, 2017 at 6:19 PM, Dmitry Eremin-Solenikov

> <dmitry.ereminsolenikov@linaro.org

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

> 

>     On 28.04.2017 01:23, Bill Fischofer wrote:

>     >

>     >

>     > On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov

>     > <dmitry.ereminsolenikov@linaro.org

>     <mailto:dmitry.ereminsolenikov@linaro.org>

>     > <mailto:dmitry.ereminsolenikov@linaro.org

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

>     >

>     >     If an application has passed invalid SA in async mode, there is no way

>     >     to report it back to application except using default queue (which does

>     >     not exist at this moment).

>     >

>     >     Signed-off-by: Dmitry Eremin-Solenikov

>     >     <dmitry.ereminsolenikov@linaro.org

>     <mailto:dmitry.ereminsolenikov@linaro.org>

>     >     <mailto:dmitry.ereminsolenikov@linaro.org

>     <mailto:dmitry.ereminsolenikov@linaro.org>>>

>     >     ---

>     >      include/odp/api/spec/ipsec.h | 7 +++++++

>     >      1 file changed, 7 insertions(+)

>     >

>     >     diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h

>     >     index 4f746f67..7f43e81c 100644

>     >     --- a/include/odp/api/spec/ipsec.h

>     >     +++ b/include/odp/api/spec/ipsec.h

>     >     @@ -190,6 +190,13 @@ typedef struct odp_ipsec_inbound_config_t {

>     >       * Configuration options for IPSEC outbound processing

>     >       */

>     >      typedef struct odp_ipsec_outbound_config_t {

>     >     +       /** Default destination queue for IPSEC events

>     >     +        *

>     >     +        *  When passed invalid SA in the asynchronous mode,

>     >     +        *  resulting IPSEC events are enqueued into this queue.

>     >     +        */

>     >     +       odp_queue_t default_queue;

>     >

>     >

>     > Is an invalid SA going to be passed for any legitimate reason or is this

>     > always a programming error? If the latter then this seems unnecessary as

>     > this case falls under the "results are undefined" category. If the

>     > former, then why can't the call simply be failed before being accepted

>     > for async processing? The caller is "owed" an odp_ipsec_result_t only if

>     > the async call is successful.

> 

>     Unfortunately returning an error or 'processing stopped here' does not

>     allow application to determine the cause of that error/processing stop.

>     So, it well might resubmit the packet later. Thus it might be better to

>     accept such packet and return an error through event.

> 

> 

> Communicating error cause is what odp_errno is designed for. If you're

> saying an application might loop on an error, that's certainly a

> possibility but not something that ODP can control nor would that be

> unique to this API.


I mean that it can loop, because (at least now) API docs don't say a
thing about possible error causes.

Moreover, another point for default_queue. Consider odp_ipsec_out()
call. There is no need to stop process on invalid/disabled SA. SYNC call
can set sa_lookup right away and be happy with that. I don't see a
reason why odp_ipsec_out_enq() should report same error in a different way.

-- 
With best wishes
Dmitry
Peltonen, Janne (Nokia - FI/Espoo) April 28, 2017, 8:29 a.m. | #5
> -----Original Message-----

> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Dmitry Eremin-

> Solenikov

> Sent: Friday, April 28, 2017 3:44 AM

> To: Bill Fischofer <bill.fischofer@linaro.org>

> Cc: lng-odp-forward <lng-odp@lists.linaro.org>

> Subject: Re: [lng-odp] [[RFCv2] 2/4] api: ipsec: add default queue for outbound events

> 

> On 28.04.2017 03:33, Bill Fischofer wrote:

> >

> >

> > On Thu, Apr 27, 2017 at 6:19 PM, Dmitry Eremin-Solenikov

> > <dmitry.ereminsolenikov@linaro.org

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

> >

> >     On 28.04.2017 01:23, Bill Fischofer wrote:

> >     >

> >     >

> >     > On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov

> >     > <dmitry.ereminsolenikov@linaro.org

> >     <mailto:dmitry.ereminsolenikov@linaro.org>

> >     > <mailto:dmitry.ereminsolenikov@linaro.org

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

> >     >

> >     >     If an application has passed invalid SA in async mode, there is no way

> >     >     to report it back to application except using default queue (which does

> >     >     not exist at this moment).

> >     >

> >     >     Signed-off-by: Dmitry Eremin-Solenikov

> >     >     <dmitry.ereminsolenikov@linaro.org

> >     <mailto:dmitry.ereminsolenikov@linaro.org>

> >     >     <mailto:dmitry.ereminsolenikov@linaro.org

> >     <mailto:dmitry.ereminsolenikov@linaro.org>>>

> >     >     ---

> >     >      include/odp/api/spec/ipsec.h | 7 +++++++

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

> >     >

> >     >     diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h

> >     >     index 4f746f67..7f43e81c 100644

> >     >     --- a/include/odp/api/spec/ipsec.h

> >     >     +++ b/include/odp/api/spec/ipsec.h

> >     >     @@ -190,6 +190,13 @@ typedef struct odp_ipsec_inbound_config_t {

> >     >       * Configuration options for IPSEC outbound processing

> >     >       */

> >     >      typedef struct odp_ipsec_outbound_config_t {

> >     >     +       /** Default destination queue for IPSEC events

> >     >     +        *

> >     >     +        *  When passed invalid SA in the asynchronous mode,

> >     >     +        *  resulting IPSEC events are enqueued into this queue.

> >     >     +        */

> >     >     +       odp_queue_t default_queue;

> >     >

> >     >

> >     > Is an invalid SA going to be passed for any legitimate reason or is this

> >     > always a programming error? If the latter then this seems unnecessary as

> >     > this case falls under the "results are undefined" category. If the

> >     > former, then why can't the call simply be failed before being accepted

> >     > for async processing? The caller is "owed" an odp_ipsec_result_t only if

> >     > the async call is successful.

> >

> >     Unfortunately returning an error or 'processing stopped here' does not

> >     allow application to determine the cause of that error/processing stop.


Passing invalid SAs through the API is a programming error, so one could argue
that it is not the application but the programmer who needs to figure it out
and do the debugging.

> >     So, it well might resubmit the packet later. Thus it might be better to

> >     accept such packet and return an error through event.

> >

> >

> > Communicating error cause is what odp_errno is designed for. If you're

> > saying an application might loop on an error, that's certainly a

> > possibility but not something that ODP can control nor would that be

> > unique to this API.

> 

> I mean that it can loop, because (at least now) API docs don't say a

> thing about possible error causes.


Since calling the ipsec API with an invalid SA is a programming error
and not something that every underlying implementation needs to be
prepared to handle, anything can happen. The application may not even
get a change to decide whether to loop since the ODP implementation
may already crash before that.

Requiring extensive and safe validity checks for the parameters provided
through the API can incur significant performance penalty, including a
need for locking.

Even if the implementation manages to return an error for an invalid SA,
I do not think it would be reasonable for the application to retry.
Generally a failure does not go away by just trying again, and the cases
where trying again is the right thing to do are typically documented and
have a specific error code (EAGAIN, EWOULDBLOCK and similar).

If adding a specific error code in the API for certain special case
would help, that would be a lighter weight thing that to add a whole
new queue.
 
> Moreover, another point for default_queue. Consider odp_ipsec_out()

> call. There is no need to stop process on invalid/disabled SA.


Some programming errors may be more easily detectable and may be
less catastrophic in the synchronous API than in the asynchronous API.

Or maybe not. What does invalid SA mean anyway? ODP_IPSE_SA_INVALID
specifically or any random crap? The implementation may not be able
to check for the latter and may very well crash.

The sync call may also crash if provided e.g. a disabled SA, depending
on the ODP implementation. The spec says that disabled SAs may not be
used as parameters to IPsec packet operations. The ODP implementation
may take advantage of that fact for its synchronization design in which
case using a disabled SA may easily lead to undefined behavior already
at the C language level.

> SYNC call

> can set sa_lookup right away and be happy with that. I don't see a

> reason why odp_ipsec_out_enq() should report same error in a different way.


I think it would be wrong to set sa_lookup to indicate SA lookup
failure in a case where SA lookup was not requested in the first
place (SA handle was explicitly provided). sa_lookup error should only
come from inbound IPsec since SA lookup is done only there.

The implementation may look up its own internal data structures based
on the SA handle, but that is different from the SA lookup in this API
where SA lookup is about searching an SA based on the packet.

	Janne
Dmitry Eremin-Solenikov April 28, 2017, 10:28 a.m. | #6
On 28.04.2017 11:29, Peltonen, Janne (Nokia - FI/Espoo) wrote:
> 

> 

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

>> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Dmitry Eremin-

>> Solenikov

>> Sent: Friday, April 28, 2017 3:44 AM

>> To: Bill Fischofer <bill.fischofer@linaro.org>

>> Cc: lng-odp-forward <lng-odp@lists.linaro.org>

>> Subject: Re: [lng-odp] [[RFCv2] 2/4] api: ipsec: add default queue for outbound events

>>

>> On 28.04.2017 03:33, Bill Fischofer wrote:

>>>

>>>

>>> On Thu, Apr 27, 2017 at 6:19 PM, Dmitry Eremin-Solenikov

>>> <dmitry.ereminsolenikov@linaro.org

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

>>>

>>>     On 28.04.2017 01:23, Bill Fischofer wrote:

>>>     >

>>>     >

>>>     > On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov

>>>     > <dmitry.ereminsolenikov@linaro.org

>>>     <mailto:dmitry.ereminsolenikov@linaro.org>

>>>     > <mailto:dmitry.ereminsolenikov@linaro.org

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

>>>     >

>>>     >     If an application has passed invalid SA in async mode, there is no way

>>>     >     to report it back to application except using default queue (which does

>>>     >     not exist at this moment).

>>>     >

>>>     >     Signed-off-by: Dmitry Eremin-Solenikov

>>>     >     <dmitry.ereminsolenikov@linaro.org

>>>     <mailto:dmitry.ereminsolenikov@linaro.org>

>>>     >     <mailto:dmitry.ereminsolenikov@linaro.org

>>>     <mailto:dmitry.ereminsolenikov@linaro.org>>>

>>>     >     ---

>>>     >      include/odp/api/spec/ipsec.h | 7 +++++++

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

>>>     >

>>>     >     diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h

>>>     >     index 4f746f67..7f43e81c 100644

>>>     >     --- a/include/odp/api/spec/ipsec.h

>>>     >     +++ b/include/odp/api/spec/ipsec.h

>>>     >     @@ -190,6 +190,13 @@ typedef struct odp_ipsec_inbound_config_t {

>>>     >       * Configuration options for IPSEC outbound processing

>>>     >       */

>>>     >      typedef struct odp_ipsec_outbound_config_t {

>>>     >     +       /** Default destination queue for IPSEC events

>>>     >     +        *

>>>     >     +        *  When passed invalid SA in the asynchronous mode,

>>>     >     +        *  resulting IPSEC events are enqueued into this queue.

>>>     >     +        */

>>>     >     +       odp_queue_t default_queue;

>>>     >

>>>     >

>>>     > Is an invalid SA going to be passed for any legitimate reason or is this

>>>     > always a programming error? If the latter then this seems unnecessary as

>>>     > this case falls under the "results are undefined" category. If the

>>>     > former, then why can't the call simply be failed before being accepted

>>>     > for async processing? The caller is "owed" an odp_ipsec_result_t only if

>>>     > the async call is successful.

>>>

>>>     Unfortunately returning an error or 'processing stopped here' does not

>>>     allow application to determine the cause of that error/processing stop.

> 

> Passing invalid SAs through the API is a programming error, so one could argue

> that it is not the application but the programmer who needs to figure it out

> and do the debugging.


I thought that we should provide our best efforts to aid debugging, if
that does not incur performance problems.

> 

>>>     So, it well might resubmit the packet later. Thus it might be better to

>>>     accept such packet and return an error through event.

>>>

>>>

>>> Communicating error cause is what odp_errno is designed for. If you're

>>> saying an application might loop on an error, that's certainly a

>>> possibility but not something that ODP can control nor would that be

>>> unique to this API.

>>

>> I mean that it can loop, because (at least now) API docs don't say a

>> thing about possible error causes.

> 

> Since calling the ipsec API with an invalid SA is a programming error

> and not something that every underlying implementation needs to be

> prepared to handle, anything can happen. The application may not even

> get a change to decide whether to loop since the ODP implementation

> may already crash before that.


Well... In case of security-related issues, a crash might be a good
ending. Bad ending would be ODP doing _something_, which is unrelated to
original app request. Like sending sensitive data (originally destined
to ESP encryption) through AH.

> 

> Requiring extensive and safe validity checks for the parameters provided

> through the API can incur significant performance penalty, including a

> need for locking.

> 

> Even if the implementation manages to return an error for an invalid SA,

> I do not think it would be reasonable for the application to retry.

> Generally a failure does not go away by just trying again, and the cases

> where trying again is the right thing to do are typically documented and

> have a specific error code (EAGAIN, EWOULDBLOCK and similar).

> 

> If adding a specific error code in the API for certain special case

> would help, that would be a lighter weight thing that to add a whole

> new queue.


The problem is that there is no such return code defined for any of
operations (both inbound and outbound).

>  

>> Moreover, another point for default_queue. Consider odp_ipsec_out()

>> call. There is no need to stop process on invalid/disabled SA.

> 

> Some programming errors may be more easily detectable and may be

> less catastrophic in the synchronous API than in the asynchronous API.

> 

> Or maybe not. What does invalid SA mean anyway? ODP_IPSE_SA_INVALID

> specifically or any random crap? The implementation may not be able

> to check for the latter and may very well crash.

> 

> The sync call may also crash if provided e.g. a disabled SA, depending

> on the ODP implementation. The spec says that disabled SAs may not be

> used as parameters to IPsec packet operations. The ODP implementation

> may take advantage of that fact for its synchronization design in which

> case using a disabled SA may easily lead to undefined behavior already

> at the C language level.


Comparing with the rest of API, I had the following semantics in mind:

- odp_ipsec_* calls may return failure or stop processing in case of
generic issues. Like being unable to offload the data to engine or just
being unable to allocate buffer.

- Any further error (like SA_INVALID, disabled SA, AH SA passed to
process ESP packet, etc) should return one of errors through
odp_ipsec_op_result_t.

>> SYNC call

>> can set sa_lookup right away and be happy with that. I don't see a

>> reason why odp_ipsec_out_enq() should report same error in a different way.

> 

> I think it would be wrong to set sa_lookup to indicate SA lookup

> failure in a case where SA lookup was not requested in the first

> place (SA handle was explicitly provided). sa_lookup error should only

> come from inbound IPsec since SA lookup is done only there.


sa_disabled/sa_invalid?

> The implementation may look up its own internal data structures based

> on the SA handle, but that is different from the SA lookup in this API

> where SA lookup is about searching an SA based on the packet.



-- 
With best wishes
Dmitry
Bill Fischofer May 1, 2017, 5:33 p.m. | #7
On Fri, Apr 28, 2017 at 5:28 AM, Dmitry Eremin-Solenikov <
dmitry.ereminsolenikov@linaro.org> wrote:

> On 28.04.2017 11:29, Peltonen, Janne (Nokia - FI/Espoo) wrote:

> >

> >

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

> >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of

> Dmitry Eremin-

> >> Solenikov

> >> Sent: Friday, April 28, 2017 3:44 AM

> >> To: Bill Fischofer <bill.fischofer@linaro.org>

> >> Cc: lng-odp-forward <lng-odp@lists.linaro.org>

> >> Subject: Re: [lng-odp] [[RFCv2] 2/4] api: ipsec: add default queue for

> outbound events

> >>

> >> On 28.04.2017 03:33, Bill Fischofer wrote:

> >>>

> >>>

> >>> On Thu, Apr 27, 2017 at 6:19 PM, Dmitry Eremin-Solenikov

> >>> <dmitry.ereminsolenikov@linaro.org

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

> >>>

> >>>     On 28.04.2017 01:23, Bill Fischofer wrote:

> >>>     >

> >>>     >

> >>>     > On Thu, Apr 27, 2017 at 6:51 AM, Dmitry Eremin-Solenikov

> >>>     > <dmitry.ereminsolenikov@linaro.org

> >>>     <mailto:dmitry.ereminsolenikov@linaro.org>

> >>>     > <mailto:dmitry.ereminsolenikov@linaro.org

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

> >>>     >

> >>>     >     If an application has passed invalid SA in async mode, there

> is no way

> >>>     >     to report it back to application except using default queue

> (which does

> >>>     >     not exist at this moment).

> >>>     >

> >>>     >     Signed-off-by: Dmitry Eremin-Solenikov

> >>>     >     <dmitry.ereminsolenikov@linaro.org

> >>>     <mailto:dmitry.ereminsolenikov@linaro.org>

> >>>     >     <mailto:dmitry.ereminsolenikov@linaro.org

> >>>     <mailto:dmitry.ereminsolenikov@linaro.org>>>

> >>>     >     ---

> >>>     >      include/odp/api/spec/ipsec.h | 7 +++++++

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

> >>>     >

> >>>     >     diff --git a/include/odp/api/spec/ipsec.h

> b/include/odp/api/spec/ipsec.h

> >>>     >     index 4f746f67..7f43e81c 100644

> >>>     >     --- a/include/odp/api/spec/ipsec.h

> >>>     >     +++ b/include/odp/api/spec/ipsec.h

> >>>     >     @@ -190,6 +190,13 @@ typedef struct

> odp_ipsec_inbound_config_t {

> >>>     >       * Configuration options for IPSEC outbound processing

> >>>     >       */

> >>>     >      typedef struct odp_ipsec_outbound_config_t {

> >>>     >     +       /** Default destination queue for IPSEC events

> >>>     >     +        *

> >>>     >     +        *  When passed invalid SA in the asynchronous mode,

> >>>     >     +        *  resulting IPSEC events are enqueued into this

> queue.

> >>>     >     +        */

> >>>     >     +       odp_queue_t default_queue;

> >>>     >

> >>>     >

> >>>     > Is an invalid SA going to be passed for any legitimate reason or

> is this

> >>>     > always a programming error? If the latter then this seems

> unnecessary as

> >>>     > this case falls under the "results are undefined" category. If

> the

> >>>     > former, then why can't the call simply be failed before being

> accepted

> >>>     > for async processing? The caller is "owed" an odp_ipsec_result_t

> only if

> >>>     > the async call is successful.

> >>>

> >>>     Unfortunately returning an error or 'processing stopped here' does

> not

> >>>     allow application to determine the cause of that error/processing

> stop.

> >

> > Passing invalid SAs through the API is a programming error, so one could

> argue

> > that it is not the application but the programmer who needs to figure it

> out

> > and do the debugging.

>

> I thought that we should provide our best efforts to aid debugging, if

> that does not incur performance problems.

>


Yes, which is why we do (limited) validity checking on non-performance
paths (e.g., odp_ipsec_sa_create()). The real answer to this is to use the
ODP_ASSERT() mechanism. That allows run-time checking that is only invoked
in debug builds. So if a programmer doesn't immediately see the error,
re-running the program against a debug build of ODP will provide more
precise diagnostic info. Quite a number of the odp_packet_xxx() APIs take
this approach since default runtime checking would impose unacceptable
performance penalties for many of these APIs. Many IPsec APIs would fall
into the same category and could benefit from the same approach.


>

> >

> >>>     So, it well might resubmit the packet later. Thus it might be

> better to

> >>>     accept such packet and return an error through event.

> >>>

> >>>

> >>> Communicating error cause is what odp_errno is designed for. If you're

> >>> saying an application might loop on an error, that's certainly a

> >>> possibility but not something that ODP can control nor would that be

> >>> unique to this API.

> >>

> >> I mean that it can loop, because (at least now) API docs don't say a

> >> thing about possible error causes.

> >

> > Since calling the ipsec API with an invalid SA is a programming error

> > and not something that every underlying implementation needs to be

> > prepared to handle, anything can happen. The application may not even

> > get a change to decide whether to loop since the ODP implementation

> > may already crash before that.

>

> Well... In case of security-related issues, a crash might be a good

> ending. Bad ending would be ODP doing _something_, which is unrelated to

> original app request. Like sending sensitive data (originally destined

> to ESP encryption) through AH.

>


No different than due to an application programming error the wrong data is
sent out on an incorrect (but valid) SA.


>

> >

> > Requiring extensive and safe validity checks for the parameters provided

> > through the API can incur significant performance penalty, including a

> > need for locking.

> >

> > Even if the implementation manages to return an error for an invalid SA,

> > I do not think it would be reasonable for the application to retry.

> > Generally a failure does not go away by just trying again, and the cases

> > where trying again is the right thing to do are typically documented and

> > have a specific error code (EAGAIN, EWOULDBLOCK and similar).

> >

> > If adding a specific error code in the API for certain special case

> > would help, that would be a lighter weight thing that to add a whole

> > new queue.

>

> The problem is that there is no such return code defined for any of

> operations (both inbound and outbound).

>


We could define them, but then that becomes an obligation for all ODP
implementations. What we've said is each implementation may set odp_errno
to an implementation-defined value to provide additional reason info when
an API gives an error return (RC < 0). These presumably would be logged so
some human could properly interpret them based on the implementation this
program is running on. These codes are inherently implementation dependent
since the reasons why a given API may fail may be unique to that
implementation (e.g., internal resource limits or other conditions that may
not affect other implementations).


>

> >

> >> Moreover, another point for default_queue. Consider odp_ipsec_out()

> >> call. There is no need to stop process on invalid/disabled SA.

> >

> > Some programming errors may be more easily detectable and may be

> > less catastrophic in the synchronous API than in the asynchronous API.

> >

> > Or maybe not. What does invalid SA mean anyway? ODP_IPSE_SA_INVALID

> > specifically or any random crap? The implementation may not be able

> > to check for the latter and may very well crash.

> >

> > The sync call may also crash if provided e.g. a disabled SA, depending

> > on the ODP implementation. The spec says that disabled SAs may not be

> > used as parameters to IPsec packet operations. The ODP implementation

> > may take advantage of that fact for its synchronization design in which

> > case using a disabled SA may easily lead to undefined behavior already

> > at the C language level.

>

> Comparing with the rest of API, I had the following semantics in mind:

>

> - odp_ipsec_* calls may return failure or stop processing in case of

> generic issues. Like being unable to offload the data to engine or just

> being unable to allocate buffer.

>

> - Any further error (like SA_INVALID, disabled SA, AH SA passed to

> process ESP packet, etc) should return one of errors through

> odp_ipsec_op_result_t.

>


This is a reasonable approach, however the caller is owed an
odp_opsec_op_result_t only if the originating API returns with RC == 0.
However, every valid SA already has a result return queue associated with
it, so it seems the only use this default queue would get would be for this
specific programming error of passing an invalid SA that cannot be
determined to be invalid at the time of the original API call. That seems
very narrow. The argument for the default queue would be stronger if there
were some other conditions under which it would be used.


>

> >> SYNC call

> >> can set sa_lookup right away and be happy with that. I don't see a

> >> reason why odp_ipsec_out_enq() should report same error in a different

> way.

> >

> > I think it would be wrong to set sa_lookup to indicate SA lookup

> > failure in a case where SA lookup was not requested in the first

> > place (SA handle was explicitly provided). sa_lookup error should only

> > come from inbound IPsec since SA lookup is done only there.

>

> sa_disabled/sa_invalid?

>


An SA is disabled if someone has called odp_ipsec_sa_disable() first. An SA
is invalid under two conditions: (a) a previously valid SA was successfully
closed or (b) a bogus odp_ipsec_sa_t is provided that was never valid in
the first place. Depending on the implementation it may be easy to provide
an error RC in these cases (e.g., odp_ipsec_sa_t is an index value and it's
a simple range check or table entry examination) but in other
implementation it may not (e.g., odp_ipsec_sa_t is a pointer to some
dynamic struct so a bogus value will likely result in a segfault). Both
fall under the "results are undefined" heading.


>

> > The implementation may look up its own internal data structures based

> > on the SA handle, but that is different from the SA lookup in this API

> > where SA lookup is about searching an SA based on the packet.

>

>

> --

> With best wishes

> Dmitry

>

Patch hide | download patch | download mbox

diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h
index 4f746f67..7f43e81c 100644
--- a/include/odp/api/spec/ipsec.h
+++ b/include/odp/api/spec/ipsec.h
@@ -190,6 +190,13 @@  typedef struct odp_ipsec_inbound_config_t {
  * Configuration options for IPSEC outbound processing
  */
 typedef struct odp_ipsec_outbound_config_t {
+	/** Default destination queue for IPSEC events
+	 *
+	 *  When passed invalid SA in the asynchronous mode,
+	 *  resulting IPSEC events are enqueued into this queue.
+	 */
+	odp_queue_t default_queue;
+
 	/** Flags to control L3/L4 checksum insertion as part of outbound
 	 *  packet processing. Packet must have set with valid L3/L4 offsets.
 	 *  Checksum configuration is ignored for packets that checksum cannot