From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radu Nicolau Subject: Re: [PATCH 1/2] lib/security: add support for saving app cookie Date: Tue, 21 Nov 2017 10:15:32 +0000 Message-ID: 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> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: Narayana Prasad , Jerin Jacob , dev@dpdk.org To: Anoob Joseph , Akhil Goyal , Declan Doherty , Sergio Gonzalez Monroy 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) In-Reply-To: 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/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 >