[bpf,v5,1/2] bpf: Resolve fext program type when checking map compatibility

Commit Message

Toke Høiland-Jørgensen Dec. 14, 2022, 11:02 p.m. UTC

The bpf_prog_map_compatible() check makes sure that BPF program types are
not mixed inside BPF map types that can contain programs (tail call maps,
cpumaps and devmaps). It does this by setting the fields of the map->owner
struct to the values of the first program being checked against, and
rejecting any subsequent programs if the values don't match.

One of the values being set in the map owner struct is the program type,
and since the code did not resolve the prog type for fext programs, the map
owner type would be set to PROG_TYPE_EXT and subsequent loading of programs
of the target type into the map would fail.

This bug is seen in particular for XDP programs that are loaded as
PROG_TYPE_EXT using libxdp; these cannot insert programs into devmaps and
cpumaps because the check fails as described above.

Fix the bug by resolving the fext program type to its target program type
as elsewhere in the verifier. This requires constifying the parameter of
resolve_prog_type() to avoid a compiler warning from the new call site.

v3:
- Add Yonghong's ACK

Fixes: f45d5b6ce2e8 ("bpf: generalise tail call map compatibility check")
Acked-by: Yonghong Song <[e-mail]>
Signed-off-by: Toke Høiland-Jørgensen <[e-mail]>
---
 kernel/bpf/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

[e-mail] Dec. 15, 2022, 5:40 a.m. UTC | #1

Hello:

This series was applied to bpf/bpf.git (master)
by Martin KaFai Lau <[e-mail]>:

On Thu, 15 Dec 2022 00:02:53 +0100 you wrote:
> The bpf_prog_map_compatible() check makes sure that BPF program types are
> not mixed inside BPF map types that can contain programs (tail call maps,
> cpumaps and devmaps). It does this by setting the fields of the map->owner
> struct to the values of the first program being checked against, and
> rejecting any subsequent programs if the values don't match.
> 
> One of the values being set in the map owner struct is the program type,
> and since the code did not resolve the prog type for fext programs, the map
> owner type would be set to PROG_TYPE_EXT and subsequent loading of programs
> of the target type into the map would fail.
> 
> [...]

Here is the summary with links:
  - [bpf,v5,1/2] bpf: Resolve fext program type when checking map compatibility
    https://git.kernel.org/bpf/bpf/c/1c123c567fb1
  - [bpf,v5,2/2] selftests/bpf: Add a test for using a cpumap from an freplace-to-XDP program
    https://git.kernel.org/bpf/bpf/c/f506439ec3de

You are awesome, thank you!

Martin KaFai Lau Dec. 15, 2022, 5:41 a.m. UTC | #2

On 12/14/22 3:02 PM, Toke Høiland-Jørgensen wrote:
> This requires constifying the parameter of
> resolve_prog_type() to avoid a compiler warning from the new call site.

Applied with this part removed from the commit message.  This change is not in 
this patch.  The const had already been added a while back.

Toke Høiland-Jørgensen Dec. 15, 2022, 11:03 a.m. UTC | #3

Martin KaFai Lau <[e-mail]> writes:

> On 12/14/22 3:02 PM, Toke Høiland-Jørgensen wrote:
>> This requires constifying the parameter of
>> resolve_prog_type() to avoid a compiler warning from the new call site.
>
> Applied with this part removed from the commit message.  This change is not in 
> this patch.  The const had already been added a while back.

Ah, right; it was in an earlier version (because I was making the change
in the bpf tree, not bpf-next), and I didn't notice it disappeared when
I rebased. Thanks for fixing! :)

-Toke

Patch

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 7f98dec6e90f..b334f4ddc4d5 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2092,6 +2092,7 @@  static unsigned int __bpf_prog_ret0_warn(const void *ctx,
 bool bpf_prog_map_compatible(struct bpf_map *map,
 			     const struct bpf_prog *fp)
 {
+	enum bpf_prog_type prog_type = resolve_prog_type(fp);
 	bool ret;
 
 	if (fp->kprobe_override)
@@ -2102,12 +2103,12 @@  bool bpf_prog_map_compatible(struct bpf_map *map,
 		/* There's no owner yet where we could check for
 		 * compatibility.
 		 */
-		map->owner.type  = fp->type;
+		map->owner.type  = prog_type;
 		map->owner.jited = fp->jited;
 		map->owner.xdp_has_frags = fp->aux->xdp_has_frags;
 		ret = true;
 	} else {
-		ret = map->owner.type  == fp->type &&
+		ret = map->owner.type  == prog_type &&
 		      map->owner.jited == fp->jited &&
 		      map->owner.xdp_has_frags == fp->aux->xdp_has_frags;
 	}