Re: [PATCH v6 bpf-next 06/12] bpf: Add uptr support in the map_value of the task local storage.

[Shakeel Butt <shakeel.butt@linux.dev>]

On Wed, Oct 23, 2024 at 04:47:53PM GMT, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <martin.lau@kernel.org>
> 
> This patch adds uptr support in the map_value of the task local storage.
> 
> struct map_value {
> 	struct user_data __uptr *uptr;
> };
> 
> struct {
> 	__uint(type, BPF_MAP_TYPE_TASK_STORAGE);
> 	__uint(map_flags, BPF_F_NO_PREALLOC);
> 	__type(key, int);
> 	__type(value, struct value_type);
> } datamap SEC(".maps");
> 
> A new bpf_obj_pin_uptrs() is added to pin the user page and
> also stores the kernel address back to the uptr for the
> bpf prog to use later. It currently does not support
> the uptr pointing to a user struct across two pages.
> It also excludes PageHighMem support to keep it simple.
> As of now, the 32bit bpf jit is missing other more crucial bpf
> features. For example, many important bpf features depend on
> bpf kfunc now but so far only one arch (x86-32) supports it
> which was added by me as an example when kfunc was first
> introduced to bpf.
> 
> The uptr can only be stored to the task local storage by the
> syscall update_elem. Meaning the uptr will not be considered
> if it is provided by the bpf prog through
> bpf_task_storage_get(BPF_LOCAL_STORAGE_GET_F_CREATE).
> This is enforced by only calling
> bpf_local_storage_update(swap_uptrs==true) in
> bpf_pid_task_storage_update_elem. Everywhere else will
> have swap_uptrs==false.
> 
> This will pump down to bpf_selem_alloc(swap_uptrs==true). It is
> the only case that bpf_selem_alloc() will take the uptr value when
> updating the newly allocated selem. bpf_obj_swap_uptrs() is added
> to swap the uptr between the SDATA(selem)->data and the user provided
> map_value in "void *value". bpf_obj_swap_uptrs() makes the
> SDATA(selem)->data takes the ownership of the uptr and the user space
> provided map_value will have NULL in the uptr.
> 
> The bpf_obj_unpin_uptrs() is called after map->ops->map_update_elem()
> returning error. If the map->ops->map_update_elem has reached
> a state that the local storage has taken the uptr ownership,
> the bpf_obj_unpin_uptrs() will be a no op because the uptr
> is NULL. A "__"bpf_obj_unpin_uptrs is added to make this
> error path unpin easier such that it does not have to check
> the map->record is NULL or not.
> 
> BPF_F_LOCK is not supported when the map_value has uptr.
> This can be revisited later if there is a use case. A similar
> swap_uptrs idea can be considered.
> 
> The final bit is to do unpin_user_page in the bpf_obj_free_fields().
> The earlier patch has ensured that the bpf_obj_free_fields() has
> gone through the rcu gp when needed.
> 
> Cc: linux-mm@kvack.org
> Cc: Shakeel Butt <shakeel.butt@linux.dev>
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>

Acked-by: Shakeel Butt <shakeel.butt@linux.dev>