[dpdk-dev] [PATCH 1/2] security: enforce semantics for Tx inline processing

[Ananyev, Konstantin]

Adding more rte_security and PMD maintainers into the loop.

> > > > > > > > > > > For Tx inline processing, when RTE_SECURITY_TX_OLOAD_NEED_MDATA is
> > > > > > > > > > > set, rte_security_set_pkt_metadata() needs to be called for pkts
> > > > > > > > > > > to associate a Security session with a mbuf before submitting
> > > > > > > > > > > to Ethdev Tx. This is apart from setting PKT_TX_SEC_OFFLOAD in
> > > > > > > > > > > mbuf.ol_flags. rte_security_set_pkt_metadata() is also used to
> > > > > > > > > > > set some opaque metadata in mbuf for PMD's use.
> > > > > > > > > > > This patch updates documentation that rte_security_set_pkt_metadata()
> > > > > > > > > > > should be called only with mbuf containing Layer 3 and above data.
> > > > > > > > > > > This behaviour is consistent with existing PMD's such as ixgbe.
> > > > > > > > > > >
> > > > > > > > > > > On Tx, not all net PMD's/HW can parse packet and identify
> > > > > > > > > > > L2 header and L3 header locations on Tx. This is inline with other
> > > > > > > > > > > Tx offloads requirements such as L3 checksum, L4 checksum offload,
> > > > > > > > > > > etc, where mbuf.l2_len, mbuf.l3_len etc, needs to be set for
> > > > > > > > > > > HW to be able to generate checksum. Since Inline IPSec is also
> > > > > > > > > > > such a Tx offload, some PMD's at least need mbuf.l2_len to be
> > > > > > > > > > > valid to find L3 header and perform Outbound IPSec processing.
> > > > > > > > > > > Hence, this patch updates documentation to enforce setting
> > > > > > > > > > > mbuf.l2_len while setting PKT_TX_SEC_OFFLOAD in mbuf.ol_flags
> > > > > > > > > > > for Inline IPSec Crypto / Protocol offload processing to
> > > > > > > > > > > work on Tx.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Nithin Dabilpuram <ndabilpuram at marvell.com>
> > > > > > > > > > > Reviewed-by: Akhil Goyal <gakhil at marvell.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  doc/guides/nics/features.rst           | 2 ++
> > > > > > > > > > >  doc/guides/prog_guide/rte_security.rst | 6 +++++-
> > > > > > > > > > >  lib/mbuf/rte_mbuf_core.h               | 2 ++
> > > > > > > > > > >  3 files changed, 9 insertions(+), 1 deletion(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
> > > > > > > > > > > index 403c2b03a..414baf14f 100644
> > > > > > > > > > > --- a/doc/guides/nics/features.rst
> > > > > > > > > > > +++ b/doc/guides/nics/features.rst
> > > > > > > > > > > @@ -430,6 +430,7 @@ of protocol operations. See Security library and PMD documentation for more deta
> > > > > > > > > > >
> > > > > > > > > > >  * **[uses]       rte_eth_rxconf,rte_eth_rxmode**: ``offloads:DEV_RX_OFFLOAD_SECURITY``,
> > > > > > > > > > >  * **[uses]       rte_eth_txconf,rte_eth_txmode**: ``offloads:DEV_TX_OFFLOAD_SECURITY``.
> > > > > > > > > > > +* **[uses]       mbuf**: ``mbuf.l2_len``.
> > > > > > > > > > >  * **[implements] rte_security_ops**: ``session_create``, ``session_update``,
> > > > > > > > > > >    ``session_stats_get``, ``session_destroy``, ``set_pkt_metadata``, ``capabilities_get``.
> > > > > > > > > > >  * **[provides] rte_eth_dev_info**: ``rx_offload_capa,rx_queue_offload_capa:DEV_RX_OFFLOAD_SECURITY``,
> > > > > > > > > > > @@ -451,6 +452,7 @@ protocol operations. See security library and PMD documentation for more details
> > > > > > > > > > >
> > > > > > > > > > >  * **[uses]       rte_eth_rxconf,rte_eth_rxmode**: ``offloads:DEV_RX_OFFLOAD_SECURITY``,
> > > > > > > > > > >  * **[uses]       rte_eth_txconf,rte_eth_txmode**: ``offloads:DEV_TX_OFFLOAD_SECURITY``.
> > > > > > > > > > > +* **[uses]       mbuf**: ``mbuf.l2_len``.
> > > > > > > > > > >  * **[implements] rte_security_ops**: ``session_create``, ``session_update``,
> > > > > > > > > > >    ``session_stats_get``, ``session_destroy``, ``set_pkt_metadata``, ``get_userdata``,
> > > > > > > > > > >    ``capabilities_get``.
> > > > > > > > > > > diff --git a/doc/guides/prog_guide/rte_security.rst b/doc/guides/prog_guide/rte_security.rst
> > > > > > > > > > > index f72bc8a78..7b68c698d 100644
> > > > > > > > > > > --- a/doc/guides/prog_guide/rte_security.rst
> > > > > > > > > > > +++ b/doc/guides/prog_guide/rte_security.rst
> > > > > > > > > > > @@ -560,7 +560,11 @@ created by the application is attached to the security session by the API
> > > > > > > > > > >
> > > > > > > > > > >  For Inline Crypto and Inline protocol offload, device specific defined metadata is
> > > > > > > > > > >  updated in the mbuf using ``rte_security_set_pkt_metadata()`` if
> > > > > > > > > > > -``DEV_TX_OFFLOAD_SEC_NEED_MDATA`` is set.
> > > > > > > > > > > +``RTE_SECURITY_TX_OLOAD_NEED_MDATA`` is set. ``rte_security_set_pkt_metadata()``
> > > > > > > > > > > +should be called on mbuf only with Layer 3 and above data present and
> > > > > > > > > > > +``mbuf.data_off`` should be pointing to Layer 3 Header.
> > > > > > > > > >
> > > > > > > > > > Hmm... not sure why mbuf.data_off should point to L3 hdr.
> > > > > > > > > > Who will add L2 hdr to the packet in that case?
> > > > > > > > > > Or did you mean ``mbuf.data_off + mbuf.l2_len`` here?
> > > > > > > > >
> > > > > > > > > That is the semantics I was trying to define. I think below are the sequence of
> > > > > > > > > operations to be done for ipsec processing,
> > > > > > > > >
> > > > > > > > > 1. receive_pkt()
> > > > > > > > > 2. strip_l2_hdr()
> > > > > > > > > 3. Do policy lookup ()
> > > > > > > > > 4. Call rte_security_set_pkt_metadata() if pkt needs to be encrypted with a
> > > > > > > > > particular SA. Now pkt only has L3 and above data.
> > > > > > > > > 5. Do route_lookup()
> > > > > > > > > 6. add_l2hdr() which might be different from stripped l2hdr.
> > > > > > > > > 7. Send packet out.
> > > > > > > > >
> > > > > > > > > The above sequence is what I believe the current poll mode worker thread in
> > > > > > > > > ipsec-secgw is following.
> > > > > > > >
> > > > > > > > That's just a sample app, it doesn't mean it has to be the only possible way.
> > > > > > > >
> > > > > > > > > While in event mode, step 2 and step 6 are missing.
> > > > > > > >
> > > > > > > > I think this L2 hdr manipulation is totally optional.
> > > > > > > > If your rte_security_set_pkt_metadata() implementation really needs to know L3 hdr offset (not sure why?),
> > > > > > > Since rte_security_set_pkt_metadata() is PMD specific function ptr call, we are currently doing some pre-processing
> > > > > > > here before submitting packet to inline IPSec via rte_eth_tx_burst(). This saves us cycles later in rte_eth_tx_burst().
> > > > > > > If we cannot know for sure, the pkt content at the time of rte_security_set_pkt_metadata() call, then I think
> > > > > > > having a PMD specific callback is not much of use except for saving SA priv data to rte_mbuf.
> > > > > > >
> > > > > > > > then I suppose we can add a requirement that l2_len has to be set properly before calling rte_security_set_pkt_metadata().
> > > > > > >
> > > > > > > This is also fine with us.
> > > > > >
> > > > > > Ok, so to make sure we are on the same page, you propose:
> > > > > > 1. before calling rte_security_set_pkt_metadata() mbuf.l2_len should be properly set.
> > > > > > 2. after rte_security_set_pkt_metadata() and before rte_eth_tx_burst() packet contents
> > > > > >     at [mbuf.l2_len, mbuf.pkt_len) can't be modified?
> > > > > Yes.
> > > > >
> > > > > >
> > > > > > Is that correct understanding?
> > > > > > If yes, I wonder how 2) will correlate with rte_eth_tx_prepare() concept?
> > > > >
> > > > > Since our PMD doesn't have a prepare function, I missed that but, since
> > > > > rte_security_set_pkt_metadata() is only used for Inline Crypto/Protocol via
> > > > > a rte_eth_dev, and both rte_security_set_pkt_metadata() and rte_eth_tx_prepare()
> > > > > are callbacks from same PMD, do you see any issue ?
> > > > >
> > > > > The restriction is from user side, data is not supposed to be modified unless
> > > > > rte_security_set_pkt_metadata() is called again.
> > > >
> > > > Yep, I do have a concern here.
> > > > Right now it is perfectly valid to do something like that:
> > > > rte_security_set_pkt_metadata(..., mb, ...);
> > > > /* can modify contents of the packet */
> > > > rte_eth_tx_prepare(..., &mb, 1);
> > > > rte_eth_tx_burst(..., &mb, 1);
> > > >
> > > > With the new restrictions you are proposing it wouldn't be allowed any more.
> > > You can still modify L2 header and IPSEC is only concerned about L3 and above.
> > >
> > > I think insisting that rte_security_set_pkt_metadata() be called after all L3
> > > and above header modifications is no a problem. I guess existing ixgbe/txgbe
> > > PMD which are the ones only implementing the call back are already expecting the
> > > same ?
> >
> > AFAIK, no there are no such requirements for ixgbe or txgbe.
> > All that ixgbe callback does - store session related data inside mbuf.
> > It's only expectation to have ESP trailer at the proper place (after ICV):
> 
> This implies rte_security_set_pkt_metadata() cannot be called when mbuf does't
> have ESP trailer updated or when mbuf->pkt_len = 0
> 
> >
> > union ixgbe_crypto_tx_desc_md *mdata = (union ixgbe_crypto_tx_desc_md *)
> >                                 rte_security_dynfield(m);
> >   mdata->enc = 1;
> >   mdata->sa_idx = ic_session->sa_index;
> >   mdata->pad_len = ixgbe_crypto_compute_pad_len(m);
> >
> > Then this data will be used by tx_burst() function.
> So it implies that after above rte_security_set_pkt_metadata() call, and before tx_burst(),
> mbuf data / packet len cannot be modified right as if modified, then tx_burst()
> will be using incorrect pad len ?

