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

[Sabrina Dubroca <sd@queasysnail.net>]

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