From: Akhil Goyal <akhil.goyal@nxp.com>
To: Anoob <anoob.joseph@caviumnetworks.com>,
Radu Nicolau <radu.nicolau@intel.com>,
Declan Doherty <declan.doherty@intel.com>,
Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
Cc: Jerin Jacob <jerin.jacob@caviumnetworks.com>,
Narayana Prasad <narayanaprasad.athreya@caviumnetworks.com>,
<dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v3 1/2] lib/security: add support for get metadata
Date: Tue, 12 Dec 2017 14:25:33 +0530 [thread overview]
Message-ID: <f2cac47e-3c19-8327-0f53-ce5b17d88a39@nxp.com> (raw)
In-Reply-To: <b4519f2a-45bc-9c36-6f91-cb8a56cbe6cf@caviumnetworks.com>
Hi Anoob,
On 12/11/2017 12:51 PM, Anoob wrote:
> Hi Akhil,
>
> Can you confirm if you are fine with the approach explained inline.
>
> Thanks,
> Anoob
>
> On 12/06/2017 03:13 PM, Radu Nicolau wrote:
>> Hi,
>>
>>
>> On 12/6/2017 7:30 AM, Anoob wrote:
>>> Hi Akhil, Radu,
>>>
>>> Please see inline.
>>>
>>> Thanks,
>>>
>>> Anoob
>>>
>>>
>>> On 11/24/2017 05:33 PM, Akhil Goyal wrote:
>>>> On 11/24/2017 5:29 PM, Radu Nicolau wrote:
>>>>>
>>>>>
>>>>> On 11/24/2017 11:34 AM, 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@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.
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> 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.
>>> Is rte_security_get_pkt_metadata() expected to return a "device
>>> specific" pointer? If that's the case, we would need another call
>>> (something like, rte_security_get_userdata()) to get back the
>>> userdata, right? Or is it fine, if the application assumes it will
>>> get userdata (the one passed in conf while creating security session)
>>> with rte_security_get_pkt_metadata()?
>> Yes, this will be my assumption, a "device specific" pointer (similar
>> to the "void *params" parameter of the rte_security_set_pkt_metadata
>> function), which will contain an arbitrary defined structure that will
>> be decoded by calling a PMD defined function.
>> But I think Akhil has a different view on this.
I am ok with the approach, if we are adding this as a limitation of
using udata in the documentation for inline cases.
The ideal approach should be such that driver should not be knowing the
content of the udata. But, if we cannot do away with it, we can mention
it in the documentation.
>>>>>>>
>>>>>>
>>>>>> 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.
>>>>> Indeed, we should update the doc stating that the set_metadata may
>>>>> change the mbuf userdata field so the application should use only
>>>>> private data if needed.
>>>>
>>>> Agreed, but it is dependent on which driver/mode(inline or
>>>> lookaside), it will be used.
>>>> Lookaside may not need this API as of now. Other implementations may
>>>> also don't require. So this shall be documented that way.
>>>>
>>>> -Akhil
>>>>
>>>
>>
>
>
next prev parent reply other threads:[~2017-12-12 8:55 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-20 10:31 [dpdk-dev] [PATCH 0/2] add inline protocol support Anoob Joseph
2017-11-20 10:31 ` [dpdk-dev] [PATCH 1/2] lib/security: add support for saving app cookie Anoob Joseph
2017-11-20 12:12 ` Radu Nicolau
2017-11-20 15:32 ` Anoob
2017-11-20 17:49 ` Radu Nicolau
2017-11-20 19:09 ` Anoob Joseph
2017-11-21 10:15 ` Radu Nicolau
2017-11-20 10:31 ` [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: add support for inline protocol Anoob Joseph
2017-11-22 6:55 ` [dpdk-dev] [PATCH v2 0/2] add inline protocol support Anoob Joseph
2017-11-22 6:55 ` [dpdk-dev] [PATCH v2 1/2] lib/security: add support for get metadata Anoob Joseph
2017-11-22 11:29 ` Radu Nicolau
2017-11-22 11:52 ` Anoob
2017-11-22 12:12 ` Radu Nicolau
2017-11-22 13:27 ` Neil Horman
2017-11-22 14:13 ` Anoob
2017-11-27 13:55 ` Neil Horman
2017-11-22 6:55 ` [dpdk-dev] [PATCH v2 2/2] examples/ipsec-secgw: add support for inline protocol Anoob Joseph
2017-11-22 12:21 ` [dpdk-dev] [PATCH v2 0/2] add inline protocol support Nelio Laranjeiro
2017-11-22 12:55 ` Anoob
2017-11-22 13:05 ` Nelio Laranjeiro
2017-11-22 13:38 ` Anoob
2017-11-22 13:53 ` Anoob
2017-11-22 15:13 ` Anoob
2017-11-22 15:25 ` Nelio Laranjeiro
2017-11-23 11:19 ` [dpdk-dev] [PATCH v3 " Anoob Joseph
2017-11-23 11:19 ` [dpdk-dev] [PATCH v3 1/2] lib/security: add support for get metadata Anoob Joseph
2017-11-24 8:50 ` Akhil Goyal
2017-11-24 9:39 ` Radu Nicolau
2017-11-24 10:55 ` Akhil Goyal
2017-11-24 11:17 ` Radu Nicolau
2017-11-24 11:34 ` Akhil Goyal
2017-11-24 11:59 ` Radu Nicolau
2017-11-24 12:03 ` Akhil Goyal
2017-12-06 7:30 ` Anoob
2017-12-06 9:43 ` Radu Nicolau
2017-12-11 7:21 ` Anoob
2017-12-12 8:55 ` Akhil Goyal [this message]
2017-12-12 13:50 ` Anoob Joseph
2017-12-13 14:38 ` Akhil Goyal
2017-11-24 12:22 ` Anoob
2017-11-29 5:43 ` Anoob
2017-12-04 9:28 ` Akhil Goyal
2017-12-04 10:16 ` Anoob
2017-11-23 11:19 ` [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add support for inline protocol Anoob Joseph
2017-12-11 11:02 ` Radu Nicolau
2017-12-15 8:30 ` [dpdk-dev] [PATCH v4 0/2] add inline protocol support Anoob Joseph
2017-12-15 8:30 ` [dpdk-dev] [PATCH v4 1/2] lib/security: add support for get userdata Anoob Joseph
2017-12-15 8:30 ` [dpdk-dev] [PATCH v4 2/2] examples/ipsec-secgw: add support for inline protocol Anoob Joseph
2017-12-15 8:43 ` [dpdk-dev] [PATCH v5 0/2] add inline protocol support Anoob Joseph
2017-12-15 8:43 ` [dpdk-dev] [PATCH v5 1/2] lib/security: add support for get userdata Anoob Joseph
2017-12-15 10:01 ` Akhil Goyal
2017-12-15 10:53 ` Anoob Joseph
2017-12-15 10:58 ` Akhil Goyal
2017-12-15 8:43 ` [dpdk-dev] [PATCH v5 2/2] examples/ipsec-secgw: add support for inline protocol Anoob Joseph
2017-12-15 9:39 ` Nelio Laranjeiro
2017-12-15 11:03 ` Anoob Joseph
2017-12-15 13:35 ` Nelio Laranjeiro
2017-12-15 10:04 ` Akhil Goyal
2017-12-15 11:16 ` Anoob Joseph
2017-12-18 7:15 ` [dpdk-dev] [PATCH v6 0/2] add inline protocol support Anoob Joseph
2017-12-18 7:15 ` [dpdk-dev] [PATCH v6 1/2] lib/security: add support for get userdata Anoob Joseph
2017-12-18 7:34 ` Akhil Goyal
2017-12-18 7:15 ` [dpdk-dev] [PATCH v6 2/2] examples/ipsec-secgw: add support for inline protocol Anoob Joseph
2018-01-08 16:10 ` De Lara Guarch, Pablo
2018-01-09 9:12 ` Akhil Goyal
2018-01-16 11:00 ` Nicolau, Radu
2018-01-09 16:05 ` [dpdk-dev] [PATCH v6 0/2] add inline protocol support De Lara Guarch, Pablo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f2cac47e-3c19-8327-0f53-ce5b17d88a39@nxp.com \
--to=akhil.goyal@nxp.com \
--cc=anoob.joseph@caviumnetworks.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
--cc=jerin.jacob@caviumnetworks.com \
--cc=narayanaprasad.athreya@caviumnetworks.com \
--cc=radu.nicolau@intel.com \
--cc=sergio.gonzalez.monroy@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).