No, pkt_len can be modified.
Though ESP trailer pad_len can't.

> 
> This patch is also trying to add similar restriction on when
> rte_security_set_pkt_metadata() should be called and what cannot be done after
> calling rte_security_set_pkt_metadata().

No, I don't think it is really the same.
Also, IMO, inside ixgbe set_pkt_metadata() implementaion we probably shouldn't silently imply
that ESP packet is already formed and trailer contains valid data.
In fact, I think this pad_len calculation can be moved to actual TX function.

> 
> >
> > >
> > > >
> > > > >
> > > > > If your question is can't we do the preprocessing in rte_eth_tx_prepare() for
> > > > > security,
> > > >
> > > > Yes, that was my thought.
> > > >
> > > > > my only argument was that since there is already a hit in
> > > > > rte_security_set_pkt_metadata() to PMD specific callback and
> > > > > struct rte_security_session is passed as an argument to it, it is more benefitial to
> > > > > do security related pre-processing there.
> > > >
> > > > Yes, it would be extra callback call that way.
> > > > Though tx_prepare() accepts burst of packets, so the overhead
> > > > of function call will be spread around the whole burst, and I presume
> > > > shouldn't be too high.
> > > >
> > > > > Also rte_eth_tx_prepare() if implemented will be called for both security and
> > > > > non-security pkts.
> > > >
> > > > Yes, but tx_prepare() can distinguish (by ol_flags and/or other field contents) which
> > > > modifications are required for the packet.
> > >
> > > But the major issues I see are
> > >
> > > 1. tx_prepare() doesn't take rte_security_session as argument though ol_flags has security flag.
> > >    In our case, we need to know the security session details to do things.
> >
> > I suppose you can store pointer to session (or so) inside mbuf in rte_security_dynfield, no?
> 
> We can do. But having to call PMD specific function call via rte_security_set_pkt_metadata()
> just for storing session pointer in rte_security_dynfield consumes unnecessary
> cycles per pkt.

