[bpf-next,6/6] bpf: add test_run support for netfilter program type

Commit Message

Florian Westphal April 5, 2023, 4:11 p.m. UTC

also add two simple retval tests: as-is, a return value other
than accept or drop will cause issues.

NF_QUEUE could be implemented later IFF we can guarantee that
attachment of such programs can be rejected if they get attached
to a pf/hook that doesn't support async reinjection.

NF_STOLEN could be implemented via trusted helpers that will eventually
free the skb, else this would leak the skb reference.

Signed-off-by: Florian Westphal <[e-mail]>
---
 include/linux/bpf.h                           |   3 +
 net/bpf/test_run.c                            | 143 ++++++++++++++++++
 net/netfilter/nf_bpf_link.c                   |   1 +
 .../selftests/bpf/verifier/netfilter.c        |  23 +++
 4 files changed, 170 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/verifier/netfilter.c

Comments

kernel test robot April 5, 2023, 6:22 p.m. UTC | #1

Hi Florian,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Florian-Westphal/bpf-add-bpf_link-support-for-BPF_NETFILTER-programs/20230406-001447
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20230405161116.13565-7-fw%40strlen.de
patch subject: [PATCH bpf-next 6/6] bpf: add test_run support for netfilter program type
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20230406/[e-mail]/config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/7fba218dfc4942aa6781f4d1b5c475a0569cfd2e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Florian-Westphal/bpf-add-bpf_link-support-for-BPF_NETFILTER-programs/20230406-001447
        git checkout 7fba218dfc4942aa6781f4d1b5c475a0569cfd2e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash net/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[e-mail]>
| Link: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

All warnings (new ones prefixed by >>):

   net/bpf/test_run.c: In function 'bpf_prog_test_run_nf':
>> net/bpf/test_run.c:1750:30: warning: variable 'eth' set but not used [-Wunused-but-set-variable]
    1750 |         const struct ethhdr *eth;
         |                              ^~~


vim +/eth +1750 net/bpf/test_run.c

  1737	
  1738	int bpf_prog_test_run_nf(struct bpf_prog *prog,
  1739				 const union bpf_attr *kattr,
  1740				 union bpf_attr __user *uattr)
  1741	{
  1742		struct net *net = current->nsproxy->net_ns;
  1743		struct net_device *dev = net->loopback_dev;
  1744		struct nf_hook_state *user_ctx, hook_state = {
  1745			.pf = NFPROTO_IPV4,
  1746			.hook = NF_INET_PRE_ROUTING,
  1747		};
  1748		u32 size = kattr->test.data_size_in;
  1749		u32 repeat = kattr->test.repeat;
> 1750		const struct ethhdr *eth;
  1751		struct bpf_nf_ctx ctx = {
  1752			.state = &hook_state,
  1753		};
  1754		struct sk_buff *skb = NULL;
  1755		u32 retval, duration;
  1756		void *data;
  1757		int ret;
  1758	
  1759		if (kattr->test.flags || kattr->test.cpu || kattr->test.batch_size)
  1760			return -EINVAL;
  1761	
  1762		if (size < ETH_HLEN + sizeof(struct iphdr))
  1763			return -EINVAL;
  1764	
  1765		data = bpf_test_init(kattr, kattr->test.data_size_in, size,
  1766				     NET_SKB_PAD + NET_IP_ALIGN,
  1767				     SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
  1768		if (IS_ERR(data))
  1769			return PTR_ERR(data);
  1770	
  1771		eth = (struct ethhdr *)data;
  1772	
  1773		if (!repeat)
  1774			repeat = 1;
  1775	
  1776		user_ctx = bpf_ctx_init(kattr, sizeof(struct nf_hook_state));
  1777		if (IS_ERR(user_ctx)) {
  1778			kfree(data);
  1779			return PTR_ERR(user_ctx);
  1780		}
  1781	
  1782		if (user_ctx) {
  1783			ret = verify_and_copy_hook_state(&hook_state, user_ctx, dev);
  1784			if (ret)
  1785				goto out;
  1786		}
  1787	
  1788		skb = slab_build_skb(data);
  1789		if (!skb) {
  1790			ret = -ENOMEM;
  1791			goto out;
  1792		}
  1793	
  1794		data = NULL; /* data released via kfree_skb */
  1795	
  1796		skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN);
  1797		__skb_put(skb, size);
  1798	
  1799		skb->protocol = eth_type_trans(skb, dev);
  1800	
  1801		skb_reset_network_header(skb);
  1802	
  1803		ret = -EINVAL;
  1804	
  1805		switch (skb->protocol) {
  1806		case htons(ETH_P_IP):
  1807			if (hook_state.pf == NFPROTO_IPV4)
  1808				break;
  1809			goto out;
  1810		case htons(ETH_P_IPV6):
  1811			if (size < ETH_HLEN + sizeof(struct ipv6hdr))
  1812				goto out;
  1813			if (hook_state.pf == NFPROTO_IPV6)
  1814				break;
  1815			goto out;
  1816		default:
  1817			ret = -EPROTO;
  1818			goto out;
  1819		}
  1820	
  1821		ctx.skb = skb;
  1822	
  1823		ret = bpf_test_run(prog, &ctx, repeat, &retval, &duration, false);
  1824		if (ret)
  1825			goto out;
  1826	
  1827		ret = bpf_test_finish(kattr, uattr, NULL, NULL, 0, retval, duration);
  1828	
  1829	out:
  1830		kfree(user_ctx);
  1831		kfree_skb(skb);
  1832		kfree(data);
  1833		return ret;
  1834	}
  1835

