From mboxrd@z Thu Jan 1 00:00:00 1970 From: Akhil Goyal Subject: Re: [PATCH v3 1/2] lib/security: add support for get metadata Date: Fri, 24 Nov 2017 17:33:47 +0530 Message-ID: <7f193513-bfdb-81dc-48b4-ee844928745d@nxp.com> References: <1511333716-11955-1-git-send-email-anoob.joseph@caviumnetworks.com> <1511435969-5887-1-git-send-email-anoob.joseph@caviumnetworks.com> <1511435969-5887-2-git-send-email-anoob.joseph@caviumnetworks.com> <414c3491-7a54-09b4-0cde-a21bffaeb1ab@intel.com> <94ae34d4-b34a-22a1-e787-e2dcdffc8575@nxp.com> <63e76b4d-3e6a-bd74-3ad8-72fcacef0fcb@nxp.com> <45060b96-9b19-fe62-7653-5ad734188c93@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Cc: Jerin Jacob , Narayana Prasad , To: Radu Nicolau , Anoob Joseph , Declan Doherty , Sergio Gonzalez Monroy Return-path: Received: from NAM03-CO1-obe.outbound.protection.outlook.com (mail-co1nam03on0089.outbound.protection.outlook.com [104.47.40.89]) by dpdk.org (Postfix) with ESMTP id EF3532B83 for ; Fri, 24 Nov 2017 13:03:53 +0100 (CET) In-Reply-To: <45060b96-9b19-fe62-7653-5ad734188c93@intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >>>>>>> --- >>>>>>> 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. >>> >> >> 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