[dpdk-dev] [PATCH v3 1/2] lib/security: add support for get metadata

[Anoob]

Hi Akhil, Radu

PLease see inline.

Thanks,

Anoob


On 11/24/2017 05:04 PM, Akhil Goyal wrote:
> Hi Radu,
> On 11/24/2017 4:47 PM, Radu Nicolau wrote:
>>
>>
>> On 11/24/2017 10:55 AM, Akhil Goyal wrote:
>>> On 11/24/2017 3:09 PM, Radu Nicolau wrote:
>>>> Hi,
>>>>
>>>> Comment inline
>>>>
>>>>
>>>> On 11/24/2017 8:50 AM, Akhil Goyal wrote:
>>>>> Hi Anoob, Radu,
>>>>> On 11/23/2017 4:49 PM, 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, application could get 
>>>>>> metadata
>>>>>> from the packet which could be used to identify the security 
>>>>>> parameters
>>>>>> with which the packet was processed.
>>>>>>
>>>>>> Signed-off-by: Anoob Joseph <anoob.joseph at caviumnetworks.com>
>>>>>> ---
>>>>>> v3:
>>>>>> * Replaced 64 bit metadata in conf with (void *)userdata
>>>>>> * The API(rte_security_get_pkt_metadata) would return void * 
>>>>>> instead of
>>>>>>    uint64_t
>>>>>>
>>>>>> v2:
>>>>>> * Replaced get_session and get_cookie APIs with get_pkt_metadata API
>>>>>>
>>>>>>   lib/librte_security/rte_security.c        | 13 +++++++++++++
>>>>>>   lib/librte_security/rte_security.h        | 19 +++++++++++++++++++
>>>>>>   lib/librte_security/rte_security_driver.h | 16 ++++++++++++++++
>>>>>>   3 files changed, 48 insertions(+)
>>>>>>
>>>>>> diff --git a/lib/librte_security/rte_security.c 
>>>>>> b/lib/librte_security/rte_security.c
>>>>>> index 1227fca..a1d78b6 100644
>>>>>> --- a/lib/librte_security/rte_security.c
>>>>>> +++ b/lib/librte_security/rte_security.c
>>>>>> @@ -108,6 +108,19 @@ rte_security_set_pkt_metadata(struct 
>>>>>> rte_security_ctx *instance,
>>>>>>                              sess, m, params);
>>>>>>   }
>>>>>>   +void *
>>>>>> +rte_security_get_pkt_metadata(struct rte_security_ctx *instance,
>>>>>> +                  struct rte_mbuf *pkt)
>>>>> Can we rename pkt with m. Just to make it consistent with the set 
>>>>> API.
>>>>>> +{
>>>>>> +    void *md = NULL;
>>>>>> +
>>>>>> + RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->get_pkt_metadata, NULL);
>>>>>> +    if (instance->ops->get_pkt_metadata(instance->device, pkt, 
>>>>>> &md))
>>>>>> +        return NULL;
>>>>>> +
>>>>>> +    return md;
>>>>>> +}
>>>>>
>>>>> Pkt metadata should be set by user i.e. the application, and the 
>>>>> driver need not be aware of the format and the values of the 
>>>>> metadata.
>>>>> So setting the metadata in the driver and getting it back from the 
>>>>> driver does not look a good idea.
>>>>>
>>>>> Is it possible, that the application define the metadata on its 
>>>>> own and set it in the library itself without the call to the 
>>>>> driver ops.
My first patch was along those lines. Can you take a look at that and 
give your comments?

If we add this metadata in rte_security_session, we can achieve the 
above behavior without driver maintaining the metadata. But from the 
packet, application will have to first get the security session. And 
then application can get the metadata by calling "get metadata" with 
rte_security_session as the argument. So we will need a "get_session" 
API(which reaches the driver) and then do "get_app_metadata".
>>>> I'm not sure I understand here; even in our case (ixgbe) the driver 
>>>> sets the metadata and it is aware of the format - that is the whole 
>>>> idea. This is why we added the set_metadata API, to allow the 
>>>> driver to inject extra information into the mbuf, information that 
>>>> is driver specific and derived from the security session, so it 
>>>> makes sense to also have a symmetric get_metadata.
>>>> Private data is the one that follows those rules, i.e. application 
>>>> specific and driver transparent.
>>>
>>> As per my understanding of the user metadata, it should be in 
>>> control of the application, and the application shall know the 
>>> format of that. Setting in driver will disallow this.
>>> Please let me know if my understanding is incorrect.
Your understanding is correct. That' the requirement.
>>>
>>> If at all, some information is needed to be set on the basis of 
>>> driver, then application can get that information from the driver 
>>> and then set it in the packet metadata in its own way/format.
>>
>> The rte_security_set_pkt_metadata() doc defines the metadata as 
>> "device-specific defined metadata" and also takes a device specific 
>> params pointer, so the symmetric function is to be expected to work 
>> in the same way, i.e. return device specific metadata associated with 
>> the security session and instance and mbuf. How is this metadata 
>> stored is not specified in the security API, so the PMD 
>> implementation have the flexibility.
The requirement in this case isn't exactly parallel to 
"set_pkt_metadata". May be we can drop making it symmetric?
>>
>
> Yes it was defined that way and I did not noticed this one at the time 
> of it's implementation.
> Here, my point is that the application may be using mbuf udata for 
> it's own functionality, it should not be modified in the driver.
>
> However, if we need to do this, then we may need to clarify in the 
> documentation that for security, udata shall be set with the 
> rte_security_set_pkt_metadata() and not otherwise.
>
> -Akhil