On Mon, Mar 20, 2023 at 7:19 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Fri, 2023-03-17 at 18:13 -0700, inwardvessel wrote:
> > From: JP Kobryn <inwardvessel@xxxxxxxxx>
> >
> > Within bpf programs, the bpf helper functions can make inline calls to
> > kernel functions. In this scenario there can be a disconnect between the
> > register the kernel function writes a return value to and the register the
> > bpf program uses to evaluate that return value.
> >
> > As an example, this bpf code:
> >
> > long err = bpf_map_update_elem(...);
> > if (err && err != -EEXIST)
> > // got some error other than -EEXIST
> >
> > ...can result in the bpf assembly:
> >
> > ; err = bpf_map_update_elem(&mymap, &key, &val, BPF_NOEXIST);
> > 37: movabs $0xffff976a10730400,%rdi
> > 41: mov $0x1,%ecx
> > 46: call 0xffffffffe103291c ; htab_map_update_elem
> > ; if (err && err != -EEXIST) {
> > 4b: cmp $0xffffffffffffffef,%rax ; cmp -EEXIST,%rax
> > 4f: je 0x000000000000008e
> > 51: test %rax,%rax
> > 54: je 0x000000000000008e
> >
> > The compare operation here evaluates %rax, while in the preceding call to
> > htab_map_update_elem the corresponding assembly returns -EEXIST via %eax:
> >
> > movl $0xffffffef, %r9d
> > ...
> > movl %r9d, %eax
> >
> > ...since it's returning int (32-bit). So the resulting comparison becomes:
> >
> > cmp $0xffffffffffffffef, $0x00000000ffffffef
> >
> > ...making it not possible to check for negative errors or specific errors,
> > since the sign value is left at the 32nd bit. It means in the original
> > example, the conditional branch will be entered even when the error is
> > -EEXIST, which was not intended.
> >
> > The selftests added cover these cases for the different bpf_map_ops
> > functions. When the second patch is applied, changing the return type of
> > those functions to long, the comparison works as intended and the tests
> > pass.
> >
>
> Looks like this fixes commit from 2020:
> bdb7b79b4ce8 ("bpf: Switch most helper return values from 32-bit int to 64-bit long")
>
> To add to the summary: the issue is caused by the fact that test
> program uses map function definitions from `bpf_helper_defs.h`, e.g.:
>
> static long (*bpf_map_update_elem)(...) 2;
>
> These definitions are generated from `include/uapi/linux/bpf.h`,
> which specifies the return type for this helper to be `long`
> (changed to from `int` in the commit mentioned above).
> That's why clang does not insert sign extension instructions when
> helper is called.
JP,
could you please add Ed's clarification to the commit log
and add 'Fixes: bdb7b79b4ce8 ...' tag and respin ?
>
> Interesting how this went under the radar for so long, probably
> because user code mostly uses `int` to catch return value of map
> functions.
>
> That commit changes return types for a lot of functions.
> I looked through function definitions and verifier.c code for those,
> but have not found any additional issues, except for two obvious:
> - bpf_redirect_map / ops->map_redirect
> - bpf_for_each_map_elem / ops->map_for_each_callback
Please fix these two as well in the same patch.
> Tested-By: Eduard Zingerman <eddyz87@xxxxxxxxx>
and please carry the Tested-by tag in the respin.
Thanks!
