[dpdk-dev] [PATCH 05/15] security: switch metadata to dynamic mbuf field

[Andrew Rybchenko]

On 10/26/20 8:20 AM, Thomas Monjalon wrote:
> The device-specific metadata was stored in the deprecated field udata64.
> It is moved to a dynamic mbuf field in order to allow removal of udata64.
> 
> Signed-off-by: Thomas Monjalon <thomas at monjalon.net>

[snip]

> diff --git a/drivers/net/ixgbe/ixgbe_ipsec.c b/drivers/net/ixgbe/ixgbe_ipsec.c
> index 48f5082d49..0232db20ed 100644
> --- a/drivers/net/ixgbe/ixgbe_ipsec.c
> +++ b/drivers/net/ixgbe/ixgbe_ipsec.c
> @@ -484,7 +484,8 @@ ixgbe_crypto_update_mb(void *device __rte_unused,
>  			get_sec_session_private_data(session);
>  	if (ic_session->op == IXGBE_OP_AUTHENTICATED_ENCRYPTION) {
>  		union ixgbe_crypto_tx_desc_md *mdata =
> -			(union ixgbe_crypto_tx_desc_md *)&m->udata64;
> +			(union ixgbe_crypto_tx_desc_md *)
> +			rte_security_dynfield(m);

IMHO alignment looks a bit confusing here, may be add one more
TAB?

>  		mdata->enc = 1;
>  		mdata->sa_idx = ic_session->sa_index;
>  		mdata->pad_len = ixgbe_crypto_compute_pad_len(m);
> @@ -751,5 +752,7 @@ ixgbe_ipsec_ctx_create(struct rte_eth_dev *dev)
>  			return -ENOMEM;
>  		}
>  	}
> +	if (rte_security_dynfield_register() < 0)
> +		return -rte_errno;
>  	return 0;
>  }

[snip]

> diff --git a/examples/ipsec-secgw/ipsec_worker.c b/examples/ipsec-secgw/ipsec_worker.c
> index b6c851f257..72f698893d 100644
> --- a/examples/ipsec-secgw/ipsec_worker.c
> +++ b/examples/ipsec-secgw/ipsec_worker.c
> @@ -208,7 +208,8 @@ process_ipsec_ev_inbound(struct ipsec_ctx *ctx, struct route_table *rt,
>  					"Inbound security offload failed\n");
>  				goto drop_pkt_and_exit;
>  			}
> -			sa = pkt->userdata;
> +			sa = RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset,
> +					struct ipsec_sa *);

I think it should be de-reference above, i.e.
sa = (struct ipsec_sa *)*RTE_MBUF_DYNFIELD(pkt,
security_dynfield_offset, RTE_SECURITY_DYNFIELD_TYPE *);
or just
sa = *RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset, struct ipsec_sa **);

and why not rte_security_dynfield()?

It all looks very fragile. May be at least add
   RTE_BUILD_BUG_ON(sizeof(RTE_SECURITY_DYNFIELD_TYPE) ==
                    sizeof(ipsec_sa *));
and similar checks when an application or a library does
lookup for a dynamic field.

In general since lookup should not happen on data path,
may be lookup should return size of the field which must
be checked by the caller for consistency.

>  		}
>  
>  		/* Check if we have a match */
> @@ -226,7 +227,8 @@ process_ipsec_ev_inbound(struct ipsec_ctx *ctx, struct route_table *rt,
>  					"Inbound security offload failed\n");
>  				goto drop_pkt_and_exit;
>  			}
> -			sa = pkt->userdata;
> +			sa = RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset,
> +					struct ipsec_sa *);

same

>  		}
>  
>  		/* Check if we have a match */
> @@ -357,7 +359,8 @@ process_ipsec_ev_outbound(struct ipsec_ctx *ctx, struct route_table *rt,
>  	}
>  
>  	if (sess->security.ol_flags & RTE_SECURITY_TX_OLOAD_NEED_MDATA)
> -		pkt->userdata = sess->security.ses;
> +		*RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset,
> +			struct rte_security_session **) = sess->security.ses;

rte_security_dynfield() ?

Is it really different types of value in one example
application? It looks like it should be different
dynamic fields. Otherwise, I don't understand how
to work with it.

In fact may be above is out of scope of the patch series...

>  
>  	/* Mark the packet for Tx security offload */
>  	pkt->ol_flags |= PKT_TX_SEC_OFFLOAD;
> @@ -465,7 +468,9 @@ ipsec_wrkr_non_burst_int_port_drv_mode(struct eh_event_link_info *links,
>  			}
>  
>  			/* Save security session */
> -			pkt->userdata = sess_tbl[port_id];
> +			*RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset,
> +					struct rte_security_session **) =
> +				sess_tbl[port_id];

rte_security_dynfield() ?

>  
>  			/* Mark the packet for Tx security offload */
>  			pkt->ol_flags |= PKT_TX_SEC_OFFLOAD;

[snip]