In fact there are two function calls: one for rte_security_set_pkt_metadata(),
second for  instance->ops->set_pkt_metadata() callback.
Which off-course way too expensive for such simple operation.
Actually same thought for rte_security_get_userdata().
Both of these functions belong to data-path and ideally have to be as fast as possible.
Probably 21.11 is a right timeframe for that.
 
> >
> > > 2. AFAIU tx_prepare() is not mandatory as per spec and even by default disabled under compile time
> > >    macro RTE_ETHDEV_TX_PREPARE_NOOP.
> > > 3. Even if we do tx_prepare(), rte_security_set_pkt_mdata() is mandatory to associate
> > >    struct rte_security_session to a pkt as unlike ol_flags, there is no direct space to do the same.
> >
> > Didn't get you here, obviously we do have rte_security_dynfield inside mbuf,
> > specially for that - to store secuiryt related data inside the mbuf.
> > Yes your PMD has to request it at initialization time, but I suppose it is not a big deal.
> >
> > > So I think instead of enforcing yet another callback tx_prepare() for inline security
> > > processing, it can be done via security specific set_pkt_metadata().
> >
> > But what you proposing introduces new limitations and might existing functionality.
> > BTW, if you don't like to use tx_prepare() - why doing these calculations inside tx_burst()
> > itself is not an option?
> 
> We can do things in tx_burst() but if we are doing it there, then we want to avoid having callback for
> rte_security_set_pkt_metadata().
> 
> Are you fine if we can update the spec that "When DEV_TX_OFFLOAD_SEC_NEED_MDATA is not
> set, then, user needs to update struct rte_security_session's sess_private_data in a in
> rte_security_dynfield like below ?
> 
> <snip>
> 
> static inline void
> inline_outb_mbuf_prepare(const struct rte_ipsec_session *ss,
>         struct rte_mbuf *mb[], uint16_t num)
> {
>         uint32_t i, ol_flags;
> 
>         ol_flags = ss->security.ol_flags & RTE_SECURITY_TX_OLOAD_NEED_MDATA;
>         for (i = 0; i != num; i++) {
> 
>                 mb[i]->ol_flags |= PKT_TX_SEC_OFFLOAD;
> 
>                 if (ol_flags != 0)
>                         rte_security_set_pkt_metadata(ss->security.ctx,
>                                 ss->security.ses, mb[i], NULL);
> 		else
>                 	*rte_security_dynfield(mb[i]) =
>                                 (uint64_t)ss->security.ses->sess_private_data;
> 
> 
> If the above can be done, then in our PMD, we will not have a callback for
> set_pkt_metadata() and DEV_TX_OFFLOAD_SEC_NEED_MDATA will also be not set
> in capabilities.

That's an interesting idea, but what you propose is the change in current rte_security API behaviour.
So all existing apps that use this API will have to be changed.
We'd better avoid such changes unless there is really good reason for that.
So, I'd suggest to tweak your idea a bit:

1) change rte_security_set_pkt_metadata():
if ops->set_pkt_metadata != NULL, then call it (existing behaviour)
otherwise just: rte_security_dynfield(m) = sess->session_private_data;
(fast-path)

2) consider to make rte_security_set_pkt_metadata() inline function. 
We probably can have some special flag inside struct rte_security_ctx,
or even store inside ctx a pointer to set_pkt_metadata() itself.

