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