On 28 April 2017 at 15:41, Dmitry Eremin-Solenikov <dmitry.ereminsoleni...@linaro.org> wrote: > On 28.04.2017 12:27, Peltonen, Janne (Nokia - FI/Espoo) wrote: >> Hi, >> >>> -----Original Message----- >>> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Dmitry >>> Eremin- >>> Solenikov >>> Sent: Friday, April 28, 2017 11:59 AM >>> To: lng-odp-forward <lng-odp@lists.linaro.org> >>> Subject: [lng-odp] IPsec SA disabling >>> >>> Hello, >>> >>> While responding to Janne's email, I've come to an issue which warrants >>> a separate discussion IMO. >>> >>> Consider the following scenario. Application with two thread. >>> >>> - First thread: gets outbound packet, finds SA through SPD, configures >>> IPsec request, submits it. >>> >>> - Second thread: wants to disable SA, pending deletion. >>> >>> Is that an application error if SA was disabled after being looked up, >>> but before being submitted by another thread? Yes it is. >> >> Yes, it would be an error. >> >>> Right now SA >>> disabling/deletion is susceptible to race conditions. It requires an app >>> to lock SA after lookup, unlock it after submission, etc. >> >> To avoid races one indeed currently needs some synchronization in the >> application side. Not necessarily locks but something. >> >> The OFP IPsec draft code (which is just draft, trying to use the ODP API), >> refcounts SA usage and disables an SA only after it has been removed >> from the SAD and after ongoing operations (tracked through the refcount) >> have completed. > > Basically as we already have refcounting on ODP side, I'd suggest > reusing it in applications. > >> >> Because of the above, one could wonder why even have disable() in the >> API. The main reason is to be able to disable offloaded inbound SA >> lookup for the SA so that the SA can be safely deleted (the application >> needs to be able to clean up its own SA state but cannot do it if there >> are in-flight inbound operations that may still refer (through the ctx ptr) >> to the application SA state). > > How do you track references of SA instances returned from ODP after > lookup? How will application cope with disabling/deleting SA if it is > returned to another thread by ODP? > >> >>> Should ODP >>> provide API to 'lock' SA? >> >> It would be convenient to the application if disable() could be called >> at any time. But that could impose difficult synchronization needs >> in the ODP implementation side and therefore we thought that it is >> better to leave that to the application which may need to do some >> synchronization anyway due to its own needs. >> >> But this is definitely a good topic to ponder about. It is possible >> that the current API got this wrong, but it is not easy to see how >> things should be if they need to be changed. > > Well, right now there is a need to do synchronization on ODP side > because odp_ipsec_sa_disable() is not an optional call. > >>> I was thinking about smth like: >>> >>> int odp_ipsec_sa_use(odp_ipsec_sa_t sa); >>> >>> void odp_ipsec_sa_unuse(odp_ipsec_sa_t sa); >>> >>> One would use first function when an app has looked up SA in it's own >>> tables and is going to submit it via one of odp_ipsec_* processing calls >>> (except or including SA deletion/disabling -- TBD). >> >> I am not sure how this would help and be better than something done >> totally at the application side. This would anyway require a bit more >> specification to define the semantics. For instance, when unuse() could >> be called with respect to the enque operations and what exactly it >> would mean. > > I'm just suggesting to move reference counting from application to ODP > side, since ODP has to do refcounting anyway (_disable() should wait > till all currently running operations with this SA finish). > > I thought basic refounting in mind. Probably we can get this as easy as: > > - odp_ipsec_sa_create: > > Create SA with refcnt = 1 > > - App looks up SA in SAD > odp_ipsec_sa_use(sa) > > - App has looked up SA in SAD, but then changed its mind > odp_ipsec_sa_unuse(sa) > > - ODP looks up SA in cache (via a request from app or via inline inbound > operation > odp_ipsec_sa_use(sa) > > - Application receives SA via a return from IPsec. > - Processes it and then calls odp_ipsec_sa_unuse(). > > - Application wants to disable SA in ASYNC mode. > odp_ipsec_sa_disable() marks SA for disablement. > - ASYNC case. It calls ipsec_sa_unuse() then returns. > > - SYNC case. It waits for refcount to drop to 1, unuses it then > returns. > - OR(!) SYNC case. It calls ipsec_sa_unuse(), waits for refcount to > drop to 0, then reutrns. > > - odp_ipsec_sa_unuse() is called with refount becoming 0. > - ASYNC case: > submit an even. > > - SYNC case: > do nothing.
I believe this synchronisation at application end might not be too difficult for look-aside mode since SA create/delete call is part of control flow and ipsec lookup is part of the data flow. In case of inline mode when application calls odp_ipsec_sa_disable() the SA gets disabled but there could be few transient packets which might flow during disable operation and when application calls odp_ipsec_sa_destroy() the destroy operation could simply fail if there are few packets which are still processed then the application could call the odp_ipsec_sa_destroy() after sometime when it becomes success. Regards, Bala > > -- > With best wishes > Dmitry
