netdev - Re: [PATCH net-next v5 15/15] octeontx2-pf: ipsec: Add XFRM state and policy hooks for inbound flows

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: <aQCrWAVZh2VlOl54@krikkit>
Date: Tue, 28 Oct 2025 12:39:04 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Tanmay Jagdale <tanmay@...vell.com>
Cc: davem@...emloft.net, horms@...nel.org, leon@...nel.org,
	herbert@...dor.apana.org.au, bbhushan2@...vell.com,
	sgoutham@...vell.com, linux-crypto@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next v5 15/15] octeontx2-pf: ipsec: Add XFRM state
 and policy hooks for inbound flows

2025-10-26, 20:39:10 +0530, Tanmay Jagdale wrote:
> +static int cn10k_ipsec_policy_add(struct xfrm_policy *x,
> +				  struct netlink_ext_ack *extack)
> +{
> +	struct cn10k_inb_sw_ctx_info *inb_ctx_info = NULL, *inb_ctx;
> +	struct net_device *netdev = x->xdo.dev;
> +	bool disable_rule = true;
> +	struct otx2_nic *pf;
> +	int ret = 0;
> +
> +	if (x->xdo.dir != XFRM_DEV_OFFLOAD_IN) {
> +		netdev_err(netdev, "ERR: Can only offload Inbound policies\n");
> +		ret = -EINVAL;

missing goto/return?

> +	}
> +
> +	if (x->xdo.type != XFRM_DEV_OFFLOAD_PACKET) {
> +		netdev_err(netdev, "ERR: Only Packet mode supported\n");
> +		ret = -EINVAL;

missing goto/return?

> +	}
> +
> +	pf = netdev_priv(netdev);
> +
> +	/* If XFRM state was added before policy, then the inb_ctx_info instance
> +	 * would be allocated there.
> +	 */
> +	list_for_each_entry(inb_ctx, &pf->ipsec.inb_sw_ctx_list, list) {
> +		if (inb_ctx->reqid == x->xfrm_vec[0].reqid) {
> +			inb_ctx_info = inb_ctx;
> +			disable_rule = false;
> +			break;
> +		}
> +	}
> +
> +	if (!inb_ctx_info) {
> +		/* Allocate a structure to track SA related info in driver */
> +		inb_ctx_info = devm_kzalloc(pf->dev, sizeof(*inb_ctx_info), GFP_KERNEL);

I'm not so familiar with devm_*, but according to the kdoc for
devm_kmalloc, this will get freed automatically when the driver goes
away (but not earlier). This could take a long time. Shouldn't this be
manually freed in the error path of this function, and somewhere
during the policy_delete/policy_free calls?

I see that you've got a devm_kfree in cn10k_ipsec_inb_add_state, so
something similar here?

[...]
> +static void cn10k_ipsec_policy_free(struct xfrm_policy *x)
> +{
> +	return;
>  }

The stack can handle a NULL .xdo_dev_policy_free, so this empty
implementation is not needed. But I'm not sure releasing all
policy-related resources at delete time (even via WQ) is safe, so
possibly some of the work done in cn10k_ipsec_policy_delete should be
moved here (similar comment for the existing cn10k_ipsec_del_state
code vs adding .xdo_dev_state_free).

-- 
Sabrina