Re: [PATCH bpf-next v3 2/2] bpf: Check return from set_memory_rox()

From: Martin KaFai Lau
Date: Fri Mar 15 2024 - 14:34:33 EST


On 3/15/24 11:11 AM, Kui-Feng Lee wrote:
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
          if (err)
              goto reset_unlock;
      }
-    for (i = 0; i < st_map->image_pages_cnt; i++)
-        arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
+    for (i = 0; i < st_map->image_pages_cnt && !err; i++)
+        err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE);
+
+    if (err)

nit: Can it be more specific? I mean to check err < 0, so we can reason
that this function never returns a positive value other than 0.

I think "if (err)" is fine. It is pretty common in other places of the kernel.

Checking "(err < 0)" may actually mean the return value could be positive. At least it is how bpf_struct_ops.c is using "(err < 0)".

[ An unrelated side note is another (err < 0) check in bpf_struct_ops.c could have been changed after the recent changes in bpf_struct_ops_prepare_trampoline which no longer return +val ].