On 03/10/2023 14:52, Kevin Brodsky wrote:
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.
Yeah we can do it this way - it doesn't really seem much easier to follow for me since everything gets pretty bunched up and duplicated. Later on we need to add check_attr() here as well:
if (in_compat64_syscall()) { memset(&cattr, 0, sizeof(cattr)); if (copy_from_bpfptr((union bpf_attr *)&cattr, uattr, *size) != 0) return -EFAULT; err = check_attr(cmd, (union bpf_attr *)&cattr); if (err) return -EINVAL; convert_compat_bpf_attr(attr, &cattr, cmd); } else { if (copy_from_bpfptr(attr, uattr, *size) != 0) return -EFAULT; err = check_attr(cmd, attr); if (err) return -EINVAL; }
Maybe that changes your mind about it.
For the copy_*_to functions in patch 8, I think it remains readable, since there's no extra zeroing.
Isn't there? We still do memset(0) on the info structs?
However I wouldn't call the variable target_attr there, as it suggests it is a destination. Maybe source_attr instead.
Make sense. I'll probably go for src_attr and the copy_*_from might be better as dst_info as well (rather than target).
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.
I think this way works better than above since we avoid bpf_copy_from_user_with_ptr() and we don't have the added check_attr() duplication in there too.
I probably wouldn't be against leaving copy_bpf_attr_from_user() as is, and only amending the ones from patch 8.
Thanks, Zach
Kevin