Re: [dpdk-dev] [PATCH v4 1/3] security: support pdcp protocol

[Akhil Goyal <akhil.goyal@nxp.com>]

On 10/16/2018 12:10 PM, Joseph@dpdk.org wrote:
> Hi Akhil,
>
> https://tools.ietf.org/html/rfc4301#section-1
>
> RFC says we need to use "IPsec" and not "IPSec". Can you fix this in the
> lines you have added?
I will send a separate patch to correct it in the complete document.
>
> And do see inline for other comments.
>
> Thanks,
> Anoob
> On 15-10-2018 18:23, Akhil Goyal wrote:
>> External Email
>>
>> From: Akhil Goyal <akhil.goyal@nxp.com>
>>
>> Packet Data Convergence Protocol (PDCP) is added in rte_security
>> for 3GPP TS 36.323 for LTE.
>>
>> The patchset provide the structure definitions for configuring the
>> PDCP sessions and relevant documentation is added.
>>
>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
>> ---
>>    doc/guides/prog_guide/rte_security.rst | 107 +++++++++++++++++++++++--
>>    lib/librte_security/rte_security.c     |   4 +
>>    lib/librte_security/rte_security.h     |  91 +++++++++++++++++++++
>>    3 files changed, 195 insertions(+), 7 deletions(-)
>>
>> diff --git a/doc/guides/prog_guide/rte_security.rst b/doc/guides/prog_guide/rte_security.rst
>> index 0812abe77..f09e7c8bb 100644
>> --- a/doc/guides/prog_guide/rte_security.rst
>> +++ b/doc/guides/prog_guide/rte_security.rst
>> @@ -10,8 +10,8 @@ The security library provides a framework for management and provisioning
>>    of security protocol operations offloaded to hardware based devices. The
>>    library defines generic APIs to create and free security sessions which can
>>    support full protocol offload as well as inline crypto operation with
>> -NIC or crypto devices. The framework currently only supports the IPSec protocol
>> -and associated operations, other protocols will be added in future.
>> +NIC or crypto devices. The framework currently only supports the IPSec and PDCP
>> +protocol and associated operations, other protocols will be added in future.
>>
>>    Design Principles
>>    -----------------
>> @@ -253,6 +253,49 @@ for any protocol header addition.
>>            +--------|--------+
>>                     V
>>
>> +PDCP Flow Diagram
>> +~~~~~~~~~~~~~~~~~
>> +
>> +Based on 3GPP TS 36.323 Evolved Universal Terrestrial Radio Access (E-UTRA);
>> +Packet Data Convergence Protocol (PDCP) specification
>> +
>> +.. code-block:: c
>> +
>> +        Transmitting PDCP Entity          Receiving PDCP Entity
>> +                  |                                   ^
>> +                  |                       +-----------|-----------+
>> +                  V                       | In order delivery and |
>> +        +---------|----------+            | Duplicate detection   |
>> +        | Sequence Numbering |            |  (Data Plane only)    |
>> +        +---------|----------+            +-----------|-----------+
>> +                  |                                   |
>> +        +---------|----------+            +-----------|----------+
>> +        | Header Compression*|            | Header Decompression*|
>> +        | (Data-Plane only)  |            |   (Data Plane only)  |
>> +        +---------|----------+            +-----------|----------+
>> +                  |                                   |
>> +        +---------|-----------+           +-----------|----------+
>> +        | Integrity Protection|           |Integrity Verification|
>> +        | (Control Plane only)|           | (Control Plane only) |
>> +        +---------|-----------+           +-----------|----------+
>> +        +---------|-----------+            +----------|----------+
>> +        |     Ciphering       |            |     Deciphering     |
>> +        +---------|-----------+            +----------|----------+
>> +        +---------|-----------+            +----------|----------+
>> +        |   Add PDCP header   |            | Remove PDCP Header  |
>> +        +---------|-----------+            +----------|----------+
>> +                  |                                   |
>> +                  +----------------->>----------------+
>> +
>> +
>> +.. note::
>> +
>> +    * Header Compression and decompression are not supported currently.
>> +
>> +Just like IPSec, in case of PDCP also header addition/deletion, cipher/
>> +de-cipher, integrity protection/verification is done based on the action
>> +type chosen.
>> +
>>    Device Features and Capabilities
>>    ---------------------------------
>>
>> @@ -271,7 +314,7 @@ structure in the *DPDK API Reference*.
>>
>>    Each driver (crypto or ethernet) defines its own private array of capabilities
>>    for the operations it supports. Below is an example of the capabilities for a
>> -PMD which supports the IPSec protocol.
>> +PMD which supports the IPSec and PDCP protocol.
>>
>>    .. code-block:: c
>>
>> @@ -298,6 +341,24 @@ PMD which supports the IPSec protocol.
>>                    },
>>                    .crypto_capabilities = pmd_capabilities
>>            },
>> +        { /* PDCP Lookaside Protocol offload Data Plane */
>> +                .action = RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL,
>> +                .protocol = RTE_SECURITY_PROTOCOL_PDCP,
>> +                .pdcp = {
>> +                        .domain = RTE_SECURITY_PDCP_MODE_DATA,
>> +                        .capa_flags = 0
>> +                },
>> +                .crypto_capabilities = pmd_capabilities
>> +        },
>> +        { /* PDCP Lookaside Protocol offload Control */
>> +                .action = RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL,
>> +                .protocol = RTE_SECURITY_PROTOCOL_PDCP,
>> +                .pdcp = {
>> +                        .domain = RTE_SECURITY_PDCP_MODE_CONTROL,
>> +                        .capa_flags = 0
>> +                },
>> +                .crypto_capabilities = pmd_capabilities
>> +        },
>>            {
>>                    .action = RTE_SECURITY_ACTION_TYPE_NONE
>>            }
>> @@ -429,6 +490,7 @@ Security Session configuration structure is defined as ``rte_security_session_co
>>            union {
>>                    struct rte_security_ipsec_xform ipsec;
>>                    struct rte_security_macsec_xform macsec;
>> +                struct rte_security_pdcp_xform pdcp;
>>            };
>>            /**< Configuration parameters for security session */
>>            struct rte_crypto_sym_xform *crypto_xform;
>> @@ -463,15 +525,17 @@ The ``rte_security_session_protocol`` is defined as
>>    .. code-block:: c
>>
>>        enum rte_security_session_protocol {
>> -        RTE_SECURITY_PROTOCOL_IPSEC,
>> +        RTE_SECURITY_PROTOCOL_IPSEC = 1,
>>            /**< IPsec Protocol */
>>            RTE_SECURITY_PROTOCOL_MACSEC,
>>            /**< MACSec Protocol */
>> +        RTE_SECURITY_PROTOCOL_PDCP,
>> +        /**< PDCP Protocol */
>>        };
>>
>> -Currently the library defines configuration parameters for IPSec only. For other
>> -protocols like MACSec, structures and enums are defined as place holders which
>> -will be updated in the future.
>> +Currently the library defines configuration parameters for IPSec and PDCP only.
>> +For other protocols like MACSec, structures and enums are defined as place holders
>> +which will be updated in the future.
>>
>>    IPsec related configuration parameters are defined in ``rte_security_ipsec_xform``
>>
>> @@ -494,6 +558,35 @@ IPsec related configuration parameters are defined in ``rte_security_ipsec_xform
>>            /**< Tunnel parameters, NULL for transport mode */
>>        };
>>
>> +PDCP related configuration parameters are defined in ``rte_security_pdcp_xform``
>> +
>> +.. code-block:: c
>> +
>> +    struct rte_security_pdcp_xform {
>> +        int8_t bearer; /**< PDCP bearer ID */
>> +        /**< PDCP mode of operation: Control or data */
>> +        uint8_t en_ordering;
>> +        /**< Enable in order delivery, this field shall be set only if
>> +         * driver/HW is capable. See RTE_SECURITY_PDCP_ORDERING_CAP.
>> +         */
>> +        uint8_t remove_duplicates;
>> +        /**< Notify driver/HW to detect and remove duplicate packets.
>> +         * This field should be set only when driver/hw is capable.
>> +         * See RTE_SECURITY_PDCP_DUP_DETECT_CAP.
>> +         */
>> +        enum rte_security_pdcp_domain domain;
>> +        /**< PDCP Frame Direction 0:UL 1:DL */
>> +        enum rte_security_pdcp_direction pkt_dir;
>> +        /**< Sequence number size, 5/7/12/15/18 */
>> +        enum rte_security_pdcp_sn_size sn_size;
>> +        /**< Starting Hyper Frame Number to be used together with the SN
>> +         * from the PDCP frames
>> +         */
>> +        uint32_t hfn;
>> +        /**< HFN Threashold for key renegotiation */
>> +        uint32_t hfn_threshold;
>> +    };
>> +
>>
>>    Security API
>>    ~~~~~~~~~~~~
>> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
>> index 1954960a5..c6355de95 100644
>> --- a/lib/librte_security/rte_security.c
>> +++ b/lib/librte_security/rte_security.c
>> @@ -131,6 +131,10 @@ rte_security_capability_get(struct rte_security_ctx *instance,
>>                                           capability->ipsec.direction ==
>>                                                           idx->ipsec.direction)
>>                                           return capability;
>> +                       } else if (idx->protocol == RTE_SECURITY_PROTOCOL_PDCP) {
>> +                               if (capability->pdcp.domain ==
>> +                                                       idx->pdcp.domain)
>> +                                       return capability;
>>                           }
>>                   }
>>           }
>> diff --git a/lib/librte_security/rte_security.h b/lib/librte_security/rte_security.h
>> index b0d1b97ee..1d20530f4 100644
>> --- a/lib/librte_security/rte_security.h
>> +++ b/lib/librte_security/rte_security.h
>> @@ -206,6 +206,66 @@ struct rte_security_macsec_xform {
>>           int dummy;
>>    };
>>
>> +/**
>> + * PDCP Mode of session
>> + */
>> +enum rte_security_pdcp_domain {
>> +       RTE_SECURITY_PDCP_MODE_CONTROL, /**< PDCP control plane */
>> +       RTE_SECURITY_PDCP_MODE_DATA,    /**< PDCP data plane */
>> +};
>> +
>> +/** PDCP Frame direction */
>> +enum rte_security_pdcp_direction {
>> +       RTE_SECURITY_PDCP_UPLINK,       /**< Uplink */
>> +       RTE_SECURITY_PDCP_DOWNLINK,     /**< Downlink */
>> +};
>> +
>> +/**
>> + * PDCP Sequence Number Size selectors
>> + * @PDCP_SN_SIZE_5: 5bit sequence number
>> + * @PDCP_SN_SIZE_7: 7bit sequence number
>> + * @PDCP_SN_SIZE_12: 12bit sequence number
>> + * @PDCP_SN_SIZE_15: 15bit sequence number
>> + * @PDCP_SN_SIZE_18: 18bit sequence number
>> + */
>> +enum rte_security_pdcp_sn_size {
>> +       RTE_SECURITY_PDCP_SN_SIZE_5 = 5,
>> +       RTE_SECURITY_PDCP_SN_SIZE_7 = 7,
>> +       RTE_SECURITY_PDCP_SN_SIZE_12 = 12,
>> +       RTE_SECURITY_PDCP_SN_SIZE_15 = 15,
>> +       RTE_SECURITY_PDCP_SN_SIZE_18 = 18
>> +};
>> +
>> +/**
>> + * PDCP security association configuration data.
>> + *
>> + * This structure contains data required to create a PDCP security session.
>> + */
>> +struct rte_security_pdcp_xform {
>> +       int8_t bearer;  /**< PDCP bearer ID */
>> +       /**< PDCP mode of operation: Control or data */
>> +       uint8_t en_ordering;
>> +       /**< Enable in order delivery, this field shall be set only if
>> +        * driver/HW is capable. See RTE_SECURITY_PDCP_ORDERING_CAP.
>> +        */
>> +       uint8_t remove_duplicates;
>> +       /**< Notify driver/HW to detect and remove duplicate packets.
>> +        * This field should be set only when driver/hw is capable.
>> +        * See RTE_SECURITY_PDCP_DUP_DETECT_CAP.
>> +        */
>> +       enum rte_security_pdcp_domain domain;
>> +       /**< PDCP Frame Direction 0:UL 1:DL */
>> +       enum rte_security_pdcp_direction pkt_dir;
>> +       /**< Sequence number size, 5/7/12/15/18 */
>> +       enum rte_security_pdcp_sn_size sn_size;
>> +       /**< Starting Hyper Frame Number to be used together with the SN
>> +        * from the PDCP frames
>> +        */
>> +       uint32_t hfn;
>> +       /**< HFN Threshold for key renegotiation */
> The above comment is for which member?
this is for hfn_threshold. However, one comment it misplaced, will 
correct it.
>> +       uint32_t hfn_threshold;
>> +};
>> +
>>    /**
>>     * Security session action type.
>>     */
>> @@ -232,6 +292,8 @@ enum rte_security_session_protocol {
>>           /**< IPsec Protocol */
>>           RTE_SECURITY_PROTOCOL_MACSEC,
>>           /**< MACSec Protocol */
>> +       RTE_SECURITY_PROTOCOL_PDCP,
>> +       /**< PDCP Protocol */
>>    };
>>
>>    /**
>> @@ -246,6 +308,7 @@ struct rte_security_session_conf {
>>           union {
>>                   struct rte_security_ipsec_xform ipsec;
>>                   struct rte_security_macsec_xform macsec;
>> +               struct rte_security_pdcp_xform pdcp;
>>           };
>>           /**< Configuration parameters for security session */
>>           struct rte_crypto_sym_xform *crypto_xform;
>> @@ -413,6 +476,10 @@ struct rte_security_ipsec_stats {
>>
>>    };
>>
>> +struct rte_security_pdcp_stats {
>> +       uint64_t reserved;
>> +};
>> +
>>    struct rte_security_stats {
>>           enum rte_security_session_protocol protocol;
>>           /**< Security protocol to be configured */
>> @@ -421,6 +488,7 @@ struct rte_security_stats {
>>           union {
>>                   struct rte_security_macsec_stats macsec;
>>                   struct rte_security_ipsec_stats ipsec;
>> +               struct rte_security_pdcp_stats pdcp;
>>           };
>>    };
>>
>> @@ -465,6 +533,13 @@ struct rte_security_capability {
>>                           int dummy;
>>                   } macsec;
>>                   /**< MACsec capability */
>> +               struct {
>> +                       enum rte_security_pdcp_domain domain;
>> +                       /** < PDCP mode of operation: Control or data */
>> +                       uint32_t capa_flags;
>> +                       /** < Capabilitity flags, see RTE_SECURITY_PDCP_* */
>> +               } pdcp;
>> +               /**< PDCP capability */
>>           };
>>
>>           const struct rte_cryptodev_capabilities *crypto_capabilities;
>> @@ -474,6 +549,19 @@ struct rte_security_capability {
>>           /**< Device offload flags */
>>    };
>>
>> +/**< Underlying Hardware/driver which support PDCP may or may not support
>> + * packet ordering. Set RTE_SECURITY_PDCP_ORDERING_CAP if it support.
>> + * If it is not set, driver/HW assumes packets received are in order
>> + * and it will be application's responsibility to maintain ordering.
>> + */
>> +#define RTE_SECURITY_PDCP_ORDERING_CAP         0x00000001
> Would this flag contradict with RTE_SECURITY_TX_OLOAD_NEED_MDATA?
> Suppose if we have a security device which would do PDCP in inline mode,
> this would become a problem, right?
I think this would not, as I have defined a separate field in pdcp 
capability as capa_flags.
>> +
>> +/**< Underlying Hardware/driver which support PDCP may or may not detect
>> + * duplicate packet. Set RTE_SECURITY_PDCP_DUP_DETECT_CAP if it support.
>> + * If it is not set, driver/HW assumes there is no duplicate packet received.
>> + */
>> +#define RTE_SECURITY_PDCP_DUP_DETECT_CAP       0x00000002
>> +
>>    #define RTE_SECURITY_TX_OLOAD_NEED_MDATA       0x00000001
>>    /**< HW needs metadata update, see rte_security_set_pkt_metadata().
>>     */
>> @@ -506,6 +594,9 @@ struct rte_security_capability_idx {
>>                           enum rte_security_ipsec_sa_mode mode;
>>                           enum rte_security_ipsec_sa_direction direction;
>>                   } ipsec;
>> +               struct {
>> +                       enum rte_security_pdcp_domain domain;
missed the capa_flags in this one. Will add it. It is added in the 
rte_security_capability
>> +               } pdcp;
>>           };
>>    };
>>
>> --
>> 2.17.1
>>