On 26/09/2023 12:02, Zachary Leaf wrote:
[...]
+static int copy_bpf_attr_from_user(union bpf_attr *attr, int cmd,
bpfptr_t uattr, unsigned int *size)+{
- union compat_bpf_attr cattr;
- void *target_attr = in_compat64_syscall() ? (union bpf_attr *)&cattr
: attr;
I'm feeling that in all these copy_*_from functions (here and in patch 8), since we added the explicit compat zeroing, introducing this variable is not making things simpler, rather the opposite. It's used only once, and I feel it would be easier to follow what's happening by keeping two separate paths:
if (in_compat64_syscall() { memset(&cattr, ...); copy_from_bpfptr(&cattr, ...); convert_compat_bpf_attr(...); } else { copy_from_bpfptr(attr, ...); }
Another nice thing is that we only use in_compat64_syscall() twice instead of 4 times this way.
For the copy_*_to functions in patch 8, I think it remains readable, since there's no extra zeroing. However I wouldn't call the variable target_attr there, as it suggests it is a destination. Maybe source_attr instead.
Thinking some more after looking at patch 10: that suggestion removes the need for both copy_from_bpfptr_with_ptr and bpf_copy_from_user_with_ptr I believe. Maybe we're better off using the same approach for copy_*_to, up to you.
Kevin
- size_t attr_size = in_compat64_syscall() ? sizeof(union compat_bpf_attr)
: sizeof(union bpf_attr);- int err;
- err = bpf_check_uarg_tail_zero(uattr, attr_size, *size);
- if (err)
return err;- *size = min_t(u32, *size, attr_size);
- memset(attr, 0, sizeof(*attr));
- if (in_compat64_syscall())
memset(&cattr, 0, sizeof(cattr));- /* copy attributes from user space, may be less than sizeof(bpf_attr) */
- if (copy_from_bpfptr(target_attr, uattr, *size) != 0)
return -EFAULT;- if (in_compat64_syscall())
convert_compat_bpf_attr(attr, &cattr, cmd);- return 0;
+}
static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size) { union bpf_attr attr; @@ -5035,15 +5326,9 @@ static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size) (cmd == BPF_MAP_CREATE || cmd == BPF_PROG_LOAD)) return -EPERM;
- err = bpf_check_uarg_tail_zero(uattr, sizeof(attr), size);
- err = copy_bpf_attr_from_user(&attr, cmd, uattr, &size); if (err) return err;
- size = min_t(u32, size, sizeof(attr));
- /* copy attributes from user space, may be less than sizeof(bpf_attr) */
- memset(&attr, 0, sizeof(attr));
- if (copy_from_bpfptr(&attr, uattr, size) != 0)
return -EFAULT;err = security_bpf(cmd, &attr, size); if (err < 0)