On 18/08/2023 17:15, Zachary Leaf wrote:
- case BPF_PROG_BIND_MAP:
dest->prog_bind_map.prog_fd = cattr->prog_bind_map.prog_fd;dest->prog_bind_map.map_fd = cattr->prog_bind_map.map_fd;dest->prog_bind_map.flags = cattr->prog_bind_map.flags;break;- };
Considering the really large number of assignments, I think it would be worth introducing a macro that takes dest, cattr and the name of the member to copy. That would shorten things a bit for nested structs, but most importantly it would prevent typos slipping in.
That's a good idea.
I've added a generic copy_field() macro in include/linux/stddef.h:
#define copy_field(DEST, SRC, FIELD) \ ((DEST)->FIELD = (SRC)->FIELD)
I wasn't thinking about adding it to a generic header but considering that it is indeed a completely generic macro, I don't see why not! stddef.h looks like a reasonable place to add it. Usual comment: macro arguments should not be capitalised.
A similar macro for pointer conversion could be introduced in the last patch (which would also hide the very noisy cast to __kernel_aligned_uintptr_t).
This is a bit more tricky to name and figure out where it should live. So far I have compat_uptr_to_kern() in arch/arm64/include/asm/compat.h next to compat_ptr() define just for lack of a better place. It's kind of generic (not bpf related) but also pretty specific at the same time, hence hard to name without the name being really long. Maybe we just make it bpf specific and limit it to bpf/syscall.c + bpf/btf.c where it's used.
I think we should indeed do that, it would have its place in bpf_compat.h. You are right that it is somewhat generic as we need to do this everywhere we introduced compat64 handling (e.g. io_uring), but the number of conversions is sufficiently low there that the most readable is probably to leave them as-is.
One comment on the naming, "to_kern" doesn't make much sense - it suggests converting to a kernel pointer, which is not what happens. __kernel_aligned_uintptr_t is thus named because it is a uapi type, like __kernel_long_t for instance - that is "kernel" means a kernel type here, nothing else. Maybe bpf_uattr_compat_ptr(), combining compat_ptr() and bpf_put_uattr()?
#define compat_uptr_to_kern(DEST, SRC, FIELD) \ ((DEST)->FIELD = (__kernel_aligned_uintptr_t)compat_ptr((SRC)->FIELD))
[...]
+COMPAT_SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, size) +{
- return __sys_compat_bpf(cmd, USER_BPFPTR(uattr), size);
+}
Adding a compat handler is the "normal" way to handle this situation, so the approach in this patch is sound. However, it seems preferable to keep a native handler only and use in_compat_syscall() in __sys_bpf() for a couple of reasons:
- AFAICT the diff in this file would be a lot smaller - just a few lines
changed in __sys_bpf(), and of course the new conversion function.
It's a bit more compact - but there's some extra handling to make sure we're selecting the right size and union for everything:
The principle looks good to me, I think the diff should be quite clear too. Some comments inline.
inline bool in_compat64_syscall() { // generic - can go in arch/arm64/include/asm/compat.h
Why not, this could be used in a few other places too. That said if we do this we should make it consistent with [1], in fact Kristina mentioned the idea of adding 64-bit helpers in the commit message.
[1] https://git.morello-project.org/morello/kernel/linux/-/commit/fd6902f3c5266
return IS_ENABLED(CONFIG_COMPAT64) && in_compat_syscall(); }
#define select_attr (in_compat64_syscall() ? &cattr : attr) #define attr_size (in_compat64_syscall() ? sizeof(union compat_bpf_attr) : \ sizeof(union bpf_attr))
I don't think these macros are necessary (also note that macros that make function calls should themselves be function macros to make it clear they are not constants). We could have a local variable for each instead.
#define bpfptr_copy (in_compat64_syscall() ? copy_from_bpfptr : \ copy_from_bpfptr_with_ptr) static int copy_bpf_attr_from_user(union bpf_attr *attr, int cmd, bpfptr_t uattr, unsigned int *size) { union compat_bpf_attr cattr; int err;
err = bpf_check_uarg_tail_zero(uattr, attr_size, *size); if (err) return err; *size = min_t(u32, *size, attr_size);
/* copy attributes from user space, may be less than sizeof(bpf_attr) */ memset(select_attr, 0, attr_size); if (bpfptr_copy(select_attr, uattr, *size) != 0) return -EFAULT;
err = check_attr(cmd, select_attr); if (err) return -EINVAL;
if(in_compat64_syscall()) convert_compat_bpf_attr(attr, &cattr, cmd);
return 0; } #undef select_attr #undef attr_size #undef bpfptr_copy
static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size) { union bpf_attr attr; int err;
err = bpf_check_perms(cmd); if (err) return err;
err = copy_bpf_attr_from_user(&attr, cmd, uattr, &size); if (err) return err;
return dispatch_bpf(cmd, &attr, uattr, size); }
- It would avoid overriding the handler in sys_compat64.c, which is not
ideal as it makes it hard to figure out what handler is actually used. The situation would be different if we were adding a "generic" compat handler, as in that case we could just change include/uapi/asm-generic/unistd.h (the normal way to do this).
I don't follow that - it adds a handler in sys_compat64.c, and the handler in bpf/syscall.c is clearly guarded by #ifdef CONFIG_COMPAT64. We can even add a comment stating the compat handler is for compat64 only.
Sure, we could do that. My point is that ideally we shouldn't use this mechanism as it's a hack - it makes it hard to tell how a syscall is handled in compat64, unless you already know where to look (unlike the regular mechanism, where include/uapi/asm-generic/unistd.h makes it clear).
With that approach, a further small improvement can be made. On the same model as Tudor's "io_uring: Implement compat versions of uAPI structs and handle them", __sys_bpf() could call a generic function, say copy_bpf_attr_from_user(). This function would then call the conversion function in compat64. Besides a better encapsulation, the main advantage is that the union compat_bpf_attr variable is allocated in the conversion function, and it is therefore live only for the duration of the conversion. Conversely, allocating it in __sys_compat_bpf() means it lives for the whole duration of the syscall handling. In other words, it slightly improves the stack usage. Not essential but nice to have. I think that approach could also be used in patch 5-8.
Makes sense. That should be possible with the __sys_compat_bpf() handler too, just call out into another small helper function.
Finally to reduce the amount of #ifdef, we could have a macro like io_in_compat64() that only returns true in compat64. This allows keeping most of the code out of any #ifdef, assuming struct definitions and such are not #ifdef'd either (which is not particularly useful anyway).
I had preferred the other option just because everything compat64 is behind an #ifdef; so in the CONFIG_CHERI_PURECAP_UABI=n case it's as if compat64 doesn't even exist. We don't allocate, define or otherwise do anything compat64 related when CONFIG_COMPAT64 is disabled. So what's the problem with #ifdef? Maybe that is not that useful since I doubt anyone is using this kernel without PCuABI. Just seems a bit "cleaner" and straight forward to me, at the cost of some minor duplication.
There's no real point in #ifdef'ing declarations, as they have no impact on codegen. However #ifdef's have a negative impact on readability, and they should especially be avoided in the middle of a big function. Since we can use some macro magic to replace the explicit #ifdef's with no difference in codegen, we could make patch 5-8 more readable without much downside.
Either way should work really so I don't know if you have strong feelings one way or the other after seeing the above. I'm leaning towards __sys_compat_bpf() as before but I might have mis-understood some stuff or not implemented it correctly.
Hopefully I've clarified a few aspects! No strong preference either but it does feel like just one handler is more straightforward.
Kevin