On 28.04.2017 15:32, Bala Manoharan wrote: > 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.
Even for lookaside mode synchronization is required since ODP can lookup SA from inbound packet asynchronously with main app. > 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. This would require additional checks in ODP side. Also if currently sa_destroy() can be called from event handler, completely destroying SA, with your suggestion it is unclear when sa_destroy() can be called. Especially if first call fails. -- With best wishes Dmitry
