[...]
> > + tuplehash = flow_offload_lookup(flow_table, tuple);
> > + if (!tuplehash)
> > + return ERR_PTR(-ENOENT);
>
> This is fine to do, but the caller should catch it using IS_ERR_PTR
> and return NULL.
> BPF side cannot distinguish ERR_PTR from normal pointer, so this will
> cause a bad deref in the program.
ack, I will fix it in v2.
>
> > +
> > + flow = container_of(tuplehash, struct flow_offload,
> > + tuplehash[tuplehash->tuple.dir]);
> > + flow_offload_refresh(flow_table, flow, false);
> > +
> > + return tuplehash;
> > +}
> > +
> > +__bpf_kfunc struct flow_offload_tuple_rhash *
> > +bpf_xdp_flow_offload_lookup(struct xdp_md *ctx,
> > + struct bpf_fib_lookup *fib_tuple,
> > + u32 fib_tuple__sz)
>
> Do you think the __sz has the intended effect? It only works when the
> preceding parameter is a void *.
> If you have a type like struct bpf_fib_lookup, I think it should work
> fine without taking a size at all.
ack, I will fix it in v2.
>
> > +{
> > + struct xdp_buff *xdp = (struct xdp_buff *)ctx;
> > + struct flow_offload_tuple tuple = {
> > + .iifidx = fib_tuple->ifindex,
> > + .l3proto = fib_tuple->family,
> > + .l4proto = fib_tuple->l4_protocol,
> > + .src_port = fib_tuple->sport,
> > + .dst_port = fib_tuple->dport,
> > + };
> > + __be16 proto;
> > +
> > + switch (fib_tuple->family) {
> > + case AF_INET:
> > + tuple.src_v4.s_addr = fib_tuple->ipv4_src;
> > + tuple.dst_v4.s_addr = fib_tuple->ipv4_dst;
> > + proto = htons(ETH_P_IP);
> > + break;
> > + case AF_INET6:
> > + tuple.src_v6 = *(struct in6_addr *)&fib_tuple->ipv6_src;
> > + tuple.dst_v6 = *(struct in6_addr *)&fib_tuple->ipv6_dst;
> > + proto = htons(ETH_P_IPV6);
> > + break;
> > + default:
> > + return ERR_PTR(-EINVAL);
>
> Likewise. While you check IS_ERR_VALUE in selftest, direct dereference
> will be allowed by verifier, which would crash the kernel.
> It's better to do something like conntrack kfuncs, where they set
> opts->error when returning NULL, allowing better debugging in case
> lookup fails.
ack, I will fix it in v2.
>
> > + }
> > +
> > + return bpf_xdp_flow_offload_tuple_lookup(xdp->rxq->dev, &tuple, proto);
> > +}
> > +
> > +__diag_pop()
> > +
> > +BTF_KFUNCS_START(nf_ft_kfunc_set)
> > +BTF_ID_FLAGS(func, bpf_xdp_flow_offload_lookup)
> > +BTF_KFUNCS_END(nf_ft_kfunc_set)
> > +
> > +static const struct btf_kfunc_id_set nf_flow_offload_kfunc_set = {
> > + .owner = THIS_MODULE,
> > + .set = &nf_ft_kfunc_set,
> > +};
> > +
> > +int nf_flow_offload_register_bpf(void)
> > +{
> > + return register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP,
> > + &nf_flow_offload_kfunc_set);
> > +}
>
> We should probably also expose it to skb? We just need net_device, so
> it can work with both XDP and TC progs.
> That would be similar to how we expose conntrack kfuncs to both XDP
> and TC progs.
I think we will get very similar results to sw flowtable in this case,
don't you think?
>
> > +EXPORT_SYMBOL_GPL(nf_flow_offload_register_bpf);
> > diff --git a/net/netfilter/nf_flow_table_inet.c b/net/netfilter/nf_flow_table_inet.c
> > index 6eef15648b7b0..b13587238eceb 100644
> > --- a/net/netfilter/nf_flow_table_inet.c
> > +++ b/net/netfilter/nf_flow_table_inet.c
> > @@ -98,6 +98,8 @@ static int __init nf_flow_inet_module_init(void)
> > nft_register_flowtable_type(&flowtable_ipv6);
> > nft_register_flowtable_type(&flowtable_inet);
> >
> > + nf_flow_offload_register_bpf();
> > +
>
> Error checking needed here? Kfunc registration can fail.
ack, I will fix it.
Regards,
Lorenzo
>
> > return 0;
> > }
> >
> > --
> > 2.45.0
> >
> >
>
Attachment:
signature.asc
Description: PGP signature
