Re: [dpdk-dev] [EXT] RE: [PATCH] examples/ipsec-secgw: fix dropping of initial IPsec pkts

[Anoob Joseph <anoobj@marvell.com>]

Hi Konstantin,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Tuesday, April 21, 2020 8:48 PM
> To: Lukas Bartosik [C] <lbartosik@marvell.com>
> Cc: akhil.goyal@nxp.com; Nicolau, Radu <radu.nicolau@intel.com>; Anoob
> Joseph <anoobj@marvell.com>; Narayana Prasad Raju Athreya
> <pathreya@marvell.com>; dev@dpdk.org; orika@mellanox.com; Doherty,
> Declan <declan.doherty@intel.com>
> Subject: RE: [dpdk-dev] [EXT] RE: [PATCH] examples/ipsec-secgw: fix dropping
> of initial IPsec pkts
> 
> 
> Hi Lukas,
> 
> Apologies for delay with response.
> 
> > Do you have any thoughts how both cases could be covered:
> > 1. Inline not applied to inbound IPsec pkts for short duration of time
> > after rte_eth_dev_start() but before sa_init() is executed (which creates SAs).
> > 2. SAs not surviving rte_eth_dev_start() on ixgbe driver.
> 
> No, right now I don't have an idea how both can be satisfied.
> About 1) - why do you consider that as problem?
> As I understand when inline device is started by not properly programmed yet,
> these packets will be treated by device as non-ipsec ones (just plain-text
> packets).
> Then later in ipsec-secgw RX path they will probably be just dropped.
> So no harm here, no?

[Anoob] Yes, when SAs are not yet created and device is started then HW treats IPsec packets as clear packets and does not apply inline to them. The IPsec packets arrive at application level. This points at a serious hardware issue as a packet intended for IPsec has reached application without being IPsec processed. Application would check if session is created and since the session is available at the time of processing the packet (in the application), it would look like a hardware issue when it is just a false positive.

> 
> About 2) - looking at ixgbe code, I think it can be changed to make ipsec flows to
> survive device start/stop (though most likely not in 20.05 timeframe).
> So I am not sure should we do it at all.
> Obviously for ixgbe (with 1K max SA) it is not a problem to save extra info about
> HW flows and restore them after stop/start.
> Though for modern devices that might have millions of flows maintaining such
> info in PMD might become a significant overhead.
> As generic question - how PMD should behave in such situation?
> Should it support flows survival through dev_start/dev_stop, or should it be app
> responsibility to reprogram flows after dev-start/dev_stop.

[Anoob] I believe the spec talks about the behavior during start/stop and initialization.

"Flow rules are not maintained between successive port initializations. An application exiting without releasing them and restarting must re-create them from scratch."

"PMDs, not applications, are responsible for maintaining flow rules configuration when stopping and restarting a port or performing other actions which may affect them."

From the above two, following is what I could conclude,
1. Across initializations rte_flow rules are not expected to be retained.
2. Across restarts (dev start/stop), PMDs have to ensure rte_flow rules survive.

I haven't looked at the ixgbe PMD yet, but technically start/stop should only be about whether the device is running. I believe there are other settings also which need to be retained between start/stop.

From https://doc.dpdk.org/api/rte__ethdev_8h.html

Please note that some configuration is not stored between calls to rte_eth_dev_stop()/rte_eth_dev_start(). The following configuration will be retained:

- MTU
- flow control settings
- receive mode configuration (promiscuous mode, all-multicast mode,
  hardware checksum mode, RSS/VMDQ settings etc.)
- VLAN filtering configuration
- default MAC address
- MAC addresses supplied to MAC address array
- flow director filtering mode (but not filtering rules)
- NIC queue statistics mappings

Flow director refers to rte_flow, right? 

> I looked through PG for rte_flow, and it seems a bit conversional here (at least
> for me):
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__doc.dpdk.org_guides_prog-5Fguide_rte-
> 5Fflow.html&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs
> 2n6B-
> WYLn1v9SyTMrT5EQqh2TU&m=pUa6XHzwVA2IP7UEe5b98hosHmHjxlLOmn3IxH
> hZQAA&s=2BM4iySNyS0svzG26B0PiG8hQoAT2clAtkxiJrytdaI&e=  (11.7):
> Form one side:
> - DPDK does not keep track of flow rules definitions or flow rule objects
> automatically.
>   Applications may keep track of the former and must keep track of the latter.
>   PMDs may also do it for internal needs, however this must not be relied on by
> applications.
> But a bit below:
> - PMDs, not applications, are responsible for maintaining flow rules
> configuration
>    when stopping and restarting a port or performing other actions which may
> affect them.
>   They can only be destroyed explicitly by applications.
> 
> Would like to know what rte_flow maintainers and other interested parties think
> about it?

[Anoob] I do agree that the above two points can be misleading. But it actually describes the usage of rte_flow API.

