> On May 4, 2020, at 3:59 AM, Neale Ranns via lists.fd.io
> wrote:
>
>
> Hi Chris,
>
> With SAs there are two scenarios to consider for inflight packets
> 1) the SA is unlinked
> 2) the SA is deleted.
>
> We've talked at length about how to deal with 2).
> By 'unlinked' I mean that whatever config dictated that an SA be used has now
> gone (like tunnel protection or SPD policy). An inflight packet that is
> processed by an IPSec node would (by either scheme we discussed for 1))
> retrieve the SA do encrypt/decrypt and then attempt to send the packet on its
> merry way; this is the point at which it could fail. I say 'could' because it
> depends on how the unlink affected the vlib graph. In today's tunnel
> protection esp_encrpyt does vnet_feature_next(), this is not going to end
> well once esp_encrypt is no longer a feature on the arc. In tomorrow's tunnel
> protection we'll change that:
> https://gerrit.fd.io/r/c/vpp/+/26265
> and it should be safe. But, what if the next API removes the tunnel whilst
> there are still inflight packets? Is it still safe? Is it still correct to
> send encrypted tunnelled packets?
It's safe to send in-flight encrypted packets for an SA that was deleted after
they were encrypted, this reduces to buffering. After the SA is deleted new
packets won't follow that path (they won't have the index+gen associate with
them at all b/c of policy). If the user configures things such that packets
which used to be encrypted should no longer encrypted, well then that's what
they asked for for *newly arived* packets.
The security issue to be careful of here would be to have in-flight packets
that progressed through valid policy such that they were "in the tunnel" and
waiting to be encrypted, and then send them un-encrypted. Hopefully the change
you reference above does not do that.
>
> I think I'm coming round to the opinion that the safest way to approach this
> is to ensure that if the SA can be found, whatever state it is in (created,
> unlinked or deleted) then it needs to have a flag that states whether it
> should be used or the packet dropped. We'd update this state when the SA is
> [un]linked, with the barrier held.
Why does the SA object needs to be kept around? The indexes generation number
being unequal is enough to say the object is deleted. Per the concern I
mentioned above, we should make sure the packet doesn't *forget* that its been
associated with that an index+generation number (i.e., it's in the tunnel), but
that's orthogonal to whether the SA state itself is still in the pool.
>
> On a somewhat related topic, you probably saw:
> https://gerrit.fd.io/r/c/vpp/+/26811
> as an example of getting MP safe APIs wrong.
I made a similar change locally back when we started talking about this. :)
Thanks,
Chris.
> /neale
>
> On 24/04/2020 16:34, "Christian Hopps" wrote:
>
>
>Hi Neale,
>
>Comments also inline...
>
>Neale Ranns (nranns) writes:
>
>> Hi Chris,
>>
>> Comments inline...
>>
>> On 15/04/2020 15:14, "Christian Hopps" wrote:
>>
>>Hi Neale,
>>
>>I agree that something like 4, is probably the correct approach. I had a
>> side-meeting with some of the ARM folks (Govind and Honnappa), and we
>> thought using a generation number for the state rather than just waiting
>> "long-enough" to recycle it could work. The generation number would be the
>> atomic value associated with the state. So consider this API:
>>
>> - MP-safe pools store generation numbers alongside each object.
>> - When you allocate a new object from the pool you get an index and
>> generation number.
>> - When storing the object index you also save the generation number.
>> - When getting a pointer to the object you pass the API the index and
>> generation number and it will return NULL if the generation number did not
>> match the one stored with the object in the pool.
>> - When you delete a pool object its generation number is incremented
>> (with barrier).
>>
>>The size of the generation number needs to be large enough to guarantee
>> there is no wrap with objects still in the system that have stored the
>> generation number. Technically this is a "long-enough" aspect of the scheme.
>> :) One could imagine using less than 64 bits for the combination of index
>> and generation, if that was important.
>>
>> It's a good scheme, I like it.
>> I assume the pool indices would be 64 bit and the separation between vector
>> index and generation would be hidden from the user. Maybe a 32 bit value
>> would suffice in most cases, but why skimp...
>
>I was thinking to keep the index and generation number separate at the
> most basic API, to allow for selecting the size of the each independently and
> for efficient storage. I'm thinking for some applications one might want to
> do something like
>
>cacheline_packed_struct {
>...
>u32 foo_index;
>u32 bar_index;
>
