[dpdk-dev,v3,1/2] lib/security: add support for get metadata
Checks
Commit Message
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(+)
Comments
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.
> +
> const struct rte_security_capability *
> rte_security_capabilities_get(struct rte_security_ctx *instance)
> {
> diff --git a/lib/librte_security/rte_security.h b/lib/librte_security/rte_security.h
> index 653929b..35c306a 100644
> --- a/lib/librte_security/rte_security.h
> +++ b/lib/librte_security/rte_security.h
> @@ -274,6 +274,8 @@ struct rte_security_session_conf {
> /**< Configuration parameters for security session */
> struct rte_crypto_sym_xform *crypto_xform;
> /**< Security Session Crypto Transformations */
> + void *userdata;
> + /**< Application specific metadata */
> };
>
> struct rte_security_session {
> @@ -346,6 +348,23 @@ rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
> struct rte_mbuf *mb, void *params);
>
> /**
> + * Get metadata from the packet. This returns metadata associated with the
> + * security session which processed the packet.
> + *
> + * This is valid only for inline processed ingress packets.
> + *
> + * @param instance security instance
> + * @param pkt packet mbuf
> + *
> + * @return
> + * - On success, metadata
> + * - On failure, NULL
> + */
> +void *
> +rte_security_get_pkt_metadata(struct rte_security_ctx *instance,
> + struct rte_mbuf *pkt);
> +
> +/**
> * Attach a session to a symmetric crypto operation
> *
> * @param sym_op crypto operation
> diff --git a/lib/librte_security/rte_security_driver.h b/lib/librte_security/rte_security_driver.h
> index 997fbe7..561ae83 100644
> --- a/lib/librte_security/rte_security_driver.h
> +++ b/lib/librte_security/rte_security_driver.h
> @@ -122,6 +122,20 @@ typedef int (*security_set_pkt_metadata_t)(void *device,
> void *params);
>
> /**
> + * Get metadata from the packet.
> + *
> + * @param device Crypto/eth device pointer
> + * @param pkt Packet mbuf
> + * @param mt Pointer to receive metadata
> + *
> + * @return
> + * - Returns 0 if metadata is retrieved successfully.
> + * - Returns -ve value for errors.
> + */
> +typedef int (*security_get_pkt_metadata_t)(void *device,
> + struct rte_mbuf *pkt, void **md);
> +
> +/**
> * Get security capabilities of the device.
> *
> * @param device crypto/eth device pointer
> @@ -145,6 +159,8 @@ struct rte_security_ops {
> /**< Clear a security sessions private data. */
> security_set_pkt_metadata_t set_pkt_metadata;
> /**< Update mbuf metadata. */
> + security_get_pkt_metadata_t get_pkt_metadata;
> + /**< Get metadata from packet. */
> security_capabilities_get_t capabilities_get;
> /**< Get security capabilities. */
> };
>
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.
>
>> +
>> const struct rte_security_capability *
>> rte_security_capabilities_get(struct rte_security_ctx *instance)
>> {
>> diff --git a/lib/librte_security/rte_security.h
>> b/lib/librte_security/rte_security.h
>> index 653929b..35c306a 100644
>> --- a/lib/librte_security/rte_security.h
>> +++ b/lib/librte_security/rte_security.h
>> @@ -274,6 +274,8 @@ struct rte_security_session_conf {
>> /**< Configuration parameters for security session */
>> struct rte_crypto_sym_xform *crypto_xform;
>> /**< Security Session Crypto Transformations */
>> + void *userdata;
>> + /**< Application specific metadata */
>> };
>> struct rte_security_session {
>> @@ -346,6 +348,23 @@ rte_security_set_pkt_metadata(struct
>> rte_security_ctx *instance,
>> struct rte_mbuf *mb, void *params);
>> /**
>> + * Get metadata from the packet. This returns metadata associated
>> with the
>> + * security session which processed the packet.
>> + *
>> + * This is valid only for inline processed ingress packets.
>> + *
>> + * @param instance security instance
>> + * @param pkt packet mbuf
>> + *
>> + * @return
>> + * - On success, metadata
>> + * - On failure, NULL
>> + */
>> +void *
>> +rte_security_get_pkt_metadata(struct rte_security_ctx *instance,
>> + struct rte_mbuf *pkt);
>> +
>> +/**
>> * Attach a session to a symmetric crypto operation
>> *
>> * @param sym_op crypto operation
>> diff --git a/lib/librte_security/rte_security_driver.h
>> b/lib/librte_security/rte_security_driver.h
>> index 997fbe7..561ae83 100644
>> --- a/lib/librte_security/rte_security_driver.h
>> +++ b/lib/librte_security/rte_security_driver.h
>> @@ -122,6 +122,20 @@ typedef int (*security_set_pkt_metadata_t)(void
>> *device,
>> void *params);
>> /**
>> + * Get metadata from the packet.
>> + *
>> + * @param device Crypto/eth device pointer
>> + * @param pkt Packet mbuf
>> + * @param mt Pointer to receive metadata
>> + *
>> + * @return
>> + * - Returns 0 if metadata is retrieved successfully.
>> + * - Returns -ve value for errors.
>> + */
>> +typedef int (*security_get_pkt_metadata_t)(void *device,
>> + struct rte_mbuf *pkt, void **md);
>> +
>> +/**
>> * Get security capabilities of the device.
>> *
>> * @param device crypto/eth device pointer
>> @@ -145,6 +159,8 @@ struct rte_security_ops {
>> /**< Clear a security sessions private data. */
>> security_set_pkt_metadata_t set_pkt_metadata;
>> /**< Update mbuf metadata. */
>> + security_get_pkt_metadata_t get_pkt_metadata;
>> + /**< Get metadata from packet. */
>> security_capabilities_get_t capabilities_get;
>> /**< Get security capabilities. */
>> };
>>
>
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.
-Akhil
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.
>
> -Akhil
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.
>
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.
-Akhil
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.
>>
>
> 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
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.
>>>
>>
>> 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
Hi Akhil, Radu
PLease see inline.
Thanks,
Anoob
On 11/24/2017 05:04 PM, 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.
My first patch was along those lines. Can you take a look at that and
give your comments?
If we add this metadata in rte_security_session, we can achieve the
above behavior without driver maintaining the metadata. But from the
packet, application will have to first get the security session. And
then application can get the metadata by calling "get metadata" with
rte_security_session as the argument. So we will need a "get_session"
API(which reaches the driver) and then do "get_app_metadata".
>>>> 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.
Your understanding is correct. That' the requirement.
>>>
>>> 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.
The requirement in this case isn't exactly parallel to
"set_pkt_metadata". May be we can drop making it symmetric?
>>
>
> 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.
>
> -Akhil
Hi Akhil, Radu,
Is there any update on how we should approach this?
Thanks,
Anoob
On 11/24/2017 05:52 PM, Anoob wrote:
> Hi Akhil, Radu
>
> Please see inline.
>
> Thanks,
>
> Anoob
>
>
> On 11/24/2017 05:04 PM, 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.
> My first patch was along those lines. Can you take a look at that and
> give your comments?
>
> If we add this metadata in rte_security_session, we can achieve the
> above behavior without driver maintaining the metadata. But from the
> packet, application will have to first get the security session. And
> then application can get the metadata by calling "get metadata" with
> rte_security_session as the argument. So we will need a "get_session"
> API(which reaches the driver) and then do "get_app_metadata".
>>>>> 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.
> Your understanding is correct. That' the requirement.
>>>>
>>>> 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.
> The requirement in this case isn't exactly parallel to
> "set_pkt_metadata". May be we can drop making it symmetric?
>>>
>>
>> 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.
>>
>> -Akhil
>
Hi Anoob,
On 11/24/2017 5:52 PM, Anoob wrote:
> Hi Akhil, Radu
>
> PLease see inline.
>
> Thanks,
>
> Anoob
>
>
> On 11/24/2017 05:04 PM, 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.
> My first patch was along those lines. Can you take a look at that and
> give your comments?
>
> If we add this metadata in rte_security_session, we can achieve the
> above behavior without driver maintaining the metadata. But from the
> packet, application will have to first get the security session. And
> then application can get the metadata by calling "get metadata" with
> rte_security_session as the argument. So we will need a "get_session"
> API(which reaches the driver) and then do "get_app_metadata".
In that case also, the application cannot set metadata independently.
It will rather become more complex.
It is better that we document this properly in the documentation as
discussed in my/Radu's previous mail.
>>>>> 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.
> Your understanding is correct. That' the requirement.
>>>>
>>>> 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.
> The requirement in this case isn't exactly parallel to
> "set_pkt_metadata". May be we can drop making it symmetric?
>>>
>>
>> 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.
>>
>> -Akhil
>
>
Hi Akhil,
See inline.
Thanks,
Anoob
On 12/04/2017 02:58 PM, Akhil Goyal wrote:
> Hi Anoob,
>
> On 11/24/2017 5:52 PM, Anoob wrote:
>> Hi Akhil, Radu
>>
>> PLease see inline.
>>
>> Thanks,
>>
>> Anoob
>>
>>
>> On 11/24/2017 05:04 PM, 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.
>> My first patch was along those lines. Can you take a look at that and
>> give your comments?
>>
>> If we add this metadata in rte_security_session, we can achieve the
>> above behavior without driver maintaining the metadata. But from the
>> packet, application will have to first get the security session. And
>> then application can get the metadata by calling "get metadata" with
>> rte_security_session as the argument. So we will need a "get_session"
>> API(which reaches the driver) and then do "get_app_metadata".
> In that case also, the application cannot set metadata independently.
> It will rather become more complex.
> It is better that we document this properly in the documentation as
> discussed in my/Radu's previous mail.
I'll update the documentation with this behavior and will send a new patch.
>>>>>> 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.
>> Your understanding is correct. That' the requirement.
>>>>>
>>>>> 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.
>> The requirement in this case isn't exactly parallel to
>> "set_pkt_metadata". May be we can drop making it symmetric?
>>>>
>>>
>>> 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.
>>>
>>> -Akhil
>>
>>
>
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 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
>
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.
>>>>>
>>>>
>>>> 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
>>
>
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.
>>>>>>
>>>>>
>>>>> 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
>>>
>>
>
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
>>>>
>>>
>>
>
>
Hi Akhil, Radu
On 12/12/2017 02:25 PM, Akhil Goyal wrote:
> 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.
Will document the limitation of udata64's usage. Since we are
documenting that udata64 will have some device defined metadata, do we
need another API call for getting the "metadata"
(rte_security_get_pkt_metadata). The driver can set this metadata in
udata64 field, and in ingress, the userdata could be obtained with a
single API call (rte_security_get_userdata(*instance, udata64)).
The application will be aware that the udata64 in the mbuf will be the
metadata from the receive side, and userdata can be retrieved only with
that. If application need to use the udata64 field, it should save this
rx metadata. Userdata can be obtained anytime by passing this metadata
to the driver.
To summarize,
1) udata64 of the ingress traffic will be device-specific metadata
(documentation change)
2) Pass this field to rte_security_get_userdata() to get back the
application registered pointer.
Is this fine?
>
>>>>>>>>
>>>>>>>
>>>>>>> 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
>>>>>
>>>>
>>>
>>
>>
>
On 12/12/2017 7:20 PM, Anoob Joseph wrote:
> Hi Akhil, Radu
>
>
> On 12/12/2017 02:25 PM, Akhil Goyal wrote:
>> 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.
> Will document the limitation of udata64's usage. Since we are
> documenting that udata64 will have some device defined metadata, do we
> need another API call for getting the "metadata"
> (rte_security_get_pkt_metadata). The driver can set this metadata in
> udata64 field, and in ingress, the userdata could be obtained with a
> single API call (rte_security_get_userdata(*instance, udata64)).
>
> The application will be aware that the udata64 in the mbuf will be the
> metadata from the receive side, and userdata can be retrieved only with
> that. If application need to use the udata64 field, it should save this
> rx metadata. Userdata can be obtained anytime by passing this metadata
> to the driver.
>
> To summarize,
> 1) udata64 of the ingress traffic will be device-specific metadata
> (documentation change)
> 2) Pass this field to rte_security_get_userdata() to get back the
> application registered pointer.
>
> Is this fine?
It looks ok as of now.
>
>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> 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
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>
@@ -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)
+{
+ 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;
+}
+
const struct rte_security_capability *
rte_security_capabilities_get(struct rte_security_ctx *instance)
{
@@ -274,6 +274,8 @@ struct rte_security_session_conf {
/**< Configuration parameters for security session */
struct rte_crypto_sym_xform *crypto_xform;
/**< Security Session Crypto Transformations */
+ void *userdata;
+ /**< Application specific metadata */
};
struct rte_security_session {
@@ -346,6 +348,23 @@ rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
struct rte_mbuf *mb, void *params);
/**
+ * Get metadata from the packet. This returns metadata associated with the
+ * security session which processed the packet.
+ *
+ * This is valid only for inline processed ingress packets.
+ *
+ * @param instance security instance
+ * @param pkt packet mbuf
+ *
+ * @return
+ * - On success, metadata
+ * - On failure, NULL
+ */
+void *
+rte_security_get_pkt_metadata(struct rte_security_ctx *instance,
+ struct rte_mbuf *pkt);
+
+/**
* Attach a session to a symmetric crypto operation
*
* @param sym_op crypto operation
@@ -122,6 +122,20 @@ typedef int (*security_set_pkt_metadata_t)(void *device,
void *params);
/**
+ * Get metadata from the packet.
+ *
+ * @param device Crypto/eth device pointer
+ * @param pkt Packet mbuf
+ * @param mt Pointer to receive metadata
+ *
+ * @return
+ * - Returns 0 if metadata is retrieved successfully.
+ * - Returns -ve value for errors.
+ */
+typedef int (*security_get_pkt_metadata_t)(void *device,
+ struct rte_mbuf *pkt, void **md);
+
+/**
* Get security capabilities of the device.
*
* @param device crypto/eth device pointer
@@ -145,6 +159,8 @@ struct rte_security_ops {
/**< Clear a security sessions private data. */
security_set_pkt_metadata_t set_pkt_metadata;
/**< Update mbuf metadata. */
+ security_get_pkt_metadata_t get_pkt_metadata;
+ /**< Get metadata from packet. */
security_capabilities_get_t capabilities_get;
/**< Get security capabilities. */
};