1. "DPDK does not keep track of flow rules definitions or flow rule objects automatically. Applications may keep track of the former and must keep track of the latter."
DPDK rte_flow spec doesn't provide an API to check if a rule is already created. Application is configuring hardware resources using rte_flow and it has to keep track of what is configured. 

2. "PMDs, not applications, are responsible for maintaining flow rules configuration when stopping and restarting a port or performing other actions which may affect them."
In this case, we are talking about the state of the hardware. Once the rules are configured, it should survive across stop-start. Application is not required to destroy and create rte_flows in such case.

In first case, we are talking about rte_flow objects & rules (like session in case of rte_security) and in the second we are talking about hardware state.

I think this patch can be delayed till ixgbe can be fixed. Having it in 20.08 timeframe should be fine.

> 
> Konstantin
> 
> >
> > Thanks,
> > Lukasz
> >
> > On 16.04.2020 14:28, Lukas Bartosik [C] wrote:
> > > Hi Konstantin,
> > >
> > > Please see my answer below.
> > >
> > > Thanks,
> > > Lukasz
> > >
> > > On 16.04.2020 01:47, Ananyev, Konstantin wrote:
> > >> External Email
> > >>
> > >> -------------------------------------------------------------------
> > >> ---
> > >>
> > >>
> > >> Hi Lukasz,
> > >>
> > >>> Hi Konstantin,
> > >>>
> > >>> In this patch I moved the sa_init() before rte_eth_dev_start() in
> > >>> order to avoid dropping of IPsec pkts when a traffic flows and the ipsec-
> secgw application is started.
> > >>>
> > >>> However I remember that during review of event mode patches you
> > >>> mentioned that moving sa_init() before rte_eth_dev_start() is an
> > >>> issue for one of the Intel drivers.
> > >>
> > >> Yes, I think so.
> > >> https://urldefense.proofpoint.com/v2/url?u=http-3A__mails.dpdk.org_
> > >> archives_dev_2019-
> >
> 2DDecember_153908.html&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=SchR
> HhE7GLC
> > jEY4i2a1byjC_FpWgRLtq4-
> > kLvKp3_84&m=w3xh94Ox4xIhabfE-
> nD2VbEWbh2JTmiscVMb6pJZcYo&s=9rDtRPGK2QBD
> > AcY8VQf0HQzXINtQzucwIxU7DB2ND5s&e=
> > >> Moving that piece of code (dev_start) after sa_init() breaks ixgbe inline-
> crypto support.
> > >> As I understand, because configured ipsec flows don't persist dev_start().
> > >> At least for ixgbe PMD.
> > >> Any reason why to move that code at all?
> > >>
> > >
> > > [Lukasz] We're observing issue in inline mode. When traffic flows
> > > and ipsec-secgw application is started then for short period of time
> > > inline is not applied by HW and IPsec packets reach the application.
> > > This is because
> > > sa_init() (which creates security associations SAs for HW) is executed after
> rte_eth_dev_start().
> > > That's the reason I moved the code. And that movement fixes the
> > > issue because now SAs are already created when eth ports are started.
> > >
> > > Would it be possible to fix the ixgbe so that SAs would survive
> rte_eth_dev_start() ?
> > > Do you have any other idea how we could cover both cases ?
> > >
> > >>  > Is this still the case ?
> > >>
> > >> AFAIK, yes.
> > >> Thanks for bringing it to attention.
> > >> Konstantin
> > >>
> > >>
> > >>>
> > >>> Thanks,
> > >>> Lukasz
> > >>>
> > >>> On 08.04.2020 13:32, Lukasz Bartosik wrote:
> > >>>> In inline event mode when traffic flows and the ipsec-secgw app
> > >>>> is started then for short period of time IPsec packets arrive at
> > >>>> application without being decrypted and are dropped by the
> > >>>> application. This happens because eth ports are started before
> > >>>> creation of inline sessions and IPsec flows. This fix rearranges
> > >>>> the code in such a way that eth ports are always started after
> > >>>> creation of inline sessions and IPsec flows.
> > >>>>
> > >>>> Change-Id: Ifddc446082fb2897f81559517f90e1ee603e13f3
> > >>>> Signed-off-by: Lukasz Bartosik <lbartosik@marvell.com>
> > >>>> ---
> > >>>>  examples/ipsec-secgw/event_helper.c | 26
> > >>>> --------------------------  examples/ipsec-secgw/ipsec-secgw.c  |
> > >>>> 26 +++++++++++++-------------
> > >>>>  2 files changed, 13 insertions(+), 39 deletions(-)
> > >>>>
> > >>>> diff --git a/examples/ipsec-secgw/event_helper.c
> > >>>> b/examples/ipsec-secgw/event_helper.c
> > >>>> index 076f1f2..da861e4 100644
> > >>>> --- a/examples/ipsec-secgw/event_helper.c
> > >>>> +++ b/examples/ipsec-secgw/event_helper.c
> > >>>> @@ -1526,7 +1526,6 @@ int32_t
> > >>>>  eh_devs_init(struct eh_conf *conf)  {
> > >>>>  	struct eventmode_conf *em_conf;
> > >>>> -	uint16_t port_id;
> > >>>>  	int ret;
> > >>>>
> > >>>>  	if (conf == NULL) {
> > >>>> @@ -1558,16 +1557,6 @@ eh_devs_init(struct eh_conf *conf)
> > >>>>  	/* Display the current configuration */
> > >>>>  	eh_display_conf(conf);
> > >>>>
> > >>>> -	/* Stop eth devices before setting up adapter */
> > >>>> -	RTE_ETH_FOREACH_DEV(port_id) {
> > >>>> -
> > >>>> -		/* Use only the ports enabled */
> > >>>> -		if ((conf->eth_portmask & (1 << port_id)) == 0)
> > >>>> -			continue;
> > >>>> -
> > >>>> -		rte_eth_dev_stop(port_id);
> > >>>> -	}
> > >>>> -
> > >>>>  	/* Setup eventdev */
> > >>>>  	ret = eh_initialize_eventdev(em_conf);
> > >>>>  	if (ret < 0) {
> > >>>> @@ -1589,21 +1578,6 @@ eh_devs_init(struct eh_conf *conf)
> > >>>>  		return ret;
> > >>>>  	}
> > >>>>
> > >>>> -	/* Start eth devices after setting up adapter */
> > >>>> -	RTE_ETH_FOREACH_DEV(port_id) {
> > >>>> -
> > >>>> -		/* Use only the ports enabled */
> > >>>> -		if ((conf->eth_portmask & (1 << port_id)) == 0)
> > >>>> -			continue;
> > >>>> -
> > >>>> -		ret = rte_eth_dev_start(port_id);
> > >>>> -		if (ret < 0) {
> > >>>> -			EH_LOG_ERR("Failed to start eth dev %d, %d",
> > >>>> -				   port_id, ret);
> > >>>> -			return ret;
> > >>>> -		}
> > >>>> -	}
> > >>>> -
> > >>>>  	return 0;
> > >>>>  }
> > >>>>
> > >>>> diff --git a/examples/ipsec-secgw/ipsec-secgw.c
> > >>>> b/examples/ipsec-secgw/ipsec-secgw.c
> > >>>> index 5fde4f7..e03bd89 100644
> > >>>> --- a/examples/ipsec-secgw/ipsec-secgw.c
> > >>>> +++ b/examples/ipsec-secgw/ipsec-secgw.c
> > >>>> @@ -2829,6 +2829,19 @@ main(int32_t argc, char **argv)
> > >>>>  	if (ret < 0)
> > >>>>  		rte_exit(EXIT_FAILURE, "eh_devs_init failed, err=%d\n", ret);
> > >>>>
> > >>>> +	/* Replicate each context per socket */
> > >>>> +	for (i = 0; i < NB_SOCKETS && i < rte_socket_count(); i++) {
> > >>>> +		socket_id = rte_socket_id_by_idx(i);
> > >>>> +		if ((socket_ctx[socket_id].mbuf_pool != NULL) &&
> > >>>> +			(socket_ctx[socket_id].sa_in == NULL) &&
> > >>>> +			(socket_ctx[socket_id].sa_out == NULL)) {
> > >>>> +			sa_init(&socket_ctx[socket_id], socket_id);
> > >>>> +			sp4_init(&socket_ctx[socket_id], socket_id);
> > >>>> +			sp6_init(&socket_ctx[socket_id], socket_id);
> > >>>> +			rt_init(&socket_ctx[socket_id], socket_id);
> > >>>> +		}
> > >>>> +	}
> > >>>> +
> > >>>>  	/* start ports */
> > >>>>  	RTE_ETH_FOREACH_DEV(portid) {
> > >>>>  		if ((enabled_port_mask & (1 << portid)) == 0) @@ -2866,19
> > >>>> +2879,6 @@ main(int32_t argc, char **argv)
> > >>>>  			rte_exit(EXIT_FAILURE, "failed at reassemble init");
> > >>>>  	}
> > >>>>
> > >>>> -	/* Replicate each context per socket */
> > >>>> -	for (i = 0; i < NB_SOCKETS && i < rte_socket_count(); i++) {
> > >>>> -		socket_id = rte_socket_id_by_idx(i);
> > >>>> -		if ((socket_ctx[socket_id].mbuf_pool != NULL) &&
> > >>>> -			(socket_ctx[socket_id].sa_in == NULL) &&
> > >>>> -			(socket_ctx[socket_id].sa_out == NULL)) {
> > >>>> -			sa_init(&socket_ctx[socket_id], socket_id);
> > >>>> -			sp4_init(&socket_ctx[socket_id], socket_id);
> > >>>> -			sp6_init(&socket_ctx[socket_id], socket_id);
> > >>>> -			rt_init(&socket_ctx[socket_id], socket_id);
> > >>>> -		}
> > >>>> -	}
> > >>>> -
> > >>>>  	check_all_ports_link_status(enabled_port_mask);
> > >>>>
> > >>>>  	/* launch per-lcore init on every lcore */
> > >>> >