[PATCH bpf-next] bpf: introduce bpf_map_get_xdp_prog utility routine

[PATCH bpf-next] bpf: introduce bpf_map_get_xdp_prog utility routine

[Lorenzo Bianconi]

Introduce bpf_map_get_xdp_prog to load an eBPF program on
CPUMAP/DEVMAP entries since both of them share the same code.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 include/linux/bpf.h |  2 ++
 kernel/bpf/core.c   | 17 +++++++++++++++++
 kernel/bpf/cpumap.c | 12 ++++--------
 kernel/bpf/devmap.c | 16 ++++++----------
 4 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 26bf8c865103..891936b54b55 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1910,6 +1910,8 @@ static inline struct bpf_prog *bpf_prog_get_type(u32 ufd,
 	return bpf_prog_get_type_dev(ufd, type, false);
 }
 
+struct bpf_prog *bpf_map_get_xdp_prog(struct bpf_map *map, int fd,
+				      enum bpf_attach_type attach_type);
 void __bpf_free_used_maps(struct bpf_prog_aux *aux,
 			  struct bpf_map **used_maps, u32 len);
 
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index dee91a2eea7b..7e72c21b6589 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2228,6 +2228,23 @@ void __bpf_free_used_maps(struct bpf_prog_aux *aux,
 	}
 }
 
+struct bpf_prog *bpf_map_get_xdp_prog(struct bpf_map *map, int fd,
+				      enum bpf_attach_type attach_type)
+{
+	struct bpf_prog *prog;
+
+	prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_XDP);
+	if (IS_ERR(prog))
+		return prog;
+
+	if (prog->expected_attach_type != attach_type) {
+		bpf_prog_put(prog);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return prog;
+}
+
 static void bpf_free_used_maps(struct bpf_prog_aux *aux)
 {
 	__bpf_free_used_maps(aux, aux->used_maps, aux->used_map_cnt);
diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
index 585b2b77ccc4..0b3e561e0c2a 100644
--- a/kernel/bpf/cpumap.c
+++ b/kernel/bpf/cpumap.c
@@ -397,19 +397,15 @@ static int cpu_map_kthread_run(void *data)
 	return 0;
 }
 
-static int __cpu_map_load_bpf_program(struct bpf_cpu_map_entry *rcpu, int fd)
+static int __cpu_map_load_bpf_program(struct bpf_cpu_map_entry *rcpu,
+				      struct bpf_map *map, int fd)
 {
 	struct bpf_prog *prog;
 
-	prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_XDP);
+	prog = bpf_map_get_xdp_prog(map, fd, BPF_XDP_CPUMAP);
 	if (IS_ERR(prog))
 		return PTR_ERR(prog);
 
-	if (prog->expected_attach_type != BPF_XDP_CPUMAP) {
-		bpf_prog_put(prog);
-		return -EINVAL;
-	}
-
 	rcpu->value.bpf_prog.id = prog->aux->id;
 	rcpu->prog = prog;
 
