JannePeltonen replied on github web page:
doc/images/ipsec_fsm.gv
line 1
@@ -0,0 +1,32 @@
+digraph ipsec_state_machine {
Comment:
I do not quite understand the SA state machine picture, maybe because I think
it tries to include different kind of states (global IPsec subsystem state,
IPsec SA state, IPsec operation state) in the same picture.
The operation related states (Processing, Op_Complete) are not SA states but
states of individual packet operations. There can be multiple packet operations
ongoing and in different states at the same time for the same SA.
I can identify only the following four SA states:
1) Nonexistent: The SA does not exist
- odp_ipsec_sa_create() changes the SA state to SA_Ready.
2) SA_Ready: The SA can be used for processing packets
- odp_ipsec_sa_disable() changes the SA state to Disable_Pending
3) Disable_Pending: The SA is processing in-flight packets but cannot anymore
be passed as a parameter to any new IPsec packet operation.
- The ODP implementation removes the SA from inbound SA lookup (if the SA is an
inbound SA that offloads SA lookup) and completes processing the in-flight
packets for the SA and enqueues the operation completion IPsec packet events.
After that the ODP implementation enqueues a disable complete SA status event
and then the SA state changes to SA_Disabled.
4) SA_Disabled: The SA has been disabled.
- No packets are processed by the SA and no new IPsec packet or status events
for the SA appear.
- odp_ipsec_sa_destroy() destroys the SA fully and changes the SA state to
Nonexistent.
Then one could define SA_expired and SA_not_expired sub-states for some of the
states (SA_Ready and Disable_Pending), but the lifetime related states could
also be seen as a separate FSM, mostly independent from the main SA FSM.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Good catch. Fixed in v13.
>> JannePeltonen wrote
>> May it should be mentioned somewhere here for clarity that also asynchronous
>> look-a-side operations are allowed in the inline mode.
>>> JannePeltonen wrote
>>> The type of the inbound_mode and outbound_mode config settings is
>>> odp_ipsec_op_mode_t.
>>>> JannePeltonen wrote
>>>> Asynchronous look-a-side APIs may also be used in the inline mode. It is
>>>> needed for cases where inline processing of certain packets is not
>>>> possible, e.g. due to them being encapsulated with some other tunneling
>>>> protocol in addition to IPsec.
>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>> Clarified in v12. Thanks.
>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>> This version of the PR was intended to match the changes in PR #256.
>>>>>> I'll revert these in the next version since it seems we're now content
>>>>>> with the original semantics.
>>>>>>> JannePeltonen wrote
>>>>>>> Maybe it is not useful to mention length here since it is only a
>>>>>>> prefetching hint.
>>>>>>>> JannePeltonen wrote
>>>>>>>> Maybe it should be made more clear somewhere that the synchronous
>>>>>>>> processing calls are allowed only in the synchronous operating mode
>>>>>>>> and asynchronous calls in asynchronous and inline operation mode (and
>>>>>>>> inline calls only in the inline mode).
>>>>>>>>> JannePeltonen wrote
>>>>>>>>> This dummy packet stuff is not yet (if ever?) part of the API spec. I
>>>>>>>>> think this patch should be in sync with what is the current state of
>>>>>>>>> api-next, which still uses the status event for indicating SA disable
>>>>>>>>> completion.
>>>>>>>>>> JannePeltonen wrote
>>>>>>>>>> "This call" could be replaced with odp_ipsec_sa_disable() to make
>>>>>>>>>> the text more clear.
>>>>>>>>>>> JannePeltonen wrote
>>>>>>>>>>> I think this may give the impression that calling
>>>>>>>>>>> odp_ipsec_sa_disable() is optional, when in fact it is mandatory
>>>>>>>>>>> before odp_ipsec_sa_destroy().
>>>>>>>>>>>> JannePeltonen wrote
>>>>>>>>>>>> According to the API spec soft limit alerts do not need to be edge
>>>>>>>>>>>> triggered. The spec says: "It's implementation defined how many
>>>>>>>>>>>> times soft lifetime expiration is reported: only once, first N or
>>>>>>>>>>>> all packets following the limit crossing."
>>>>>>>>>>>>> JannePeltonen wrote
>>>>>>>>>>>>> Is it clear enough here that the mtu error bit is set only when
>>>>>>>>>>>>> MTU checking was requested and not in case fragmentation offload
>>>>>>>>>>>>> was requested?
>>>>>>>>>>>>>> JannePeltonen wrote
>>>>>>>>>>>>>> Same comment as for odp_ipsec_{in,out}(), the description of the
>>>>>>>>>>>>>> return code is not correct.
>>>>>>>>>>>>>>> JannePeltonen wrote
>>>>>>>>>>>>>>> The return code does not tell whether the application of the SA
>>>>>>>>>>>>>>> was successful but how many input packets were consumed by the
>>>>>>>>>>>>>>> operation. The per-packet operation status is retrieved via the
>>>>>>>>>>>>>>> odp_ipsec_result() api for the outputted packets, which will be
>>>>>>>>>>>>>>> available only when the function succeeds.
>>>>>>>>>>>>>>>> JannePeltonen wrote
>>>>>>>>>>>>>>>> The operating mode is a global setting, not per-SA setting as
>>>>>>>>>>>>>>>> the text implies. And the relevant type is
>>>>>>>>>>>>>>>> odp_ipsec_op_mode_t, not odp_ipsec_mode_t which is for
>>>>>>>>>>>>>>>> selecting between tunnel and transport mode.
>>>>>>>>>>>>>>>>> JannePeltonen wrote
>>>>>>>>>>>>>>>>> "How this is done" sounds a bit awkward as it is not clear
>>>>>>>>>>>>>>>>> what "this" refers to (the way the IPsec services are used).
>>>>>>>>>>>>>>>>> Maybe the sentence should be reworded.
>>>>>>>>>>>>>>>>>> JannePeltonen wrote
>>>>>>>>>>>>>>>>>> Would "minimum size" be better than "size" here?
>>>>>>>>>>>>>>>>>>> JannePeltonen wrote
>>>>>>>>>>>>>>>>>>> The queue is not only for status events but for ipsec
>>>>>>>>>>>>>>>>>>> packet events too. Maybe just remove the word "status"?
>>>>>>>>>>>>>>>>>>>> JannePeltonen wrote
>>>>>>>>>>>>>>>>>>>> This should be: "...can be retained..." instead of
>>>>>>>>>>>>>>>>>>>> "...should be retained..." as this is a capability and
>>>>>>>>>>>>>>>>>>>> application chooses whether it wants to retain the headers
>>>>>>>>>>>>>>>>>>>> if the ODP is capable.
>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>> I'd like to see us all make it a priority to close on
>>>>>>>>>>>>>>>>>>>>> #197 next week. I'll post a doc update once we're all
>>>>>>>>>>>>>>>>>>>>> agreed on the precise semantics and operation flow. So
>>>>>>>>>>>>>>>>>>>>> I'm fine with holding off merging this PR until then.
>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>> Any update here? @NikhilA-Linaro, @bala-manoharan can
>>>>>>>>>>>>>>>>>>>>>> you please comment wrt your implementations?
>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>> Well, we do not have #197 merged, so it is too early to
>>>>>>>>>>>>>>>>>>>>>>> depend on that.
>>>>>>>>>>>>>>>>>>>>>>> Also, frankly speaking, this `sa_disabled` warning
>>>>>>>>>>>>>>>>>>>>>>> inside `odp_ipsec_result_t` is a backup plan. It is
>>>>>>>>>>>>>>>>>>>>>>> expected that most of the implementations will report
>>>>>>>>>>>>>>>>>>>>>>> this as a proper `ODP_IPSEC_STATUS` event, carrying
>>>>>>>>>>>>>>>>>>>>>>> this warning bit inside.
>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>> OK, changed in v6.
>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>> s/default //
>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>> OK, corrected in v5.
>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>> In lookaside mode soft limit expiration is reported
>>>>>>>>>>>>>>>>>>>>>>>>>>> as `warn` part of `ipsec_op_status` packet metadata.
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>> OK, in v4 I've added a new terminal state
>>>>>>>>>>>>>>>>>>>>>>>>>>>> `SA_Expired` to the FSM and have updated the doc
>>>>>>>>>>>>>>>>>>>>>>>>>>>> to say "expired" rather than "disabled". From the
>>>>>>>>>>>>>>>>>>>>>>>>>>>> expired state the only valid operation is
>>>>>>>>>>>>>>>>>>>>>>>>>>>> `odp_ipsec_sa_destroy()`.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yep. It is just not a 'disabled' state, because
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> we have separate definition for 'disabled SA'.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> IIRC, one can call it only once in NXP case.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> @NikhilA-Linaro, could you please comment?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> So how is this indicated in lookaside mode? The
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> whole point of ODP providing the limit support
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> is so the application doesn't have to track
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> byte/packet counts itself, so it's expected
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> that soft limit overruns will happen as part of
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> lookaside processing as well.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> If you let yourself run out of gas, the car
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> can stop at inconvenient times, which is why
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> one pays attention to that low fuel warning
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> light. A hard limit is a hard limit. That's
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> what makes it hard. Any other definition seems
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> confusingly fuzzy and unnecessary.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I thought the restriction is that you can
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> call this repeatedly as long as an SA hasn't
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> yet been created. I can change this (and the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> state diagram) if that's not the case.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> It's part of the `enum`. In this case L2
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> would effectively be None.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Only in OUT INLINE mode, if I remember the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> outcome of discussions correctly.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Ahem. It does not enter disabled state per
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> se:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> - It is an undefined behaviour (iow, an
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> application error) to submit packets to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> disabled SA
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> - It is a perfectly valid to submit
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> packets to SA after hard limit overrun
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (e.g. because other packets might be
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> already queued at this moment).
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> It is worth mentioning that depending on
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the implementation and application needs,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> inline processing might be enabled either
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> in both directions or in just one
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> direction.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> TBD: mention that it MUST be called at
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> most once per IPsec application.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> L2 does not make sense here, does it?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ... parsed after IPsec processing ...
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> TBD: describe that some IPsec packets
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> still might be reported via plain
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PktIO interface (e.g. because of SA
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> lookup failure). They can be
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> resubmitted to IPsec in lookaside
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> mode.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> If SA was not determined (because SA
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> lookup failed for inbound packet),
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> event will be sent to the default
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> queue.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ... resulting packet is sent back
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> serving as IPsec completion event
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ...
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ... in inbound inline operations.
https://github.com/Linaro/odp/pull/185#discussion_r150550552
updated_at 2017-11-13 14:20:42
