From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id A2F67237 for ; Tue, 21 Nov 2017 11:15:35 +0100 (CET) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Nov 2017 02:15:34 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,432,1505804400"; d="scan'208,217";a="178376056" Received: from rnicolau-mobl.ger.corp.intel.com (HELO [10.237.221.73]) ([10.237.221.73]) by fmsmga005.fm.intel.com with ESMTP; 21 Nov 2017 02:15:32 -0800 To: Anoob Joseph , Akhil Goyal , Declan Doherty , Sergio Gonzalez Monroy Cc: Narayana Prasad , Jerin Jacob , dev@dpdk.org References: <1511173905-22117-1-git-send-email-anoob.joseph@caviumnetworks.com> <1511173905-22117-2-git-send-email-anoob.joseph@caviumnetworks.com> <906693a1-f1d3-4986-6f53-619d120ef075@caviumnetworks.com> <003db6e1-e0a1-257d-230d-da6a3cf09ab1@intel.com> From: Radu Nicolau Message-ID: Date: Tue, 21 Nov 2017 10:15:32 +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: Content-Language: en-US Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH 1/2] lib/security: add support for saving app cookie 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: Tue, 21 Nov 2017 10:15:36 -0000 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 >>>>> --- >>>>>   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(+) >>>>> >>> >> > I'll rework the patch to include your suggestions. I'll send a v2 > after doing this. > > Thanks for the feedback, > Anoob >