@@ -457,7 +453,7 @@ __cpu_map_entry_alloc(struct bpf_map *map, struct bpf_cpumap_val *value,
 	rcpu->map_id = map->id;
 	rcpu->value.qsize  = value->qsize;
 
-	if (fd > 0 && __cpu_map_load_bpf_program(rcpu, fd))
+	if (fd > 0 && __cpu_map_load_bpf_program(rcpu, map, fd))
 		goto free_ptr_ring;
 
 	/* Setup kthread */
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index f02d04540c0c..59df0745f72d 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -864,12 +864,12 @@ static struct bpf_dtab_netdev *__dev_map_alloc_node(struct net *net,
 		goto err_out;
 
 	if (val->bpf_prog.fd > 0) {
-		prog = bpf_prog_get_type_dev(val->bpf_prog.fd,
-					     BPF_PROG_TYPE_XDP, false);
-		if (IS_ERR(prog))
-			goto err_put_dev;
-		if (prog->expected_attach_type != BPF_XDP_DEVMAP)
-			goto err_put_prog;
+		prog = bpf_map_get_xdp_prog(&dtab->map, val->bpf_prog.fd,
+					    BPF_XDP_DEVMAP);
+		if (IS_ERR(prog)) {
+			dev_put(dev->dev);
+			goto err_out;
+		}
 	}
 
 	dev->idx = idx;
@@ -884,10 +884,6 @@ static struct bpf_dtab_netdev *__dev_map_alloc_node(struct net *net,
 	dev->val.ifindex = val->ifindex;
 
 	return dev;
-err_put_prog:
-	bpf_prog_put(prog);
-err_put_dev:
-	dev_put(dev->dev);
 err_out:
 	kfree(dev);
 	return ERR_PTR(-EINVAL);
-- 
2.31.1

Re: [PATCH bpf-next] bpf: introduce bpf_map_get_xdp_prog utility routine

[Alexei Starovoitov]

On Mon, Oct 25, 2021 at 2:18 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> Introduce bpf_map_get_xdp_prog to load an eBPF program on
> CPUMAP/DEVMAP entries since both of them share the same code.
>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  include/linux/bpf.h |  2 ++
>  kernel/bpf/core.c   | 17 +++++++++++++++++
>  kernel/bpf/cpumap.c | 12 ++++--------
>  kernel/bpf/devmap.c | 16 ++++++----------
>  4 files changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 26bf8c865103..891936b54b55 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1910,6 +1910,8 @@ static inline struct bpf_prog *bpf_prog_get_type(u32 ufd,
>         return bpf_prog_get_type_dev(ufd, type, false);
>  }
>
> +struct bpf_prog *bpf_map_get_xdp_prog(struct bpf_map *map, int fd,
> +                                     enum bpf_attach_type attach_type);
>  void __bpf_free_used_maps(struct bpf_prog_aux *aux,
>                           struct bpf_map **used_maps, u32 len);
>
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index dee91a2eea7b..7e72c21b6589 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -2228,6 +2228,23 @@ void __bpf_free_used_maps(struct bpf_prog_aux *aux,
>         }
>  }
>
> +struct bpf_prog *bpf_map_get_xdp_prog(struct bpf_map *map, int fd,
> +                                     enum bpf_attach_type attach_type)
> +{
> +       struct bpf_prog *prog;
> +
> +       prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_XDP);
> +       if (IS_ERR(prog))
> +               return prog;
> +
> +       if (prog->expected_attach_type != attach_type) {
> +               bpf_prog_put(prog);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       return prog;
> +}

It is supposed to be a cleanup... but...

1. it's tweaking __cpu_map_load_bpf_program()
to pass extra 'map' argument further into this helper,
but the 'map' is unused.

2. bpf_map_get_xdp_prog is a confusing name. what 'map' doing in there?

3. it's placed in core.c while it's really cpumap/devmap only.

I suggest leaving the code as-is.

Re: [PATCH bpf-next] bpf: introduce bpf_map_get_xdp_prog utility routine

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2959 bytes --]

> On Mon, Oct 25, 2021 at 2:18 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >
> > Introduce bpf_map_get_xdp_prog to load an eBPF program on
> > CPUMAP/DEVMAP entries since both of them share the same code.
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  include/linux/bpf.h |  2 ++
> >  kernel/bpf/core.c   | 17 +++++++++++++++++
> >  kernel/bpf/cpumap.c | 12 ++++--------
> >  kernel/bpf/devmap.c | 16 ++++++----------
> >  4 files changed, 29 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 26bf8c865103..891936b54b55 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1910,6 +1910,8 @@ static inline struct bpf_prog *bpf_prog_get_type(u32 ufd,
> >         return bpf_prog_get_type_dev(ufd, type, false);
> >  }
> >
> > +struct bpf_prog *bpf_map_get_xdp_prog(struct bpf_map *map, int fd,
> > +                                     enum bpf_attach_type attach_type);
> >  void __bpf_free_used_maps(struct bpf_prog_aux *aux,
> >                           struct bpf_map **used_maps, u32 len);
> >
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index dee91a2eea7b..7e72c21b6589 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -2228,6 +2228,23 @@ void __bpf_free_used_maps(struct bpf_prog_aux *aux,
> >         }
> >  }
> >
> > +struct bpf_prog *bpf_map_get_xdp_prog(struct bpf_map *map, int fd,
> > +                                     enum bpf_attach_type attach_type)
> > +{
> > +       struct bpf_prog *prog;
> > +
> > +       prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_XDP);
> > +       if (IS_ERR(prog))
> > +               return prog;
> > +
> > +       if (prog->expected_attach_type != attach_type) {
> > +               bpf_prog_put(prog);
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       return prog;
> > +}
> 
> It is supposed to be a cleanup... but...
> 
> 1. it's tweaking __cpu_map_load_bpf_program()
> to pass extra 'map' argument further into this helper,
> but the 'map' is unused.

