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

[Florian Westphal <fw@strlen.de>]

Christian Hopps via Devel <devel@linux-ipsec.org> 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.