[ovs-dev] [PATCH 1/3] datapath: Add genl_exec().

[Jesse Gross]

On Dec 21, 2011, at 8:28 PM, Pravin B Shelar wrote:
> diff --git a/datapath/linux/compat/genetlink.inc b/datapath/linux/compat/genetlink.inc
> index bf96980..1301006 100644
> --- a/datapath/linux/compat/genetlink.inc
> +++ b/datapath/linux/compat/genetlink.inc
> +static struct sk_buff *genlmsg_skb;
> +
> +int genl_exec_init(void)
> +{
> +	int err;
> +
> +	err = genl_register_family_with_ops(&genl_exec_family,
> +			genl_exec_ops,
> +			ARRAY_SIZE(genl_exec_ops));
> +
> +	if (err)
> +		return err;
> +
> +	genlmsg_skb = genlmsg_new( NLMSG_HDRLEN, GFP_KERNEL);

The argument to genlmsg_new() is actually the size of the payload that you plan on inserting, which in this case is nothing so I think we can just pass in 0 here.  There's also an extra space before NLMSG_HDRLEN.

> +void genl_exec_exit(void)
> +{
> +	genl_unregister_family(&genl_exec_family);
> +	mutex_lock(&genl_exec_lock);
> +	kfree_skb(genlmsg_skb);
> +	genlmsg_skb = NULL;
> +	mutex_unlock(&genl_exec_lock);

I think it's safe to assume that nobody is using this anymore.  This is the last step before unloading the module, so if there's any chance that somebody could still be calling into our code, we're in trouble.

> +/* genl_lock() is not exported from older kernel.
> + * Following function allows any function to be executed with
> + * genl_mutex held. */
> +
> +int genl_exec(genl_exec_func_t func, void *data)
> +{
> +	int ret;
> +	struct nlmsghdr *nlh;

Is this ever used?

> +	mutex_lock(&genl_exec_lock);
> +	if (!genlmsg_skb) {
> +		ret = -ESHUTDOWN;
> +		goto out;
> +	}

Similarly, I think this condition can never happen.

> +	skb_get(genlmsg_skb);
> +
> +	nlh = genlmsg_put(genlmsg_skb, 0, 0, &genl_exec_family, NLM_F_REQUEST, GENL_EXEC_RUN);

Is it possible to do this once when the skb is allocated or does Netlink actually change the contents of the skb?  If not, then I think we could avoid all the resetting at the end as well.

> +	genl_exec_function = func;
> +	genl_exec_data = data;
> +	init_completion(&done);
> +
> +	ret = genlmsg_unicast(&init_net, genlmsg_skb, 0);
> +	if (!ret) {
> +		wait_for_completion(&done);
> +		ret = genl_exec_function_ret;
> +	} else
> +		printk(KERN_ERR "[%s] Msg send error %d\n",__func__, ret);

This is a somewhat strange form for an error message.  Can't we just use the normal pr_err("genl_exec send error %d", ret") or even just BUG_ON(ret) since we're claiming that this can never happen?

> +	genlmsg_skb->data = genlmsg_skb->head;

Is this actually necessary?  genlmsg_put() eventually calls skb_put(), which advances the tail pointer but not data (this is back to the question of whether anybody else is modifying the skb).

> +	skb_reset_tail_pointer(genlmsg_skb);

This doesn't decrement skb->len.  I guess nobody checks it but it doesn't seem good to let it grow without bound.

> diff --git a/datapath/linux/compat/include/linux/skbuff.h b/datapath/linux/compat/include/linux/skbuff.h
> index 96d8012..01e524e 100644
> --- a/datapath/linux/compat/include/linux/skbuff.h
> +++ b/datapath/linux/compat/include/linux/skbuff.h
> @@ -34,6 +34,12 @@ static inline void skb_copy_to_linear_data_offset(struct sk_buff *skb,
> 
> #endif	/* !HAVE_SKB_COPY_FROM_LINEAR_DATA_OFFSET */
> 
> +#ifndef HAVE_SKB_RESET_TAIL_POINTER
> +static inline void skb_reset_tail_pointer(struct sk_buff *skb)
> +{
> +	skb->tail = skb->data;
> +}
> +#endif

I'm assuming that this is backported by some kernel?