As a brief code snippet:

struct rte_security_ctx {
        void *device;
        /**< Crypto/ethernet device attached */
        const struct rte_security_ops *ops;
        /**< Pointer to security ops for the device */
        uint16_t sess_cnt;
        /**< Number of sessions attached to this context */
+     int (*set_pkt_mdata)(void *, struct rte_security_session *, struct rte_mbuf *,  void *);   
}; 

static inline int
rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
                              struct rte_security_session *sess,
                              struct rte_mbuf *m, void *params)
{
     /* fast-path */
      if (instance->set_pkt_mdata == NULL) {
             *rte_security_dynfield(m) = (rte_security_dynfield_t)(session->sess_priv_data);
             return 0; 
       /* slow path */ 
       } else
           return instance->set_pkt_mdata(instance->device, sess, m, params);
}

That probably would be an ABI breakage (new fileld in rte_security_ctx) and would require 
some trivial changes for all existing PMDs that use RTE_SECURITY_TX_OFLOAD_NEED_MDATA
(ctx_create()), but hopefully will benefit everyone.

> 
> >
> > > I'm fine to
> > > introduce a burst call for the same(I was thinking to propose it in future) to
> > > compensate for the overhead.
> > >
> > > If rte_security_set_pkt_metadata() was not a PMD specific function ptr call and
> > > rte_mbuf had space for struct rte_security_session pointer,
> >
> > But it does, see above.
> > In fact it even more flexible - because it is driver specific, you are not limited to one 64-bit field.
> > If your PMD requires more data to be associated with mbuf
> > - you can request it via mbuf_dynfield and store there whatever is needed.
> >
> > > then then I guess it would have been better to do the way you proposed.
> > >
> > > >
> > > > >
> > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > This patch is trying to enforce semantics as above so that
> > > > > > > > > rte_security_set_pkt_metadata() can predict what comes in the pkt when he is
> > > > > > > > > called.
> > > > > > > > >
> > > > > > > > > I also think above sequence is what Linux kernel stack or other stacks follow.
> > > > > > > > > Does it makes sense ?
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > Once called,
> > > > > > > > > > > +Layer 3 and above data cannot be modified or moved around unless
> > > > > > > > > > > +``rte_security_set_pkt_metadata()`` is called again.
> > > > > > > > > > >
> > > > > > > > > > >  For inline protocol offloaded ingress traffic, the application can register a
> > > > > > > > > > >  pointer, ``userdata`` , in the security session. When the packet is received,
> > > > > > > > > > > diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
> > > > > > > > > > > index bb38d7f58..9d8e3ddc8 100644
> > > > > > > > > > > --- a/lib/mbuf/rte_mbuf_core.h
> > > > > > > > > > > +++ b/lib/mbuf/rte_mbuf_core.h
> > > > > > > > > > > @@ -228,6 +228,8 @@ extern "C" {
> > > > > > > > > > >
> > > > > > > > > > >  /**
> > > > > > > > > > >   * Request security offload processing on the TX packet.
> > > > > > > > > > > + * To use Tx security offload, the user needs to fill l2_len in mbuf
> > > > > > > > > > > + * indicating L2 header size and where L3 header starts.
> > > > > > > > > > >   */
> > > > > > > > > > >  #define PKT_TX_SEC_OFFLOAD	(1ULL << 43)
> > > > > > > > > > >
> > > > > > > > > > > --
> > > > > > > > > > > 2.25.1
> > > > > > > > > >