| Message ID | 155966185078.9084.7775851923786129736.stgit@alrua-x1 |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | xdp: Allow lookup into devmaps before redirect | expand |
On Tue, 04 Jun 2019 17:24:10 +0200 Toke Høiland-Jørgensen <toke@redhat.com> wrote: > We don't currently allow lookups into a devmap from eBPF, because the map > lookup returns a pointer directly to the dev->ifindex, which shouldn't be > modifiable from eBPF. > > However, being able to do lookups in devmaps is useful to know (e.g.) > whether forwarding to a specific interface is enabled. Currently, programs > work around this by keeping a shadow map of another type which indicates > whether a map index is valid. > > To allow lookups, simply copy the ifindex into a scratch variable and > return a pointer to this. If an eBPF program does modify it, this doesn't > matter since it will be overridden on the next lookup anyway. While this > does add a write to every lookup, the overhead of this is negligible > because the cache line is hot when both the write and the subsequent > read happens. When we choose the return value, here the ifindex, then this basically becomes UABI, right? Can we somehow use BTF to help us to make this extensible? As Toke mention in the cover letter, we really want to know if the chosen egress have actually enabled/allocated resources for XDP transmitting, but as we currently don't have in-kernel way to query thus (thus, we cannot expose such info). > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- > kernel/bpf/devmap.c | 8 +++++++- > kernel/bpf/verifier.c | 7 ++----- > 2 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c > index 5ae7cce5ef16..830650300ea4 100644 > --- a/kernel/bpf/devmap.c > +++ b/kernel/bpf/devmap.c > @@ -65,6 +65,7 @@ struct xdp_bulk_queue { > struct bpf_dtab_netdev { > struct net_device *dev; /* must be first member, due to tracepoint */ > struct bpf_dtab *dtab; > + int ifindex_scratch; > unsigned int bit; > struct xdp_bulk_queue __percpu *bulkq; > struct rcu_head rcu; > @@ -375,7 +376,12 @@ static void *dev_map_lookup_elem(struct bpf_map *map, void *key) > struct bpf_dtab_netdev *obj = __dev_map_lookup_elem(map, *(u32 *)key); > struct net_device *dev = obj ? obj->dev : NULL; > > - return dev ? &dev->ifindex : NULL; > + if (dev) { > + obj->ifindex_scratch = dev->ifindex; > + return &obj->ifindex_scratch; > + } > + > + return NULL; > } > > static void dev_map_flush_old(struct bpf_dtab_netdev *dev) > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 5c2cb5bd84ce..7128a9821481 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -2893,12 +2893,9 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env, > if (func_id != BPF_FUNC_get_local_storage) > goto error; > break; > - /* devmap returns a pointer to a live net_device ifindex that we cannot > - * allow to be modified from bpf side. So do not allow lookup elements > - * for now. > - */ > case BPF_MAP_TYPE_DEVMAP: > - if (func_id != BPF_FUNC_redirect_map) > + if (func_id != BPF_FUNC_redirect_map && > + func_id != BPF_FUNC_map_lookup_elem) > goto error; > break; > /* Restrict bpf side of cpumap and xskmap, open when use-cases >
Jesper Dangaard Brouer <brouer@redhat.com> writes: > On Tue, 04 Jun 2019 17:24:10 +0200 > Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> We don't currently allow lookups into a devmap from eBPF, because the map >> lookup returns a pointer directly to the dev->ifindex, which shouldn't be >> modifiable from eBPF. >> >> However, being able to do lookups in devmaps is useful to know (e.g.) >> whether forwarding to a specific interface is enabled. Currently, programs >> work around this by keeping a shadow map of another type which indicates >> whether a map index is valid. >> >> To allow lookups, simply copy the ifindex into a scratch variable and >> return a pointer to this. If an eBPF program does modify it, this doesn't >> matter since it will be overridden on the next lookup anyway. While this >> does add a write to every lookup, the overhead of this is negligible >> because the cache line is hot when both the write and the subsequent >> read happens. > > When we choose the return value, here the ifindex, then this basically > becomes UABI, right? Well, we already have UABI on the insert side, where the value being inserted has to be an ifindex. And we enforce value_size==4 when creating the map. So IMO I'm just keeping to the already established UAPI here. That being said... > Can we somehow use BTF to help us to make this extensible? ... this would not necessarily be a bad thing, it just needs to be done on both the insert and lookup sides. But I think this is a separate issue, which we need to solve anyway. And I'm still not convinced that the map value is the right place to specify what resources we want ;) -Toke
On 4 Jun 2019, at 9:35, Jesper Dangaard Brouer wrote: > On Tue, 04 Jun 2019 17:24:10 +0200 > Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> We don't currently allow lookups into a devmap from eBPF, because the map >> lookup returns a pointer directly to the dev->ifindex, which shouldn't be >> modifiable from eBPF. >> >> However, being able to do lookups in devmaps is useful to know (e.g.) >> whether forwarding to a specific interface is enabled. Currently, programs >> work around this by keeping a shadow map of another type which indicates >> whether a map index is valid. >> >> To allow lookups, simply copy the ifindex into a scratch variable and >> return a pointer to this. If an eBPF program does modify it, this doesn't >> matter since it will be overridden on the next lookup anyway. While this >> does add a write to every lookup, the overhead of this is negligible >> because the cache line is hot when both the write and the subsequent >> read happens. > > When we choose the return value, here the ifindex, then this basically > becomes UABI, right? > > Can we somehow use BTF to help us to make this extensible? > > As Toke mention in the cover letter, we really want to know if the > chosen egress have actually enabled/allocated resources for XDP > transmitting, but as we currently don't have in-kernel way to query > thus (thus, we cannot expose such info). Would it be better to add a helper like bpf_map_element_present(), which just returns a boolean value indicating whether the entry is NULL or not? This would solve this problem (and my xskmap problem).
Jonathan Lemon <jonathan.lemon@gmail.com> writes: > On 4 Jun 2019, at 9:35, Jesper Dangaard Brouer wrote: > >> On Tue, 04 Jun 2019 17:24:10 +0200 >> Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >>> We don't currently allow lookups into a devmap from eBPF, because the map >>> lookup returns a pointer directly to the dev->ifindex, which shouldn't be >>> modifiable from eBPF. >>> >>> However, being able to do lookups in devmaps is useful to know (e.g.) >>> whether forwarding to a specific interface is enabled. Currently, programs >>> work around this by keeping a shadow map of another type which indicates >>> whether a map index is valid. >>> >>> To allow lookups, simply copy the ifindex into a scratch variable and >>> return a pointer to this. If an eBPF program does modify it, this doesn't >>> matter since it will be overridden on the next lookup anyway. While this >>> does add a write to every lookup, the overhead of this is negligible >>> because the cache line is hot when both the write and the subsequent >>> read happens. >> >> When we choose the return value, here the ifindex, then this basically >> becomes UABI, right? >> >> Can we somehow use BTF to help us to make this extensible? >> >> As Toke mention in the cover letter, we really want to know if the >> chosen egress have actually enabled/allocated resources for XDP >> transmitting, but as we currently don't have in-kernel way to query >> thus (thus, we cannot expose such info). > > Would it be better to add a helper like bpf_map_element_present(), which > just returns a boolean value indicating whether the entry is NULL or not? > > This would solve this problem (and my xskmap problem). Ah, totally missed that other thread; will go reply there :) -Toke
Toke Høiland-Jørgensen <toke@redhat.com> writes: > We don't currently allow lookups into a devmap from eBPF, because the map > lookup returns a pointer directly to the dev->ifindex, which shouldn't be > modifiable from eBPF. > > However, being able to do lookups in devmaps is useful to know (e.g.) > whether forwarding to a specific interface is enabled. Currently, programs > work around this by keeping a shadow map of another type which indicates > whether a map index is valid. > > To allow lookups, simply copy the ifindex into a scratch variable and > return a pointer to this. If an eBPF program does modify it, this doesn't > matter since it will be overridden on the next lookup anyway. While this > does add a write to every lookup, the overhead of this is negligible > because the cache line is hot when both the write and the subsequent read > happens. > > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- > kernel/bpf/devmap.c | 8 +++++++- > kernel/bpf/verifier.c | 7 ++----- > 2 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c > index 5ae7cce5ef16..830650300ea4 100644 > --- a/kernel/bpf/devmap.c > +++ b/kernel/bpf/devmap.c > @@ -65,6 +65,7 @@ struct xdp_bulk_queue { > struct bpf_dtab_netdev { > struct net_device *dev; /* must be first member, due to tracepoint */ > struct bpf_dtab *dtab; > + int ifindex_scratch; Just realised I forgot to make this per-cpu; I'll send an updated version once we settle on a solution that works for xskmap as well... -Toke
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c index 5ae7cce5ef16..830650300ea4 100644 --- a/kernel/bpf/devmap.c +++ b/kernel/bpf/devmap.c @@ -65,6 +65,7 @@ struct xdp_bulk_queue { struct bpf_dtab_netdev { struct net_device *dev; /* must be first member, due to tracepoint */ struct bpf_dtab *dtab; + int ifindex_scratch; unsigned int bit; struct xdp_bulk_queue __percpu *bulkq; struct rcu_head rcu; @@ -375,7 +376,12 @@ static void *dev_map_lookup_elem(struct bpf_map *map, void *key) struct bpf_dtab_netdev *obj = __dev_map_lookup_elem(map, *(u32 *)key); struct net_device *dev = obj ? obj->dev : NULL; - return dev ? &dev->ifindex : NULL; + if (dev) { + obj->ifindex_scratch = dev->ifindex; + return &obj->ifindex_scratch; + } + + return NULL; } static void dev_map_flush_old(struct bpf_dtab_netdev *dev) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 5c2cb5bd84ce..7128a9821481 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -2893,12 +2893,9 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env, if (func_id != BPF_FUNC_get_local_storage) goto error; break; - /* devmap returns a pointer to a live net_device ifindex that we cannot - * allow to be modified from bpf side. So do not allow lookup elements - * for now. - */ case BPF_MAP_TYPE_DEVMAP: - if (func_id != BPF_FUNC_redirect_map) + if (func_id != BPF_FUNC_redirect_map && + func_id != BPF_FUNC_map_lookup_elem) goto error; break; /* Restrict bpf side of cpumap and xskmap, open when use-cases
We don't currently allow lookups into a devmap from eBPF, because the map lookup returns a pointer directly to the dev->ifindex, which shouldn't be modifiable from eBPF. However, being able to do lookups in devmaps is useful to know (e.g.) whether forwarding to a specific interface is enabled. Currently, programs work around this by keeping a shadow map of another type which indicates whether a map index is valid. To allow lookups, simply copy the ifindex into a scratch variable and return a pointer to this. If an eBPF program does modify it, this doesn't matter since it will be overridden on the next lookup anyway. While this does add a write to every lookup, the overhead of this is negligible because the cache line is hot when both the write and the subsequent read happens. Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> --- kernel/bpf/devmap.c | 8 +++++++- kernel/bpf/verifier.c | 7 ++----- 2 files changed, 9 insertions(+), 6 deletions(-)