diff mbox series

[net-next,27/34] nfp: bpf: Convert ndo_setup_tc offloads to block callbacks

Message ID 20171012171823.1431-28-jiri@resnulli.us
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series net: sched: allow qdiscs to share filter block instances | expand

Commit Message

Jiri Pirko Oct. 12, 2017, 5:18 p.m. UTC 
From: Jiri Pirko <jiri@mellanox.com>

Benefit from the newly introduced block callback infrastructure and
convert ndo_setup_tc calls for bpf offloads to block callbacks.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/netronome/nfp/bpf/main.c    | 53 +++++++++++++++++++-----
 drivers/net/ethernet/netronome/nfp/bpf/offload.c |  4 ++
 2 files changed, 47 insertions(+), 10 deletions(-)

Comments

Jakub Kicinski Oct. 13, 2017, 1:08 a.m. UTC | #1 
On Thu, 12 Oct 2017 19:18:16 +0200, Jiri Pirko wrote:
> diff --git a/drivers/net/ethernet/netronome/nfp/bpf/offload.c b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
> index a88bb5b..9e9af88 100644
> --- a/drivers/net/ethernet/netronome/nfp/bpf/offload.c
> +++ b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
> @@ -246,6 +246,10 @@ int nfp_net_bpf_offload(struct nfp_net *nn, struct tc_cls_bpf_offload *cls_bpf)
>  	void *code;
>  	int err;
>  
> +	if (cls_bpf->common.protocol != htons(ETH_P_ALL) ||
> +	    cls_bpf->common.chain_index)
> +		return -EOPNOTSUPP;
> +
>  	max_instr = nn_readw(nn, NFP_NET_CFG_BPF_MAX_LEN);
>  
>  	switch (cls_bpf->command) {

It is certainly very ugly but I send a fake struct tc_cls_bpf_offload
here for XDP.  Refactoring this mess is pretty high on my priority list
but one way or the other this function will be called from XDP so TC
checks must stay in the TC handler... :(
Jiri Pirko Oct. 17, 2017, 12:48 p.m. UTC | #2 
Fri, Oct 13, 2017 at 03:08:24AM CEST, jakub.kicinski@netronome.com wrote:
>On Thu, 12 Oct 2017 19:18:16 +0200, Jiri Pirko wrote:
>> diff --git a/drivers/net/ethernet/netronome/nfp/bpf/offload.c b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
>> index a88bb5b..9e9af88 100644
>> --- a/drivers/net/ethernet/netronome/nfp/bpf/offload.c
>> +++ b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
>> @@ -246,6 +246,10 @@ int nfp_net_bpf_offload(struct nfp_net *nn, struct tc_cls_bpf_offload *cls_bpf)
>>  	void *code;
>>  	int err;
>>  
>> +	if (cls_bpf->common.protocol != htons(ETH_P_ALL) ||
>> +	    cls_bpf->common.chain_index)
>> +		return -EOPNOTSUPP;
>> +
>>  	max_instr = nn_readw(nn, NFP_NET_CFG_BPF_MAX_LEN);
>>  
>>  	switch (cls_bpf->command) {
>
>It is certainly very ugly but I send a fake struct tc_cls_bpf_offload
>here for XDP.  Refactoring this mess is pretty high on my priority list
>but one way or the other this function will be called from XDP so TC
>checks must stay in the TC handler... :(

Okay. But currently, why is it a problem? You don't need the checks for
xdp path.
Jakub Kicinski Oct. 17, 2017, 2:39 p.m. UTC | #3 
On Tue, 17 Oct 2017 14:48:12 +0200, Jiri Pirko wrote:
> Fri, Oct 13, 2017 at 03:08:24AM CEST, jakub.kicinski@netronome.com wrote:
> >On Thu, 12 Oct 2017 19:18:16 +0200, Jiri Pirko wrote:  
> >> diff --git a/drivers/net/ethernet/netronome/nfp/bpf/offload.c b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
> >> index a88bb5b..9e9af88 100644
> >> --- a/drivers/net/ethernet/netronome/nfp/bpf/offload.c
> >> +++ b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
> >> @@ -246,6 +246,10 @@ int nfp_net_bpf_offload(struct nfp_net *nn, struct tc_cls_bpf_offload *cls_bpf)
> >>  	void *code;
> >>  	int err;
> >>  
> >> +	if (cls_bpf->common.protocol != htons(ETH_P_ALL) ||
> >> +	    cls_bpf->common.chain_index)
> >> +		return -EOPNOTSUPP;
> >> +
> >>  	max_instr = nn_readw(nn, NFP_NET_CFG_BPF_MAX_LEN);
> >>  
> >>  	switch (cls_bpf->command) {  
> >
> >It is certainly very ugly but I send a fake struct tc_cls_bpf_offload
> >here for XDP.  Refactoring this mess is pretty high on my priority list
> >but one way or the other this function will be called from XDP so TC
> >checks must stay in the TC handler... :(  
> 
> Okay. But currently, why is it a problem? You don't need the checks for
> xdp path.
> 

static int
nfp_bpf_xdp_offload(struct nfp_app *app, struct nfp_net *nn,
		    struct bpf_prog *prog)
{
	struct tc_cls_bpf_offload cmd = {
		.prog = prog,
	};
	int ret;

	if (!nfp_net_ebpf_capable(nn))
		return -EINVAL;

	if (nn->dp.ctrl & NFP_NET_CFG_CTRL_BPF) {
		if (!nn->dp.bpf_offload_xdp)
			return prog ? -EBUSY : 0;
		cmd.command = prog ? TC_CLSBPF_REPLACE : TC_CLSBPF_DESTROY;
	} else {
		if (!prog)
			return 0;
		cmd.command = TC_CLSBPF_ADD;
	}

	ret = nfp_net_bpf_offload(nn, &cmd);
	/* Stop offload if replace not possible */
	if (ret && cmd.command == TC_CLSBPF_REPLACE)
		nfp_bpf_xdp_offload(app, nn, NULL);
	nn->dp.bpf_offload_xdp = prog && !ret;
	return ret;
}

The fake offload struct is at the top of this function.  Dereferencing
cls_bpf->common in nfp_net_bpf_offload() will crash the kernel.  Or am
I missing something?
Jiri Pirko Oct. 17, 2017, 6:16 p.m. UTC | #4 
Tue, Oct 17, 2017 at 04:39:59PM CEST, jakub.kicinski@netronome.com wrote:
>On Tue, 17 Oct 2017 14:48:12 +0200, Jiri Pirko wrote:
>> Fri, Oct 13, 2017 at 03:08:24AM CEST, jakub.kicinski@netronome.com wrote:
>> >On Thu, 12 Oct 2017 19:18:16 +0200, Jiri Pirko wrote:  
>> >> diff --git a/drivers/net/ethernet/netronome/nfp/bpf/offload.c b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
>> >> index a88bb5b..9e9af88 100644
>> >> --- a/drivers/net/ethernet/netronome/nfp/bpf/offload.c
>> >> +++ b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
>> >> @@ -246,6 +246,10 @@ int nfp_net_bpf_offload(struct nfp_net *nn, struct tc_cls_bpf_offload *cls_bpf)
>> >>  	void *code;
>> >>  	int err;
>> >>  
>> >> +	if (cls_bpf->common.protocol != htons(ETH_P_ALL) ||
>> >> +	    cls_bpf->common.chain_index)
>> >> +		return -EOPNOTSUPP;
>> >> +
>> >>  	max_instr = nn_readw(nn, NFP_NET_CFG_BPF_MAX_LEN);
>> >>  
>> >>  	switch (cls_bpf->command) {  
>> >
>> >It is certainly very ugly but I send a fake struct tc_cls_bpf_offload
>> >here for XDP.  Refactoring this mess is pretty high on my priority list
>> >but one way or the other this function will be called from XDP so TC
>> >checks must stay in the TC handler... :(  
>> 
>> Okay. But currently, why is it a problem? You don't need the checks for
>> xdp path.
>> 
>
>static int
>nfp_bpf_xdp_offload(struct nfp_app *app, struct nfp_net *nn,
>		    struct bpf_prog *prog)
>{
>	struct tc_cls_bpf_offload cmd = {
>		.prog = prog,
>	};
>	int ret;
>
>	if (!nfp_net_ebpf_capable(nn))
>		return -EINVAL;
>
>	if (nn->dp.ctrl & NFP_NET_CFG_CTRL_BPF) {
>		if (!nn->dp.bpf_offload_xdp)
>			return prog ? -EBUSY : 0;
>		cmd.command = prog ? TC_CLSBPF_REPLACE : TC_CLSBPF_DESTROY;
>	} else {
>		if (!prog)
>			return 0;
>		cmd.command = TC_CLSBPF_ADD;
>	}
>
>	ret = nfp_net_bpf_offload(nn, &cmd);
>	/* Stop offload if replace not possible */
>	if (ret && cmd.command == TC_CLSBPF_REPLACE)
>		nfp_bpf_xdp_offload(app, nn, NULL);
>	nn->dp.bpf_offload_xdp = prog && !ret;
>	return ret;
>}
>
>The fake offload struct is at the top of this function.  Dereferencing
>cls_bpf->common in nfp_net_bpf_offload() will crash the kernel.  Or am
>I missing something?

We just have to init it. Should not be a problem. Will add it.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c
index 0747269..be7607b 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.c
@@ -112,22 +112,55 @@  static void nfp_bpf_vnic_free(struct nfp_app *app, struct nfp_net *nn)
 	kfree(nn->app_priv);
 }
 
-static int nfp_bpf_setup_tc(struct nfp_app *app, struct net_device *netdev,
-			    enum tc_setup_type type, void *type_data)
+static int nfp_bpf_setup_tc_block_cb(enum tc_setup_type type,
+				     void *type_data, void *cb_priv)
+{
+	struct nfp_net *nn = cb_priv;
+
+	switch (type) {
+	case TC_SETUP_CLSBPF:
+		if (!nfp_net_ebpf_capable(nn))
+			return -EOPNOTSUPP;
+		return nfp_net_bpf_offload(nn, type_data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int nfp_bpf_setup_tc_block(struct net_device *netdev,
+				  struct tc_block_offload *f)
 {
-	struct tc_cls_bpf_offload *cls_bpf = type_data;
 	struct nfp_net *nn = netdev_priv(netdev);
 
-	if (type != TC_SETUP_CLSBPF || !nfp_net_ebpf_capable(nn) ||
-	    !is_classid_clsact_ingress(cls_bpf->common.classid) ||
-	    cls_bpf->common.protocol != htons(ETH_P_ALL) ||
-	    cls_bpf->common.chain_index)
+	if (f->binder_type != TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
 		return -EOPNOTSUPP;
 
-	if (nn->dp.bpf_offload_xdp)
-		return -EBUSY;
+	switch (f->command) {
+	case TC_BLOCK_BIND:
+		return tcf_block_cb_register(f->block,
+					     nfp_bpf_setup_tc_block_cb,
+					     nn, nn);
+	case TC_BLOCK_UNBIND:
+		tcf_block_cb_unregister(f->block,
+					nfp_bpf_setup_tc_block_cb,
+					nn);
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
 
-	return nfp_net_bpf_offload(nn, cls_bpf);
+static int nfp_bpf_setup_tc(struct nfp_app *app, struct net_device *netdev,
+			    enum tc_setup_type type, void *type_data)
+{
+	switch (type) {
+	case TC_SETUP_CLSBPF:
+		return 0; /* will be removed after conversion from ndo */
+	case TC_SETUP_BLOCK:
+		return nfp_bpf_setup_tc_block(netdev, type_data);
+	default:
+		return -EOPNOTSUPP;
+	}
 }
 
 static bool nfp_bpf_tc_busy(struct nfp_app *app, struct nfp_net *nn)
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/offload.c b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
index a88bb5b..9e9af88 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
@@ -246,6 +246,10 @@  int nfp_net_bpf_offload(struct nfp_net *nn, struct tc_cls_bpf_offload *cls_bpf)
 	void *code;
 	int err;
 
+	if (cls_bpf->common.protocol != htons(ETH_P_ALL) ||
+	    cls_bpf->common.chain_index)
+		return -EOPNOTSUPP;
+
 	max_instr = nn_readw(nn, NFP_NET_CFG_BPF_MAX_LEN);
 
 	switch (cls_bpf->command) {