For xdp multi-buff we will need to extend Toke's bpf_prog_map_compatible fix
running bpf_prog_map_compatible routine for cpumaps and devmaps in
order to avoid mixing xdp mb and xdp legacy programs in a cpumaps or devmaps.
For this reason I guess we will need to pass map pointer to
__cpu_map_load_bpf_program anyway.
I do not have a strong opinion on it, but the main idea here is just to have a
common code and avoid adding the same changes to cpumap and devmap.
Anyway if you prefer to do it separately for cpumap  and devmap I am fine
with it.

> 
> 2. bpf_map_get_xdp_prog is a confusing name. what 'map' doing in there?

maybe bpf_xdp_map_load_prog? (naming is always hard :))

Regards,
Lorenzo

> 
> 3. it's placed in core.c while it's really cpumap/devmap only.
> 
> I suggest leaving the code as-is.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH bpf-next] bpf: introduce bpf_map_get_xdp_prog utility routine

[Alexei Starovoitov]

On Tue, Nov 2, 2021 at 5:14 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >
> > 1. it's tweaking __cpu_map_load_bpf_program()
> > to pass extra 'map' argument further into this helper,
> > but the 'map' is unused.
>
> For xdp multi-buff we will need to extend Toke's bpf_prog_map_compatible fix
> running bpf_prog_map_compatible routine for cpumaps and devmaps in
> order to avoid mixing xdp mb and xdp legacy programs in a cpumaps or devmaps.
> For this reason I guess we will need to pass map pointer to
> __cpu_map_load_bpf_program anyway.
> I do not have a strong opinion on it, but the main idea here is just to have a
> common code and avoid adding the same changes to cpumap and devmap.
> Anyway if you prefer to do it separately for cpumap  and devmap I am fine
> with it.

None of that information was in the original commit log.
Please make sure to provide such details in the future and make it
part of the series.
That patch alone is unnecessary.

Re: [PATCH bpf-next] bpf: introduce bpf_map_get_xdp_prog utility routine

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

> On Tue, Nov 2, 2021 at 5:14 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > >
> > > 1. it's tweaking __cpu_map_load_bpf_program()
> > > to pass extra 'map' argument further into this helper,
> > > but the 'map' is unused.
> >
> > For xdp multi-buff we will need to extend Toke's bpf_prog_map_compatible fix
> > running bpf_prog_map_compatible routine for cpumaps and devmaps in
> > order to avoid mixing xdp mb and xdp legacy programs in a cpumaps or devmaps.
> > For this reason I guess we will need to pass map pointer to
> > __cpu_map_load_bpf_program anyway.
> > I do not have a strong opinion on it, but the main idea here is just to have a
> > common code and avoid adding the same changes to cpumap and devmap.
> > Anyway if you prefer to do it separately for cpumap  and devmap I am fine
> > with it.
> 
> None of that information was in the original commit log.
> Please make sure to provide such details in the future and make it
> part of the series.
> That patch alone is unnecessary.

Yes, right. Sorry for the noise.
Regarding this patch, do you want me to repost with a proper commit log (maybe
included in the xdp multi-buff series) or do you prefer to just drop it?

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH bpf-next] bpf: introduce bpf_map_get_xdp_prog utility routine

[Alexei Starovoitov]

On Wed, Nov 3, 2021 at 9:44 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > That patch alone is unnecessary.
>
> Yes, right. Sorry for the noise.
> Regarding this patch, do you want me to repost with a proper commit log (maybe
> included in the xdp multi-buff series) or do you prefer to just drop it?

I think this patch alone is not necessary.
When you'll get to the full series it could be meaningful.
It's hard to tell without seeing the rest of the patches.

Re: [PATCH bpf-next] bpf: introduce bpf_map_get_xdp_prog utility routine

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

> On Wed, Nov 3, 2021 at 9:44 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > > That patch alone is unnecessary.
> >
> > Yes, right. Sorry for the noise.
> > Regarding this patch, do you want me to repost with a proper commit log (maybe
> > included in the xdp multi-buff series) or do you prefer to just drop it?
> 
> I think this patch alone is not necessary.
> When you'll get to the full series it could be meaningful.
> It's hard to tell without seeing the rest of the patches.

ack, fine to me. I will drop it for the moment and then we will re-evaluate.
Thanks.

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]