netdev - Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221124110748.GP424616@gauss3.secunet.de>
Date:   Thu, 24 Nov 2022 12:07:48 +0100
From:   Steffen Klassert <steffen.klassert@...unet.com>
To:     Leon Romanovsky <leon@...nel.org>
CC:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "Jakub Kicinski" <kuba@...nel.org>, <netdev@...r.kernel.org>
Subject: Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

On Wed, Nov 23, 2022 at 02:53:10PM +0200, Leon Romanovsky wrote:
> On Wed, Nov 23, 2022 at 11:36:19AM +0200, Leon Romanovsky wrote:
> > Thanks for an explanation, trying it now.
> 
> Something like that?

Yes :)

> 
> The code is untested yet.
> 
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 5076f9d7a752..5819023c32ba 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1115,6 +1115,19 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
>  	rcu_read_lock();
>  	h = xfrm_dst_hash(net, daddr, saddr, tmpl->reqid, encap_family);
>  	hlist_for_each_entry_rcu(x, net->xfrm.state_bydst + h, bydst) {
> +		if (IS_ENABLED(CONFIG_XFRM_OFFLOAD) &&
> +		    pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {

Please try to avoid that check for every state in the list.
Maybe enable this code with a static key if packet offload
is used?

> +			if (x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
> +				/* HW states are in the head of list, there is no need
> +				 * to iterate further.
> +				 */
> +				break;
> +
> +			/* Packet offload: both policy and SA should have same device */
> +			if (pol->xdo.dev != x->xso.dev)
> +				continue;
> +		}
> +
>  		if (x->props.family == encap_family &&
>  		    x->props.reqid == tmpl->reqid &&
>  		    (mark & x->mark.m) == x->mark.v &&
> @@ -1132,6 +1145,19 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
>  
>  	h_wildcard = xfrm_dst_hash(net, daddr, &saddr_wildcard, tmpl->reqid, encap_family);
>  	hlist_for_each_entry_rcu(x, net->xfrm.state_bydst + h_wildcard, bydst) {
> +		if (IS_ENABLED(CONFIG_XFRM_OFFLOAD) &&
> +		    pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {
> +			if (x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
> +				/* HW states are in the head of list, there is no need
> +				 * to iterate further.
> +				 */
> +				break;
> +
> +			/* Packet offload: both policy and SA should have same device */
> +			if (pol->xdo.dev != x->xso.dev)
> +				continue;
> +		}
> +
>  		if (x->props.family == encap_family &&
>  		    x->props.reqid == tmpl->reqid &&
>  		    (mark & x->mark.m) == x->mark.v &&
> @@ -1185,6 +1211,17 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
>  			goto out;
>  		}
>  
> +		if (pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {
> +			memcpy(&x->xso, &pol->xdo, sizeof(x->xso));
> +			error = pol->xdo.dev->xfrmdev_ops->xdo_dev_state_add(x);
> +			if (error) {
> +				x->km.state = XFRM_STATE_DEAD;
> +				to_put = x;
> +				x = NULL;
> +				goto out;
> +			}
> +		}

I guess that is to handle acquires, right?
What is the idea behind that? xdo_dev_state_add sets
offload type and dev?