[dpdk-dev,05/11] lib/librte_mbuf: add security crypto flags and mbuf fields
Checks
Commit Message
From: Boris Pismenny <borisp@mellanox.com>
add security crypto flags and update mbuf fields to support
IPsec crypto offload for transmitted packets, and to indicate
crypto result for received packets.
Signed-off-by: Aviad Yehezkel <aviadye@mellanox.com>
Signed-off-by: Boris Pismenny <borisp@mellanox.com>
Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
---
lib/librte_mbuf/rte_mbuf.c | 6 ++++++
lib/librte_mbuf/rte_mbuf.h | 32 +++++++++++++++++++++++++++++---
2 files changed, 35 insertions(+), 3 deletions(-)
Comments
Hi Olivier,
On 9/14/2017 11:27 AM, Akhil Goyal wrote:
>
> From: Boris Pismenny <borisp@mellanox.com>
>
> add security crypto flags and update mbuf fields to support
> IPsec crypto offload for transmitted packets, and to indicate
> crypto result for received packets.
>
> Signed-off-by: Aviad Yehezkel <aviadye@mellanox.com>
> Signed-off-by: Boris Pismenny <borisp@mellanox.com>
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> ---
> lib/librte_mbuf/rte_mbuf.c | 6 ++++++
> lib/librte_mbuf/rte_mbuf.h | 32 +++++++++++++++++++++++++++++---
> 2 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 26a62b8..bbd42a6 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -323,6 +323,8 @@ const char *rte_get_rx_ol_flag_name(uint64_t mask)
> case PKT_RX_QINQ_STRIPPED: return "PKT_RX_QINQ_STRIPPED";
> case PKT_RX_LRO: return "PKT_RX_LRO";
> case PKT_RX_TIMESTAMP: return "PKT_RX_TIMESTAMP";
> + case PKT_RX_SEC_OFFLOAD: return "PKT_RX_SECURITY_OFFLOAD";
> + case PKT_RX_SEC_OFFLOAD_FAILED: return
> "PKT_RX_SECURITY_OFFLOAD_FAILED";
> default: return NULL;
> }
> }
> @@ -358,6 +360,8 @@ rte_get_rx_ol_flag_list(uint64_t mask, char *buf,
> size_t buflen)
> { PKT_RX_QINQ_STRIPPED, PKT_RX_QINQ_STRIPPED, NULL },
> { PKT_RX_LRO, PKT_RX_LRO, NULL },
> { PKT_RX_TIMESTAMP, PKT_RX_TIMESTAMP, NULL },
> + { PKT_RX_SEC_OFFLOAD, PKT_RX_SEC_OFFLOAD, NULL },
> + { PKT_RX_SEC_OFFLOAD_FAILED,
> PKT_RX_SEC_OFFLOAD_FAILED, NULL },
> };
> const char *name;
> unsigned int i;
> @@ -410,6 +414,7 @@ const char *rte_get_tx_ol_flag_name(uint64_t mask)
> case PKT_TX_TUNNEL_GENEVE: return "PKT_TX_TUNNEL_GENEVE";
> case PKT_TX_TUNNEL_MPLSINUDP: return
> "PKT_TX_TUNNEL_MPLSINUDP";
> case PKT_TX_MACSEC: return "PKT_TX_MACSEC";
> + case PKT_TX_SEC_OFFLOAD: return "PKT_TX_SECURITY_OFFLOAD";
> default: return NULL;
> }
> }
> @@ -443,6 +448,7 @@ rte_get_tx_ol_flag_list(uint64_t mask, char *buf,
> size_t buflen)
> { PKT_TX_TUNNEL_MPLSINUDP, PKT_TX_TUNNEL_MASK,
> "PKT_TX_TUNNEL_NONE" },
> { PKT_TX_MACSEC, PKT_TX_MACSEC, NULL },
> + { PKT_TX_SEC_OFFLOAD, PKT_TX_SEC_OFFLOAD, NULL },
> };
> const char *name;
> unsigned int i;
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index eaed7ee..9ce61ae 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -189,11 +189,26 @@ extern "C" {
> */
> #define PKT_RX_TIMESTAMP (1ULL << 17)
>
> +/**
> + * Indicate that security offload processing was applied on the RX packet.
> + */
> +#define PKT_RX_SEC_OFFLOAD (1ULL << 18)
> +
> +/**
> + * Indicate that security offload processing failed on the RX packet.
> + */
> +#define PKT_RX_SEC_OFFLOAD_FAILED (1ULL << 19)
> +
> /* add new RX flags here */
>
> /* add new TX flags here */
>
> /**
> + * Request security offload processing on the TX packet.
> + */
> +#define PKT_TX_SEC_OFFLOAD (1ULL << 43)
> +
> +/**
> * Offload the MACsec. This flag must be set by the application to enable
> * this offload feature for a packet to be transmitted.
> */
> @@ -316,7 +331,8 @@ extern "C" {
> PKT_TX_QINQ_PKT | \
> PKT_TX_VLAN_PKT | \
> PKT_TX_TUNNEL_MASK | \
> - PKT_TX_MACSEC)
> + PKT_TX_MACSEC | \
> + PKT_TX_SEC_OFFLOAD)
>
> #define __RESERVED (1ULL << 61) /**< reserved for future mbuf use */
>
> @@ -456,8 +472,18 @@ struct rte_mbuf {
> uint32_t l3_type:4; /**< (Outer) L3 type. */
> uint32_t l4_type:4; /**< (Outer) L4 type. */
> uint32_t tun_type:4; /**< Tunnel type. */
> - uint32_t inner_l2_type:4; /**< Inner L2 type. */
> - uint32_t inner_l3_type:4; /**< Inner L3 type. */
> + RTE_STD_C11
> + union {
> + uint8_t inner_esp_next_proto;
> +
> + __extension__
> + struct {
> + uint8_t inner_l2_type:4;
> + /**< Inner L2 type. */
> + uint8_t inner_l3_type:4;
> + /**< Inner L3 type. */
> + };
> + };
> uint32_t inner_l4_type:4; /**< Inner L4 type. */
> };
> };
What do you think about this change to mbuf?
It doesn't increase the mbuf size and it replaces some fields that have no meaning
in IPsec encapsulations (inner L2 and L3) with a meaningful field of the correct
size (inner_esp_next_proto - 8 bytes).
We later use this for IPsec offload on both Tx and Rx to indicate the packet format.
Hi Boris,
Some comments inline.
On Mon, Sep 18, 2017 at 07:54:03AM +0000, Boris Pismenny wrote:
> Hi Olivier,
>
> On 9/14/2017 11:27 AM, Akhil Goyal wrote:
> >
> > From: Boris Pismenny <borisp@mellanox.com>
> >
> > add security crypto flags and update mbuf fields to support
> > IPsec crypto offload for transmitted packets, and to indicate
> > crypto result for received packets.
> >
> > Signed-off-by: Aviad Yehezkel <aviadye@mellanox.com>
> > Signed-off-by: Boris Pismenny <borisp@mellanox.com>
> > Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> > ---
> > lib/librte_mbuf/rte_mbuf.c | 6 ++++++
> > lib/librte_mbuf/rte_mbuf.h | 32 +++++++++++++++++++++++++++++---
> > 2 files changed, 35 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > index 26a62b8..bbd42a6 100644
> > --- a/lib/librte_mbuf/rte_mbuf.c
> > +++ b/lib/librte_mbuf/rte_mbuf.c
> > @@ -323,6 +323,8 @@ const char *rte_get_rx_ol_flag_name(uint64_t mask)
> > case PKT_RX_QINQ_STRIPPED: return "PKT_RX_QINQ_STRIPPED";
> > case PKT_RX_LRO: return "PKT_RX_LRO";
> > case PKT_RX_TIMESTAMP: return "PKT_RX_TIMESTAMP";
> > + case PKT_RX_SEC_OFFLOAD: return "PKT_RX_SECURITY_OFFLOAD";
> > + case PKT_RX_SEC_OFFLOAD_FAILED: return
> > "PKT_RX_SECURITY_OFFLOAD_FAILED";
I think the string should be the same than the macro.
(SEC vs SECURITY)
> ...
> > +/**
> > + * Indicate that security offload processing was applied on the RX packet.
> > + */
> > +#define PKT_RX_SEC_OFFLOAD (1ULL << 18)
> > +
> > +/**
> > + * Indicate that security offload processing failed on the RX packet.
> > + */
> > +#define PKT_RX_SEC_OFFLOAD_FAILED (1ULL << 19)
> > +
If the presence of these flags implies that some fields are
valid (ex: inner_esp_next_proto), it should be specified in
the API comments.
> ...
> > @@ -456,8 +472,18 @@ struct rte_mbuf {
> > uint32_t l3_type:4; /**< (Outer) L3 type. */
> > uint32_t l4_type:4; /**< (Outer) L4 type. */
> > uint32_t tun_type:4; /**< Tunnel type. */
> > - uint32_t inner_l2_type:4; /**< Inner L2 type. */
> > - uint32_t inner_l3_type:4; /**< Inner L3 type. */
> > + RTE_STD_C11
> > + union {
> > + uint8_t inner_esp_next_proto;
> > +
> > + __extension__
> > + struct {
> > + uint8_t inner_l2_type:4;
> > + /**< Inner L2 type. */
> > + uint8_t inner_l3_type:4;
> > + /**< Inner L3 type. */
> > + };
> > + };
> > uint32_t inner_l4_type:4; /**< Inner L4 type. */
> > };
> > };
The (quite useless) API comment is missing. I think we should
have it for consistency.
Can you please also detail in which conditions inner_esp_next_proto is
valid, and when inner_l2/l3_type is valid?
> What do you think about this change to mbuf?
>
> It doesn't increase the mbuf size and it replaces some fields that have no meaning
> in IPsec encapsulations (inner L2 and L3) with a meaningful field of the correct
> size (inner_esp_next_proto - 8 bytes).
>
> We later use this for IPsec offload on both Tx and Rx to indicate the packet format.
>
Strangely, the abi-checker script finds an abi change. To me, it looks
like a false positive of abi-checker. The html output says:
Field inner_l2_type has been removed from this type.
Applications will access incorrect memory when attempting to access this field.
Field inner_l3_type has been removed from this type.
Applications will access incorrect memory when attempting to access this field.
[−] affected symbols: 3 (15.8%)
__rte_pktmbuf_read ( struct rte_mbuf const* m, uint32_t off, uint32_t len, void* buf ) @@ DPDK_16.11
Field 'm.unnamed1.unnamed0' in 1st parameter 'm' (pointer) has type 'anon-struct-rte_mbuf.h-454'.
rte_mbuf_sanity_check ( struct rte_mbuf const* m, int is_header ) @@ DPDK_2.0
Field 'm.unnamed1.unnamed0' in 1st parameter 'm' (pointer) has type 'anon-struct-rte_mbuf.h-454'.
rte_pktmbuf_dump ( FILE* f, struct rte_mbuf const* m, unsigned int dump_len ) @@ DPDK_2.0
Field 'm.unnamed1.unnamed0' in 2nd parameter 'm' (pointer) has type 'anon-struct-rte_mbuf.h-454'.
If someone has a better explanation :)
You can reproduce it with the following patch:
http://dpdk.org/dev/patchwork/patch/28985/
However, with pahole, we can check that the sizes/offsets are
correct and also, the following test program behaves as expected:
#include <stdio.h>
#include <stdint.h>
#include <string.h>
#include <unistd.h>
#include <sys/mman.h>
struct mbuf1 {
union {
uint32_t packet_type;
struct {
uint32_t l2_type:4;
uint32_t l3_type:4;
uint32_t l4_type:4;
uint32_t tun_type:4;
uint32_t inner_l2_type:4;
uint32_t inner_l3_type:4;
uint32_t inner_l4_type:4;
};
};
};
struct mbuf2 {
union {
uint32_t packet_type;
struct {
uint32_t l2_type:4;
uint32_t l3_type:4;
uint32_t l4_type:4;
uint32_t tun_type:4;
union {
uint8_t inner_esp_next_proto;
struct {
uint8_t inner_l2_type:4;
uint8_t inner_l3_type:4;
};
};
uint32_t inner_l4_type:4;
};
};
};
int main(void)
{
struct mbuf1 m1;
struct mbuf2 m2;
m1.l2_type = 0x1;
m1.l3_type = 0x2;
m1.l4_type = 0x3;
m1.tun_type = 0x4;
m1.inner_l2_type = 0x5;
m1.inner_l3_type = 0x6;
m1.inner_l4_type = 0x7;
printf("m1.l2_type=%x\n", m1.l2_type);
printf("m1.l3_type=%x\n", m1.l3_type);
printf("m1.l4_type=%x\n", m1.l4_type);
printf("m1.tun_type=%x\n", m1.tun_type);
printf("m1.inner_l2_type=%x\n", m1.inner_l2_type);
printf("m1.inner_l3_type=%x\n", m1.inner_l3_type);
printf("m1.inner_l4_type=%x\n", m1.inner_l4_type);
memcpy(&m2, &m1, sizeof(m2));
printf("m2.l2_type=%x\n", m2.l2_type);
printf("m2.l3_type=%x\n", m2.l3_type);
printf("m2.l4_type=%x\n", m2.l4_type);
printf("m2.tun_type=%x\n", m2.tun_type);
printf("m2.inner_l2_type=%x\n", m2.inner_l2_type);
printf("m2.inner_l3_type=%x\n", m2.inner_l3_type);
printf("m2.inner_l4_type=%x\n", m2.inner_l4_type);
return 0;
}
Olivier
Hi Olivier,
Thanks for the feedback. I'll submit the fixes for our V2 of this series.
Some comments inline.
Olivier Matz wrote:
>
> Hi Boris,
>
> Some comments inline.
>
> On Mon, Sep 18, 2017 at 07:54:03AM +0000, Boris Pismenny wrote:
> > Hi Olivier,
> >
> > On 9/14/2017 11:27 AM, Akhil Goyal wrote:
> > >
> > > From: Boris Pismenny <borisp@mellanox.com>
> > >
> > > add security crypto flags and update mbuf fields to support
> > > IPsec crypto offload for transmitted packets, and to indicate
> > > crypto result for received packets.
> > >
> > > Signed-off-by: Aviad Yehezkel <aviadye@mellanox.com>
> > > Signed-off-by: Boris Pismenny <borisp@mellanox.com>
> > > Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> > > ---
> > > lib/librte_mbuf/rte_mbuf.c | 6 ++++++
> > > lib/librte_mbuf/rte_mbuf.h | 32 +++++++++++++++++++++++++++++---
> > > 2 files changed, 35 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > > index 26a62b8..bbd42a6 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.c
> > > +++ b/lib/librte_mbuf/rte_mbuf.c
> > > @@ -323,6 +323,8 @@ const char *rte_get_rx_ol_flag_name(uint64_t
> mask)
> > > case PKT_RX_QINQ_STRIPPED: return "PKT_RX_QINQ_STRIPPED";
> > > case PKT_RX_LRO: return "PKT_RX_LRO";
> > > case PKT_RX_TIMESTAMP: return "PKT_RX_TIMESTAMP";
> > > + case PKT_RX_SEC_OFFLOAD: return "PKT_RX_SECURITY_OFFLOAD";
> > > + case PKT_RX_SEC_OFFLOAD_FAILED: return
> > > "PKT_RX_SECURITY_OFFLOAD_FAILED";
>
> I think the string should be the same than the macro.
> (SEC vs SECURITY)
>
> > ...
> > > +/**
> > > + * Indicate that security offload processing was applied on the RX packet.
> > > + */
> > > +#define PKT_RX_SEC_OFFLOAD (1ULL << 18)
> > > +
> > > +/**
> > > + * Indicate that security offload processing failed on the RX packet.
> > > + */
> > > +#define PKT_RX_SEC_OFFLOAD_FAILED (1ULL << 19)
> > > +
>
> If the presence of these flags implies that some fields are
> valid (ex: inner_esp_next_proto), it should be specified in
> the API comments.
>
> > ...
> > > @@ -456,8 +472,18 @@ struct rte_mbuf {
> > > uint32_t l3_type:4; /**< (Outer) L3 type. */
> > > uint32_t l4_type:4; /**< (Outer) L4 type. */
> > > uint32_t tun_type:4; /**< Tunnel type. */
> > > - uint32_t inner_l2_type:4; /**< Inner L2 type. */
> > > - uint32_t inner_l3_type:4; /**< Inner L3 type. */
> > > + RTE_STD_C11
> > > + union {
> > > + uint8_t inner_esp_next_proto;
> > > +
> > > + __extension__
> > > + struct {
> > > + uint8_t inner_l2_type:4;
> > > + /**< Inner L2 type. */
> > > + uint8_t inner_l3_type:4;
> > > + /**< Inner L3 type. */
> > > + };
> > > + };
> > > uint32_t inner_l4_type:4; /**< Inner L4 type. */
> > > };
> > > };
>
> The (quite useless) API comment is missing. I think we should
> have it for consistency.
>
> Can you please also detail in which conditions inner_esp_next_proto is
> valid, and when inner_l2/l3_type is valid?
>
It is valid when the packet has an ESP header with plaintext data.
As explained below, ESP packets have no inner_l2 and possibly no inner_l3
So these meaningless fields are replaced with a meaningful field for ESP packets.
I'll add a new PTYPE_TUNNEL_ESP to indicate that the inner_esp_next_proto
is valid.
We will use this field later in the ipsec-secgw example for both Rx and Tx to
indicate the inner ESP protocol type in packets whose trailer is added/removed
by HW.
Thanks,
Boris.
@@ -323,6 +323,8 @@ const char *rte_get_rx_ol_flag_name(uint64_t mask)
case PKT_RX_QINQ_STRIPPED: return "PKT_RX_QINQ_STRIPPED";
case PKT_RX_LRO: return "PKT_RX_LRO";
case PKT_RX_TIMESTAMP: return "PKT_RX_TIMESTAMP";
+ case PKT_RX_SEC_OFFLOAD: return "PKT_RX_SECURITY_OFFLOAD";
+ case PKT_RX_SEC_OFFLOAD_FAILED: return "PKT_RX_SECURITY_OFFLOAD_FAILED";
default: return NULL;
}
}
@@ -358,6 +360,8 @@ rte_get_rx_ol_flag_list(uint64_t mask, char *buf, size_t buflen)
{ PKT_RX_QINQ_STRIPPED, PKT_RX_QINQ_STRIPPED, NULL },
{ PKT_RX_LRO, PKT_RX_LRO, NULL },
{ PKT_RX_TIMESTAMP, PKT_RX_TIMESTAMP, NULL },
+ { PKT_RX_SEC_OFFLOAD, PKT_RX_SEC_OFFLOAD, NULL },
+ { PKT_RX_SEC_OFFLOAD_FAILED, PKT_RX_SEC_OFFLOAD_FAILED, NULL },
};
const char *name;
unsigned int i;
@@ -410,6 +414,7 @@ const char *rte_get_tx_ol_flag_name(uint64_t mask)
case PKT_TX_TUNNEL_GENEVE: return "PKT_TX_TUNNEL_GENEVE";
case PKT_TX_TUNNEL_MPLSINUDP: return "PKT_TX_TUNNEL_MPLSINUDP";
case PKT_TX_MACSEC: return "PKT_TX_MACSEC";
+ case PKT_TX_SEC_OFFLOAD: return "PKT_TX_SECURITY_OFFLOAD";
default: return NULL;
}
}
@@ -443,6 +448,7 @@ rte_get_tx_ol_flag_list(uint64_t mask, char *buf, size_t buflen)
{ PKT_TX_TUNNEL_MPLSINUDP, PKT_TX_TUNNEL_MASK,
"PKT_TX_TUNNEL_NONE" },
{ PKT_TX_MACSEC, PKT_TX_MACSEC, NULL },
+ { PKT_TX_SEC_OFFLOAD, PKT_TX_SEC_OFFLOAD, NULL },
};
const char *name;
unsigned int i;
@@ -189,11 +189,26 @@ extern "C" {
*/
#define PKT_RX_TIMESTAMP (1ULL << 17)
+/**
+ * Indicate that security offload processing was applied on the RX packet.
+ */
+#define PKT_RX_SEC_OFFLOAD (1ULL << 18)
+
+/**
+ * Indicate that security offload processing failed on the RX packet.
+ */
+#define PKT_RX_SEC_OFFLOAD_FAILED (1ULL << 19)
+
/* add new RX flags here */
/* add new TX flags here */
/**
+ * Request security offload processing on the TX packet.
+ */
+#define PKT_TX_SEC_OFFLOAD (1ULL << 43)
+
+/**
* Offload the MACsec. This flag must be set by the application to enable
* this offload feature for a packet to be transmitted.
*/
@@ -316,7 +331,8 @@ extern "C" {
PKT_TX_QINQ_PKT | \
PKT_TX_VLAN_PKT | \
PKT_TX_TUNNEL_MASK | \
- PKT_TX_MACSEC)
+ PKT_TX_MACSEC | \
+ PKT_TX_SEC_OFFLOAD)
#define __RESERVED (1ULL << 61) /**< reserved for future mbuf use */
@@ -456,8 +472,18 @@ struct rte_mbuf {
uint32_t l3_type:4; /**< (Outer) L3 type. */
uint32_t l4_type:4; /**< (Outer) L4 type. */
uint32_t tun_type:4; /**< Tunnel type. */
- uint32_t inner_l2_type:4; /**< Inner L2 type. */
- uint32_t inner_l3_type:4; /**< Inner L3 type. */
+ RTE_STD_C11
+ union {
+ uint8_t inner_esp_next_proto;
+
+ __extension__
+ struct {
+ uint8_t inner_l2_type:4;
+ /**< Inner L2 type. */
+ uint8_t inner_l3_type:4;
+ /**< Inner L3 type. */
+ };
+ };
uint32_t inner_l4_type:4; /**< Inner L4 type. */
};
};