On 13/07/2023 09:17, Zachary Leaf wrote:
On 12/07/2023 12:19, Kevin Brodsky wrote:
On 08/06/2023 10:42, Zachary Leaf wrote:
There are two ways that the kernel writes back to __user *bpf_attr unions and each requires different compat64 handling:
- Where the union member to be written to is a __user *ptr e.g. a pointer to some memory buffer
- Writing to a normal non-ptr field e.g. __u32 prog_cnt
In the first instance, no additional conversion out or handling is required. When converting inbound each __user *ptr is simply copied from union compat_bpf_attr into a new union bpf_attr and this can be written to directly with copy_to_user().
It took me a while but I think eventually I managed to get what you're trying to say: in that case, because there is a pointer in the struct, what we're getting from userspace is a compat layout, which we have to convert into a native struct and then we operate on that.
What you say is correct, but it's not what I was getting at.
Assume you have converted some compat struct, containing both __user ptrs and normal members, into a struct with native offsets into the kernel. You then have two types of members in the native struct after that - __user pointers, for example, the location of some memory buffer in userspace and then normal non-ptr members.
When you write out to the __user pointer you don't need to interact with or worry about the struct offsets anymore as you have a direct pointer to some user memory somewhere. You just do copy_to_user() on that __user pointer and start writing directly to the memory buffer wherever it is in userspace memory. The compat and native __user pointer is the same - it's just an address and writing to that address doesn't matter what offsets we got that address from.
If you're trying to write back to some specific field in the struct e.g. uattr->some_int in the struct, you now need to interact with or ensure you're writing at the correct offsets and do all this casting magic.
Assuming I got this right, this prompts another question: in the second case, how is the layout different at all between compat and native? I can't really grasp these two situations as described. My initial expectation would have been more or less the opposite: if you have pointers, then the layout is different, and when writing out you will have to do something: either write to a (temporary) native struct and then convert before writing to userspace, or write directly at the right offset with the sort of macro magic you've got in this patch. I don't immediately see how the field you're writing to makes a difference, rather the fields *before* do (as they change the offset). I'm probably missing at least a couple of things though!
I think you're seeing the distinction here as structs with pointers and structs without pointers, but really it's about how you can can write back out to some memory and where it is.
With non-ptr members, the memory you're writing to is inside the struct, so you need to ensure offsets are correct to write to correct location.
Sure, I was just trying to make sense of my wrong interpretation so that got me on a tangent :) Clearly you do want to have this sort of macro to write to struct members regardless of where they are in the struct (i.e. even if they have the same offset in native and compat), to make things reasonably robust and consistent.
With user ptr members, the memory you're writing to is outside the struct, so you can just write directly to it, you don't need to worry about struct offsets anymore.
Hence writing out to __user pointer members doesn't require any conversion out. For everything else you need to do this magic. I'll try and rework the commit message to hopefully clear this up.
Right, that was the critical bit I was missing: for pointer members, it is always *the pointer target* that is written to. In that case clearly the struct layout is irrelevant, as you clarified above. In hindsight it is rather unlikely for this syscall that the *pointer itself* would be written to (where would that new user pointer come from?), but it is still worth making the language more precise in the commit message. "Writing to a pointer" can be interpreted both as writing to the pointer itself, or writing to its target.
Thinking further, there is a more fundamental reason why I found that wording confusing: if you are writing to the pointer target, you are not writing back to the user-provided union bpf_attr (uattr) at all. Rather you are extracting the user pointer from the copied-in (and potentially converted) union bpf_attr (attr), and then writing through that. That's why I didn't think about that situation, as it is fundamentally different. To make the commit message more intelligible, I would rather make a note about this aspect at the end of it, and start with what the patch is actually addressing.
Kevin