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