netdev - RE: [PATCH RFC v2 1/8] xdp: Infrastructure to generalize XDP

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BL2PR07MB2306990912A16BD6B9A5FE3F8D450@BL2PR07MB2306.namprd07.prod.outlook.com>
Date:   Thu, 9 Feb 2017 14:22:34 +0000
From:   "Mintz, Yuval" <Yuval.Mintz@...ium.com>
To:     Tom Herbert <tom@...bertland.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "kernel-team@...com" <kernel-team@...com>
Subject: RE: [PATCH RFC v2 1/8] xdp: Infrastructure to generalize XDP

> + * Fields:
> + *
> + *   priority - priority for insertion into set. The set is ordered lowest to
> + *	highest priority.
> + *   is_bpf - indicates that the hook is a BPF program (priv refers to a
> + *	bpf_prog structure. This allows calling the BPF program directly
> + *	from xdp_run without a extra level of indirection.
> + *   hookfn - function to call when hook are run.
> + *   priv - private data associated with hook. This is passed as an argument
> + *	to the hook function (in the case of BPF this is a bpf_prog structure).
> + *   put_priv - function call when XDP is done with private data.
> + *   def - point to definitions of xdp_hook. The pointer value is saved as

def->template

> + *      a refernce the instance of hook loaded (used to find and unregister a
> + *      hook).
> + *   tag - readable tag for reporting purposes
> + */
> +struct xdp_hook {
> +	int priority;
> +	bool is_bpf;
> +	xdp_hookfn *hookfn;
> +	void __rcu *priv;
> +	xdp_put_privfn *put_priv;
> +	const struct xdp_hook *template;
> +	u8 tag[XDP_TAG_SIZE];
> +};

...

> +static inline int __xdp_run_one_hook(struct xdp_hook *hook,
> +				     struct xdp_buff *xdp)
> +{
> +	void *priv = rcu_dereference(hook->priv);
> +
> +	if (hook->is_bpf) {

Shouldn't this branch be 'likely' [until we have other flavors of xdp]?

> +		/* Run BPF programs directly do avoid one layer of
> +		 * indirection.
> +		 */
> +		return BPF_PROG_RUN((struct bpf_prog *)priv, (void *)xdp);
> +	} else {
> +		return hook->hookfn(priv, xdp);
> +	}
> +}
> +
> +/* Core function to run the XDP hooks. This must be as fast as possible
> +*/ static inline int __xdp_hook_run(struct xdp_hook_set *hook_set,
> +				 struct xdp_buff *xdp,
> +				 struct xdp_hook **last_hook)
> +{
> +	struct xdp_hook *hook;
> +	int i, ret;
> +
> +	if (unlikely(!hook_set))
> +		return XDP_PASS;
> +
> +	hook = &hook_set->hooks[0];
> +	ret = __xdp_run_one_hook(hook, xdp);
> +	*last_hook = hook;

Not setting last_hook in loop; Probably failing the 'last' intention.

> +	for (i = 1; i < hook_set->num; i++) {
> +		if (ret != XDP_PASS)
> +			break;
> +		hook = &hook_set->hooks[i];
> +		ret = __xdp_run_one_hook(hook, xdp);
> +	}
> +
> +	return ret;
> +}
...

> +/* Run a BPF/XDP program. RCU read lock must be held */ static u32
> +dev_bpf_prog_run_xdp(const void *priv,
> +				struct xdp_buff *xdp)
> +{
> +	const struct bpf_prog *prog = (const struct bpf_prog *)priv;
> +
> +	return BPF_PROG_RUN(prog, (void *)xdp); }
> +
> +static void dev_bpf_prog_put_xdp(const void *priv) {
> +	bpf_prog_put((struct bpf_prog *)priv); }
> +
> +struct xdp_hook xdp_bpf_hook = {
> +	.hookfn = dev_bpf_prog_run_xdp,
> +	.put_priv = dev_bpf_prog_put_xdp,
> +	.priority = 0,
> +	.is_bpf = true
> +};

What's the purpose of populating hookfn,
if for performance you've chosen the function based on 'is_bpf'?

...

> -	if (!dev->netdev_ops->ndo_xdp)
> +	if (!(dev->features & NETIF_F_XDP))
>  		return 0;

This should probably go in the cleanup patch.