On 28 April 2017 at 18:32, Dmitry Eremin-Solenikov <[email protected]> wrote: > On 28.04.2017 15:32, Bala Manoharan wrote: >> On 28 April 2017 at 15:41, Dmitry Eremin-Solenikov >> <[email protected]> wrote: >>> On 28.04.2017 12:27, Peltonen, Janne (Nokia - FI/Espoo) wrote: >>>> Hi, >>>> >>>>> -----Original Message----- >>>>> From: lng-odp [mailto:[email protected]] On Behalf Of >>>>> Dmitry Eremin- >>>>> Solenikov >>>>> Sent: Friday, April 28, 2017 11:59 AM >>>>> To: lng-odp-forward <[email protected]> >>>>> 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.
odp_ipsec_sa_disable() is not a hard limit a few transient packets which getting processed in old SA should be fine. > >> 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. But this design does not require any blocking call and since application has to maintain the SA irrespective of handling by the implementation this could avoid it in two places. During failure the application can call sa_destroy() back after an expired timer or some other trigger from the control plane. The real issue here is that based on implementation this SA could either be active in a local memory or in the co-processor, so there is no exact time when this SA becomes inactive if the SA is copied on a per packet basis between the local cache into the co-processor then the SA could be destroyed the moment is has been successfully copied into the co-processor. > > -- > With best wishes > Dmitry
