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

[Martin KaFai Lau]

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