On 21/11/2022 18:57, Zachary Leaf wrote:
  +static inline bpfptr_t make_bpfptr_fixed(__kernel_aligned_uintptr_t
ptr, bool is_kernel)
+{
+    if (is_kernel)
+        return KERNEL_BPFPTR((void *) (uintptr_t) ptr);

a cast to (void *) should be sufficient without going through uintptr_t
now.

That results in a CToPtr warning for me:

./include/linux/bpfptr.h:37:24: warning: the following conversion will
result in a CToPtr operation; the behaviour of CToPtr can be confusing
since using CToPtr on an untagged capability will give 0 instead of the
integer value and should therefore be explicitly annotated [-Wcher
i-pointer-conversion]

For PCuABI __kernel_aligned_uintptr_t is __uintcap_t (128b), void* is
64b and uintptr_t should be u64.

I guess the warning here is erroneous since in this code path, in theory
it should be the kernel calling this with a 64b kernel ptr, so ptr
should contain a 64b integer representation in __uintcap_t
(__kernel_uintptr_t), with nothing in the top bits.

Explicit cast to uintptr_t then tells the compiler there is no chance
this __uintcap_t contains a capability so no CToPtr warning. I'm not
sure exactly. Would be interested if anyone understands that differently.

The way I like to think about casts involving capabilities is that you should only do *one* of the two possible operations that a cast entails a time, that is either 1. representation change (== actual change in the bit pattern) or 2. interpretation change (== moving to a type of a different nature but without any change in the representation).

So if you want to go from A = uintcap_t to B = void *, you actually need two casts, because the representation is different (capability -> 64-bit), and the interpretation is also different (integer -> pointer). uintcap_t can be cast to any other integer type (representation change), and any 64-bit integer can be cast to a pointer (interpretation change), so the following chain works: uintcap_t -> uintptr_t -> void *.

FWIW this maps quite well to the breakdown of casts in C++ (and indeed, on a vanilla 64-bit ABI an int cannot be directly converted to a void * using C++ casts):
1. Representation change == static_cast
2. Interpretation change == reinterpret_cast

Coming back to make_bpfptr(), if the argument contains a kernel pointer stored in a __kernel_uintptr_t, then you indeed know that the top 64 bits are irrelevant and truncating the type is what you want to do. However as discussed offline we should get to the bottom of whether kernel pointers actually get stored in e.g. union bpf_attr, as this is rather unclear at the moment.

Final remark regarding conversions: the warning that Clang prints is significant and explains why the conversion would fail if you didn't do the two-step cast. In hybrid implicit conversions to/from capabilities have a special null-pointer handling: a null pointer always yields the null capability, and an untagged capability always yields the null pointer. Here the input isn't a capability at all but a 64-bit pointer represented as capability, so explicit representation change is required.

Kevin