From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 727D0223 for ; Fri, 24 Nov 2017 12:59:40 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Nov 2017 03:59:39 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,447,1505804400"; d="scan'208";a="5332399" Received: from rnicolau-mobl.ger.corp.intel.com (HELO [10.237.221.73]) ([10.237.221.73]) by FMSMGA003.fm.intel.com with ESMTP; 24 Nov 2017 03:59:37 -0800 To: Akhil Goyal , Anoob Joseph , Declan Doherty , Sergio Gonzalez Monroy Cc: Jerin Jacob , Narayana Prasad , dev@dpdk.org 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> From: Radu Nicolau Message-ID: <45060b96-9b19-fe62-7653-5ad734188c93@intel.com> Date: Fri, 24 Nov 2017 11:59:37 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <63e76b4d-3e6a-bd74-3ad8-72fcacef0fcb@nxp.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH v3 1/2] lib/security: add support for get metadata X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Nov 2017 11:59:42 -0000 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. > > -Akhil