[dpdk-dev] [PATCH 05/11] lib/librte_mbuf: add security crypto flags and mbuf fields
Olivier MATZ
olivier.matz at 6wind.com
Wed Sep 20 11:43:49 CEST 2017
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 at 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 at mellanox.com>
> > Signed-off-by: Boris Pismenny <borisp at mellanox.com>
> > Signed-off-by: Radu Nicolau <radu.nicolau at 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
More information about the dev
mailing list