Re: [dpdk-dev] [PATCH 05/11] lib/librte_mbuf: add security crypto flags and mbuf fields

[Olivier MATZ <olivier.matz@6wind.com>]

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