On Tue, 17 Oct 2017 14:48:12 +0200, Jiri Pirko wrote:
> Fri, Oct 13, 2017 at 03:08:24AM CEST, jakub.kicin...@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?
