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

On 3/15/24 11:11 AM, Kui-Feng Lee wrote:

quoted

--- 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 ].