Re: [dpdk-dev] [PATCH 1/2] lib/security: add support for saving app cookie

[Radu Nicolau <radu.nicolau@intel.com>]

On 11/20/2017 7:09 PM, Anoob Joseph wrote:
>
> Hi
>
> See inline.
>
> On 20-11-2017 23:19, Radu Nicolau wrote:
>> Hi
>>
>>
>> On 11/20/2017 3:32 PM, Anoob wrote:
>>> Hi,
>>>
>>> Having something like "get_pkt_metadata" should be fine for inline 
>>> protocol usage. But we will still need a "get cookie" call to get 
>>> the cookie, as the application would need something it can interpret.
>> Why can't you have a get_pkt_metadata that returns the "cookie" - 
>> which I would call it userdata or similar? What I'm trying to say is 
>> that we should try to keep it as generic as possible. For example, I 
>> wouldn't assume that the cookie is stored in pkt->udata64 in the 
>> application.
> I agree to your suggestion. The only problem is in the asymmetry of 
> how we would set the "cookie" (or userdata) and get it back. Right now 
> we are passing this as a member in conf. Do you have any thoughts on 
> that? For a more meaningful approach, we could pass this as another 
> argument in create_session API. I was thinking of a scenario of 
> supporting more items. So added it in the structure.
>
> Naming is open for suggestions. I'll use userdata instead.
I think keeping it in the conf is best, but it can be either way.

>>> And, even though it seems both are symmetric operations(get & set 
>>> pkt metadata), there are some minor differences in what they would 
>>> do. Set metadata would be setting metadata(udata64 member) in mbuf, 
>>> while get metadata is not exactly returning metadata. We use the 
>>> actual metadata to get something else(security session in the 
>>> proposed patch). That was the primary motive for adding 
>>> "session_get" API.
>> What they do exactly is left to the PMD implementation. From the 
>> user's perspective, it does not matter.
>> There is no requirement that set pkt metadata will set the udata64 
>> member.
> May be I misunderstood the terminology. "udata64" in mbuf was 
> documented as
> |RTE_STD_C11 union { void *userdata; /**< Can be used for external 
> metadata */ uint64_t udata64; /**< Allow 8-byte userdata on 32-bit */ };|
>
> I thought the metadata in set_pkt_metadata was coming from this. And 
> we were setting this member itself in ixgbe driver.
We're using it in the ixgbe because it is the most obvious choice when 
there is only a small data set to be passed (an table index in this 
case) but it was intended to allow the driver to implement any behavior 
necessary.

>
> But yes, it makes sense not to expose it that way. The API can take 
> mbuf pointer and then, the PMD could dictate how it had set the 
> metadata in rx path.
>
>>>
>>> Thanks,
>>> Anoob
>>>
>>> On 11/20/2017 05:42 PM, Radu Nicolau wrote:
>>>> Hi,
>>>>
>>>>
>>>> Why not have something similar to rte_security_set_pkt_metadata, 
>>>> for example:
>>>>
>>>> void *
>>>> rte_security_get_pkt_metadata(struct rte_security_ctx *instance,
>>>>                   struct rte_mbuf *mb);
>>>>
>>>> and keep internally in the PMD all the required references. The 
>>>> returned value will be device-specific, so it's flexible enough to 
>>>> include anything required (just as void *params is in the 
>>>> set_pkt_metadata).
>>>>
>>>> I think it will make a cleaner and more consistent implementation.
>>>>
>>>>
>>>> Regards,
>>>>
>>>> Radu
>>>>
>>>>
>>>>
>>>> On 11/20/2017 10:31 AM, Anoob Joseph wrote:
>>>>> In case of inline protocol processed ingress traffic, the packet 
>>>>> may not
>>>>> have enough information to determine the security parameters with 
>>>>> which
>>>>> the packet was processed. In such cases, the application could 
>>>>> register
>>>>> a cookie, which will be saved in the the security session.
>>>>>
>>>>> As the ingress packets are received in the application, it will have
>>>>> some metadata set in the mbuf. Application can pass this metadata to
>>>>> "rte_security_session_get" API to retrieve the security session. Once
>>>>> the security session is determined, another driver call with the
>>>>> security session will give the application the cookie it had 
>>>>> registered.
>>>>>
>>>>> The cookie will be registered while creating the security session.
>>>>> Without the cookie, the selector check (SP-SA check) for the incoming
>>>>> IPsec traffic won't be possible in the application.
>>>>>
>>>>> Application can choose what it should register as the cookie. It can
>>>>> register SPI or a pointer to SA.
>>>>>
>>>>> Signed-off-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>
>>>>> ---
>>>>>   lib/librte_security/rte_security.c        | 26 
>>>>> +++++++++++++++++++++++
>>>>>   lib/librte_security/rte_security.h        | 30 
>>>>> +++++++++++++++++++++++++++
>>>>>   lib/librte_security/rte_security_driver.h | 34 
>>>>> +++++++++++++++++++++++++++++++
>>>>>   3 files changed, 90 insertions(+)
>>>>> <snip>
>>>
>>
> I'll rework the patch to include your suggestions. I'll send a v2 
> after doing this.
>
> Thanks for the feedback,
> Anoob
>