netdev - Re: [EXTERNAL] 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:
 <IA2PR18MB58851AF05D38A69B15571967D6FAA@IA2PR18MB5885.namprd18.prod.outlook.com>
Date: Wed, 29 Oct 2025 17:36:52 +0000
From: Tanmay Jagdale <tanmay@...vell.com>
To: Sabrina Dubroca <sd@...asysnail.net>
CC: "davem@...emloft.net" <davem@...emloft.net>,
        "horms@...nel.org"
	<horms@...nel.org>,
        "leon@...nel.org" <leon@...nel.org>,
        "herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
        Bharat Bhushan
	<bbhushan2@...vell.com>,
        Sunil Kovvuri Goutham <sgoutham@...vell.com>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [EXTERNAL] Re: [PATCH net-next v5 15/15] octeontx2-pf: ipsec: Add
 XFRM state and policy hooks for inbound flows

Hi Sabrina,

>> +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?
Oops. Will fix this in the next version.

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

>> +	}
>> +
>> +	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?
Yes I agree. Will free this memory in the error paths.

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

> [...]
>> +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).
Okay sure. I'll revisit the policy and state delete/free routines and
see what can be split between these functions.

> -- 
> Sabrina
Thanks,
Tanmay