Subject: Re: [devel-ipsec] [PATCH ipsec-next v5 06/17] xfrm: add mode_cbs module functionality

From: Christian Hopps <[email-masked]>
Date: Fri, 19 Jul 2024 17:06:41 -0700

Florian Westphal <[email-masked]> writes:

> Christian Hopps via Devel <[email-masked]> wrote:
>> +static struct xfrm_mode_cbs xfrm_mode_cbs_map[XFRM_MODE_MAX];
>
> Why not
> static struct xfrm_mode_cbs *xfrm_mode_cbs_map[XFRM_MODE_MAX];
> i.e., why does
>
>> +int xfrm_register_mode_cbs(u8 mode, const struct xfrm_mode_cbs *mode_cbs)
>> +{
>> +	if (mode >= XFRM_MODE_MAX)
>> +		return -EINVAL;
>> +
>> +	xfrm_mode_cbs_map[mode] = *mode_cbs;
>
> do a deep copy here rather than
> 	xfrm_mode_cbs_map[mode] = mode_cbs;
>
> ?
>
>> +	if (mode == XFRM_MODE_IPTFS && !xfrm_mode_cbs_map[mode].create_state)
>> +		request_module("xfrm-iptfs");
>> +	return &xfrm_mode_cbs_map[mode];
>
> Sabrina noticed that this looks weird, and I agree.
>
> This can, afaics, return a partially initialised structure or an
> all-zero structure.
>
> request_module() might load the module, it can fail to load the
> module, and, most importantly, there is no guarantee that the
> module is removed right after.
>
> Is there any locking scheme in place that prevents this from
> happening? If so, a comment should be added that explains this.

I've switched it to the more obvious array of pointers and storing the module provided pointer.

Thanks,
Chris.