Re: [PATCH bpf-next 2/3] libbpf: move global data mmap()'ing into bpf_object__load()

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Oct 22, 2024 at 09:39:07PM -0700, Andrii Nakryiko wrote:

SNIP

> @@ -5146,11 +5147,43 @@ bpf_object__populate_internal_map(struct bpf_object *obj, struct bpf_map *map)
>  		if (err) {
>  			err = -errno;
>  			cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
> -			pr_warn("Error freezing map(%s) as read-only: %s\n",
> -				map->name, cp);
> +			pr_warn("map '%s': failed to freeze as read-only: %s\n",
> +				bpf_map__name(map), cp);
>  			return err;
>  		}
>  	}
> +
> +	/* Remap anonymous mmap()-ed "map initialization image" as
> +	 * a BPF map-backed mmap()-ed memory, but preserving the same
> +	 * memory address. This will cause kernel to change process'
> +	 * page table to point to a different piece of kernel memory,
> +	 * but from userspace point of view memory address (and its
> +	 * contents, being identical at this point) will stay the
> +	 * same. This mapping will be released by bpf_object__close()
> +	 * as per normal clean up procedure.
> +	 */
> +	mmap_sz = bpf_map_mmap_sz(map);
> +	if (map->def.map_flags & BPF_F_MMAPABLE) {
> +		void *mmaped;
> +		int prot;
> +
> +		if (map->def.map_flags & BPF_F_RDONLY_PROG)
> +			prot = PROT_READ;
> +		else
> +			prot = PROT_READ | PROT_WRITE;
> +		mmaped = mmap(map->mmaped, mmap_sz, prot, MAP_SHARED | MAP_FIXED, map->fd, 0);
> +		if (mmaped == MAP_FAILED) {
> +			err = -errno;
> +			pr_warn("map '%s': failed to re-mmap() contents: %d\n",
> +				bpf_map__name(map), err);
> +			return err;
> +		}
> +		map->mmaped = mmaped;
> +	} else if (map->mmaped) {
> +		munmap(map->mmaped, mmap_sz);
> +		map->mmaped = NULL;
> +	}

this caught my eye because we did not do that in bpf_object__load_skeleton,
makes sense, but why do we mmap *!*BPF_F_MMAPABLE maps in the first place?

jirka

> +
>  	return 0;
>  }
>  
> @@ -5467,8 +5500,7 @@ bpf_object__create_maps(struct bpf_object *obj)
>  				err = bpf_object__populate_internal_map(obj, map);
>  				if (err < 0)
>  					goto err_out;
> -			}
> -			if (map->def.type == BPF_MAP_TYPE_ARENA) {
> +			} else if (map->def.type == BPF_MAP_TYPE_ARENA) {
>  				map->mmaped = mmap((void *)(long)map->map_extra,
>  						   bpf_map_mmap_sz(map), PROT_READ | PROT_WRITE,
>  						   map->map_extra ? MAP_SHARED | MAP_FIXED : MAP_SHARED,
> @@ -13916,46 +13948,11 @@ int bpf_object__load_skeleton(struct bpf_object_skeleton *s)
>  	for (i = 0; i < s->map_cnt; i++) {
>  		struct bpf_map_skeleton *map_skel = (void *)s->maps + i * s->map_skel_sz;
>  		struct bpf_map *map = *map_skel->map;
> -		size_t mmap_sz = bpf_map_mmap_sz(map);
> -		int prot, map_fd = map->fd;
> -		void **mmaped = map_skel->mmaped;
> -
> -		if (!mmaped)
> -			continue;
> -
> -		if (!(map->def.map_flags & BPF_F_MMAPABLE)) {
> -			*mmaped = NULL;
> -			continue;
> -		}
>  
> -		if (map->def.type == BPF_MAP_TYPE_ARENA) {
> -			*mmaped = map->mmaped;
> +		if (!map_skel->mmaped)
>  			continue;
> -		}
> -
> -		if (map->def.map_flags & BPF_F_RDONLY_PROG)
> -			prot = PROT_READ;
> -		else
> -			prot = PROT_READ | PROT_WRITE;
>  
> -		/* Remap anonymous mmap()-ed "map initialization image" as
> -		 * a BPF map-backed mmap()-ed memory, but preserving the same
> -		 * memory address. This will cause kernel to change process'
> -		 * page table to point to a different piece of kernel memory,
> -		 * but from userspace point of view memory address (and its
> -		 * contents, being identical at this point) will stay the
> -		 * same. This mapping will be released by bpf_object__close()
> -		 * as per normal clean up procedure, so we don't need to worry
> -		 * about it from skeleton's clean up perspective.
> -		 */
> -		*mmaped = mmap(map->mmaped, mmap_sz, prot, MAP_SHARED | MAP_FIXED, map_fd, 0);
> -		if (*mmaped == MAP_FAILED) {
> -			err = -errno;
> -			*mmaped = NULL;
> -			pr_warn("failed to re-mmap() map '%s': %d\n",
> -				 bpf_map__name(map), err);
> -			return libbpf_err(err);
> -		}
> +		*map_skel->mmaped = map->mmaped;
>  	}
>  
>  	return 0;
> -- 
> 2.43.5
> 
>