Alexei Starovoitov April 7, 2023, 1:36 a.m. UTC | #2

On Wed, Apr 05, 2023 at 06:11:16PM +0200, Florian Westphal wrote:
> also add two simple retval tests: as-is, a return value other
> than accept or drop will cause issues.
> 
> NF_QUEUE could be implemented later IFF we can guarantee that
> attachment of such programs can be rejected if they get attached
> to a pf/hook that doesn't support async reinjection.
> 
> NF_STOLEN could be implemented via trusted helpers that will eventually
> free the skb, else this would leak the skb reference.
> 
> Signed-off-by: Florian Westphal <[e-mail]>
> ---
>  include/linux/bpf.h                           |   3 +
>  net/bpf/test_run.c                            | 143 ++++++++++++++++++
>  net/netfilter/nf_bpf_link.c                   |   1 +
>  .../selftests/bpf/verifier/netfilter.c        |  23 +++
>  4 files changed, 170 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/verifier/netfilter.c
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 2d8f3f639e68..453cee1efdd3 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2235,6 +2235,9 @@ int bpf_prog_test_run_raw_tp(struct bpf_prog *prog,
>  int bpf_prog_test_run_sk_lookup(struct bpf_prog *prog,
>  				const union bpf_attr *kattr,
>  				union bpf_attr __user *uattr);
> +int bpf_prog_test_run_nf(struct bpf_prog *prog,
> +			 const union bpf_attr *kattr,
> +			 union bpf_attr __user *uattr);
>  bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>  		    const struct bpf_prog *prog,
>  		    struct bpf_insn_access_aux *info);
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index f1652f5fbd2e..c14f577fd987 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -19,7 +19,9 @@
>  #include <linux/error-injection.h>
>  #include <linux/smp.h>
>  #include <linux/sock_diag.h>
> +#include <linux/netfilter.h>
>  #include <net/xdp.h>
> +#include <net/netfilter/nf_bpf_link.h>
>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/bpf_test_run.h>
> @@ -1690,6 +1692,147 @@ int bpf_prog_test_run_syscall(struct bpf_prog *prog,
>  	return err;
>  }
>  
> +static int verify_and_copy_hook_state(struct nf_hook_state *state,
> +				      const struct nf_hook_state *user,
> +				      struct net_device *dev)
> +{
> +	if (user->in || user->out)
> +		return -EINVAL;
> +
> +	if (user->net || user->sk || user->okfn)
> +		return -EINVAL;
> +
> +	switch (user->pf) {
> +	case NFPROTO_IPV4:
> +	case NFPROTO_IPV6:
> +		switch (state->hook) {
> +		case NF_INET_PRE_ROUTING:
> +			state->in = dev;
> +			break;
> +		case NF_INET_LOCAL_IN:
> +			state->in = dev;
> +			break;
> +		case NF_INET_FORWARD:
> +			state->in = dev;
> +			state->out = dev;
> +			break;
> +		case NF_INET_LOCAL_OUT:
> +			state->out = dev;
> +			break;
> +		case NF_INET_POST_ROUTING:
> +			state->out = dev;
> +			break;
> +		}
> +
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	state->pf = user->pf;
> +	state->hook = user->hook;
> +
> +	return 0;
> +}
> +
> +int bpf_prog_test_run_nf(struct bpf_prog *prog,
> +			 const union bpf_attr *kattr,
> +			 union bpf_attr __user *uattr)
> +{
> +	struct net *net = current->nsproxy->net_ns;
> +	struct net_device *dev = net->loopback_dev;
> +	struct nf_hook_state *user_ctx, hook_state = {
> +		.pf = NFPROTO_IPV4,
> +		.hook = NF_INET_PRE_ROUTING,
> +	};
> +	u32 size = kattr->test.data_size_in;
> +	u32 repeat = kattr->test.repeat;
> +	const struct ethhdr *eth;
> +	struct bpf_nf_ctx ctx = {
> +		.state = &hook_state,
> +	};
> +	struct sk_buff *skb = NULL;
> +	u32 retval, duration;
> +	void *data;
> +	int ret;
> +
> +	if (kattr->test.flags || kattr->test.cpu || kattr->test.batch_size)
> +		return -EINVAL;
> +
> +	if (size < ETH_HLEN + sizeof(struct iphdr))
> +		return -EINVAL;
> +
> +	data = bpf_test_init(kattr, kattr->test.data_size_in, size,
> +			     NET_SKB_PAD + NET_IP_ALIGN,
> +			     SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	eth = (struct ethhdr *)data;
> +
> +	if (!repeat)
> +		repeat = 1;
> +
> +	user_ctx = bpf_ctx_init(kattr, sizeof(struct nf_hook_state));
> +	if (IS_ERR(user_ctx)) {
> +		kfree(data);
> +		return PTR_ERR(user_ctx);
> +	}
> +
> +	if (user_ctx) {
> +		ret = verify_and_copy_hook_state(&hook_state, user_ctx, dev);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	skb = slab_build_skb(data);
> +	if (!skb) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	data = NULL; /* data released via kfree_skb */
> +
> +	skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN);
> +	__skb_put(skb, size);
> +
> +	skb->protocol = eth_type_trans(skb, dev);
> +
> +	skb_reset_network_header(skb);
> +
> +	ret = -EINVAL;
> +
> +	switch (skb->protocol) {
> +	case htons(ETH_P_IP):
> +		if (hook_state.pf == NFPROTO_IPV4)
> +			break;
> +		goto out;
> +	case htons(ETH_P_IPV6):
> +		if (size < ETH_HLEN + sizeof(struct ipv6hdr))
> +			goto out;
> +		if (hook_state.pf == NFPROTO_IPV6)
> +			break;
> +		goto out;
> +	default:
> +		ret = -EPROTO;
> +		goto out;
> +	}
> +
> +	ctx.skb = skb;
> +
> +	ret = bpf_test_run(prog, &ctx, repeat, &retval, &duration, false);
> +	if (ret)
> +		goto out;
> +
> +	ret = bpf_test_finish(kattr, uattr, NULL, NULL, 0, retval, duration);
> +
> +out:
> +	kfree(user_ctx);
> +	kfree_skb(skb);
> +	kfree(data);
> +	return ret;
> +}
> +
>  static const struct btf_kfunc_id_set bpf_prog_test_kfunc_set = {
>  	.owner = THIS_MODULE,
>  	.set   = &test_sk_check_kfunc_ids,
> diff --git a/net/netfilter/nf_bpf_link.c b/net/netfilter/nf_bpf_link.c
> index 4b22a31d6df5..c27fd569adf1 100644
> --- a/net/netfilter/nf_bpf_link.c
> +++ b/net/netfilter/nf_bpf_link.c
> @@ -128,6 +128,7 @@ int bpf_nf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
>  }
>  
>  const struct bpf_prog_ops netfilter_prog_ops = {
> +	.test_run = bpf_prog_test_run_nf,
>  };
>  
>  static bool nf_ptr_to_btf_id(struct bpf_insn_access_aux *info, const char *name)
> diff --git a/tools/testing/selftests/bpf/verifier/netfilter.c b/tools/testing/selftests/bpf/verifier/netfilter.c
> new file mode 100644
> index 000000000000..deeb87afdf50
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/verifier/netfilter.c
> @@ -0,0 +1,23 @@
> +{
> +	"netfilter, accept all",
> +	.insns = {
> +	BPF_MOV64_IMM(BPF_REG_0, 1),
> +	BPF_EXIT_INSN(),
> +	},
> +	.result = ACCEPT,
> +	.prog_type = BPF_PROG_TYPE_NETFILTER,
> +	.retval = 1,
> +	.data = {
> +		0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0x08, 0x00,
> +	},
> +},
> +{
> +	"netfilter, stolen verdict",
> +	.insns = {
> +	BPF_MOV64_IMM(BPF_REG_0, 2),
> +	BPF_EXIT_INSN(),
> +	},
> +	.result = REJECT,
> +	.errstr = "At program exit the register R0 has value (0x2; 0x0) should have been in (0x0; 0x1)",
> +	.prog_type = BPF_PROG_TYPE_NETFILTER,

We're adding all new asm tests to test_progs now instead of test_verifier. See progs/verifier_*.c.

Other than this nit and build bot complains it looks good to me.

Florian Westphal April 8, 2023, 9:38 p.m. UTC | #3

Alexei Starovoitov <[e-mail]> wrote:
> >  static bool nf_ptr_to_btf_id(struct bpf_insn_access_aux *info, const char *name)
> > diff --git a/tools/testing/selftests/bpf/verifier/netfilter.c b/tools/testing/selftests/bpf/verifier/netfilter.c
> > new file mode 100644
> > index 000000000000..deeb87afdf50
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/verifier/netfilter.c
> > @@ -0,0 +1,23 @@
> > +{
> > +	"netfilter, accept all",

[..]

> We're adding all new asm tests to test_progs now instead of test_verifier. See progs/verifier_*.c.

Thanks for the pointer, I'll have a look at this for v2.

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 2d8f3f639e68..453cee1efdd3 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2235,6 +2235,9 @@  int bpf_prog_test_run_raw_tp(struct bpf_prog *prog,
 int bpf_prog_test_run_sk_lookup(struct bpf_prog *prog,
 				const union bpf_attr *kattr,
 				union bpf_attr __user *uattr);
+int bpf_prog_test_run_nf(struct bpf_prog *prog,
+			 const union bpf_attr *kattr,
+			 union bpf_attr __user *uattr);
 bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 		    const struct bpf_prog *prog,
 		    struct bpf_insn_access_aux *info);
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index f1652f5fbd2e..c14f577fd987 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -19,7 +19,9 @@ 
 #include <linux/error-injection.h>
 #include <linux/smp.h>
 #include <linux/sock_diag.h>
+#include <linux/netfilter.h>
 #include <net/xdp.h>
+#include <net/netfilter/nf_bpf_link.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/bpf_test_run.h>
@@ -1690,6 +1692,147 @@  int bpf_prog_test_run_syscall(struct bpf_prog *prog,
 	return err;
 }
 
+static int verify_and_copy_hook_state(struct nf_hook_state *state,
+				      const struct nf_hook_state *user,
+				      struct net_device *dev)
+{
+	if (user->in || user->out)
+		return -EINVAL;
+
+	if (user->net || user->sk || user->okfn)
+		return -EINVAL;
+
+	switch (user->pf) {
+	case NFPROTO_IPV4:
+	case NFPROTO_IPV6:
+		switch (state->hook) {
+		case NF_INET_PRE_ROUTING:
+			state->in = dev;
+			break;
+		case NF_INET_LOCAL_IN:
+			state->in = dev;
+			break;
+		case NF_INET_FORWARD:
+			state->in = dev;
+			state->out = dev;
+			break;
+		case NF_INET_LOCAL_OUT:
+			state->out = dev;
+			break;
+		case NF_INET_POST_ROUTING:
+			state->out = dev;
+			break;
+		}
+
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	state->pf = user->pf;
+	state->hook = user->hook;
+
+	return 0;
+}
+
+int bpf_prog_test_run_nf(struct bpf_prog *prog,
+			 const union bpf_attr *kattr,
+			 union bpf_attr __user *uattr)
+{
+	struct net *net = current->nsproxy->net_ns;
+	struct net_device *dev = net->loopback_dev;
+	struct nf_hook_state *user_ctx, hook_state = {
+		.pf = NFPROTO_IPV4,
+		.hook = NF_INET_PRE_ROUTING,
+	};
+	u32 size = kattr->test.data_size_in;
+	u32 repeat = kattr->test.repeat;
+	const struct ethhdr *eth;
+	struct bpf_nf_ctx ctx = {
+		.state = &hook_state,
+	};
+	struct sk_buff *skb = NULL;
+	u32 retval, duration;
+	void *data;
+	int ret;
+
+	if (kattr->test.flags || kattr->test.cpu || kattr->test.batch_size)
+		return -EINVAL;
+
+	if (size < ETH_HLEN + sizeof(struct iphdr))
+		return -EINVAL;
+
+	data = bpf_test_init(kattr, kattr->test.data_size_in, size,
+			     NET_SKB_PAD + NET_IP_ALIGN,
+			     SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	eth = (struct ethhdr *)data;
+
+	if (!repeat)
+		repeat = 1;
+
+	user_ctx = bpf_ctx_init(kattr, sizeof(struct nf_hook_state));
+	if (IS_ERR(user_ctx)) {
+		kfree(data);
+		return PTR_ERR(user_ctx);
+	}
+
+	if (user_ctx) {
+		ret = verify_and_copy_hook_state(&hook_state, user_ctx, dev);
+		if (ret)
+			goto out;
+	}
+
+	skb = slab_build_skb(data);
+	if (!skb) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	data = NULL; /* data released via kfree_skb */
+
+	skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN);
+	__skb_put(skb, size);
+
+	skb->protocol = eth_type_trans(skb, dev);
+
+	skb_reset_network_header(skb);
+
+	ret = -EINVAL;
+
+	switch (skb->protocol) {
+	case htons(ETH_P_IP):
+		if (hook_state.pf == NFPROTO_IPV4)
+			break;
+		goto out;
+	case htons(ETH_P_IPV6):
+		if (size < ETH_HLEN + sizeof(struct ipv6hdr))
+			goto out;
+		if (hook_state.pf == NFPROTO_IPV6)
+			break;
+		goto out;
+	default:
+		ret = -EPROTO;
+		goto out;
+	}
+
+	ctx.skb = skb;
+
+	ret = bpf_test_run(prog, &ctx, repeat, &retval, &duration, false);
+	if (ret)
+		goto out;
+
+	ret = bpf_test_finish(kattr, uattr, NULL, NULL, 0, retval, duration);
+
+out:
+	kfree(user_ctx);
+	kfree_skb(skb);
+	kfree(data);
+	return ret;
+}
+
 static const struct btf_kfunc_id_set bpf_prog_test_kfunc_set = {
 	.owner = THIS_MODULE,
 	.set   = &test_sk_check_kfunc_ids,
diff --git a/net/netfilter/nf_bpf_link.c b/net/netfilter/nf_bpf_link.c
index 4b22a31d6df5..c27fd569adf1 100644
--- a/net/netfilter/nf_bpf_link.c
+++ b/net/netfilter/nf_bpf_link.c
@@ -128,6 +128,7 @@  int bpf_nf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 }
 
 const struct bpf_prog_ops netfilter_prog_ops = {
+	.test_run = bpf_prog_test_run_nf,
 };
 
 static bool nf_ptr_to_btf_id(struct bpf_insn_access_aux *info, const char *name)
diff --git a/tools/testing/selftests/bpf/verifier/netfilter.c b/tools/testing/selftests/bpf/verifier/netfilter.c
new file mode 100644
index 000000000000..deeb87afdf50
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/netfilter.c
@@ -0,0 +1,23 @@ 
+{
+	"netfilter, accept all",
+	.insns = {
+	BPF_MOV64_IMM(BPF_REG_0, 1),
+	BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+	.prog_type = BPF_PROG_TYPE_NETFILTER,
+	.retval = 1,
+	.data = {
+		0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0x08, 0x00,
+	},
+},
+{
+	"netfilter, stolen verdict",
+	.insns = {
+	BPF_MOV64_IMM(BPF_REG_0, 2),
+	BPF_EXIT_INSN(),
+	},
+	.result = REJECT,
+	.errstr = "At program exit the register R0 has value (0x2; 0x0) should have been in (0x0; 0x1)",
+	.prog_type = BPF_PROG_TYPE_NETFILTER,
+},