Re: [PATCH bpf-next] bpf: select proper size for bpf_prog_pack

On Mar 8, 2022, at 8:58 AM, Daniel Borkmann [off-list ref] wrote:

On 3/4/22 7:43 PM, Song Liu wrote:

quoted

Using HPAGE_PMD_SIZE as the size for bpf_prog_pack is not ideal in some
cases. Specifically, for NUMA systems, __vmalloc_node_range requires
PMD_SIZE * num_online_nodes() to allocate huge pages. Also, if the system
does not support huge pages (i.e., with cmdline option nohugevmalloc), it
is better to use PAGE_SIZE packs.
Add logic to select proper size for bpf_prog_pack. This solution is not
ideal, as it makes assumption about the behavior of module_alloc and
__vmalloc_node_range. However, it appears to be the easiest solution as
it doesn't require changes in module_alloc and vmalloc code.

nit: Fixes tag?

quoted

Signed-off-by: Song Liu <song@kernel.org>

[...]

quoted

 +static size_t bpf_prog_pack_size = -1;
+
+static inline int bpf_prog_chunk_count(void)
+{
+	WARN_ON_ONCE(bpf_prog_pack_size == -1);
+	return bpf_prog_pack_size / BPF_PROG_CHUNK_SIZE;
+}
+
 static DEFINE_MUTEX(pack_mutex);
 static LIST_HEAD(pack_list);
   static struct bpf_prog_pack *alloc_new_pack(void)
 {
 	struct bpf_prog_pack *pack;
+	size_t size;
+	void *ptr;
 -	pack = kzalloc(sizeof(*pack) + BITS_TO_BYTES(BPF_PROG_CHUNK_COUNT), GFP_KERNEL);
-	if (!pack)
+	if (bpf_prog_pack_size == -1) {
+		/* Test whether we can get huge pages. If not just use
+		 * PAGE_SIZE packs.
+		 */
+		size = PMD_SIZE * num_online_nodes();
+		ptr = module_alloc(size);
+		if (ptr && is_vm_area_hugepages(ptr)) {
+			bpf_prog_pack_size = size;
+			goto got_ptr;
+		} else {
+			bpf_prog_pack_size = PAGE_SIZE;
+			vfree(ptr);
+		}
+	}
+
+	ptr = module_alloc(bpf_prog_pack_size);
+	if (!ptr)
 		return NULL;
-	pack->ptr = module_alloc(BPF_PROG_PACK_SIZE);
-	if (!pack->ptr) {
-		kfree(pack);
+got_ptr:
+	pack = kzalloc(struct_size(pack, bitmap, BITS_TO_LONGS(bpf_prog_chunk_count())),
+		       GFP_KERNEL);
+	if (!pack) {
+		vfree(ptr);
 		return NULL;
 	}
-	bitmap_zero(pack->bitmap, BPF_PROG_PACK_SIZE / BPF_PROG_CHUNK_SIZE);
+	pack->ptr = ptr;
+	bitmap_zero(pack->bitmap, bpf_prog_pack_size / BPF_PROG_CHUNK_SIZE);
 	list_add_tail(&pack->list, &pack_list);
   	set_vm_flush_reset_perms(pack->ptr);
-	set_memory_ro((unsigned long)pack->ptr, BPF_PROG_PACK_SIZE / PAGE_SIZE);
-	set_memory_x((unsigned long)pack->ptr, BPF_PROG_PACK_SIZE / PAGE_SIZE);
+	set_memory_ro((unsigned long)pack->ptr, bpf_prog_pack_size / PAGE_SIZE);
+	set_memory_x((unsigned long)pack->ptr, bpf_prog_pack_size / PAGE_SIZE);
 	return pack;
 }
 @@ -864,7 +886,7 @@ static void *bpf_prog_pack_alloc(u32 size)
 	unsigned long pos;
 	void *ptr = NULL;
 -	if (size > BPF_PROG_MAX_PACK_PROG_SIZE) {
+	if (size > bpf_prog_pack_size) {
 		size = round_up(size, PAGE_SIZE);
 		ptr = module_alloc(size);
 		if (ptr) {

What happens if the /very first/ program requests an allocation size of >PAGE_SIZE? Wouldn't
this result in OOB write?

The 'size > bpf_prog_pack_size' is initially skipped due to -1 but then the module_alloc()
won't return a huge page, so we redo the allocation with bpf_prog_pack_size as PAGE_SIZE and
return a pointer into this pack?

Good catch! Let me see how to fix this.

Thanks,
Song