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.
--
With best wishes
Dmitry
