diff mbox series

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

Message ID 20241015005008.767267-7-martin.lau@linux.dev (mailing list archive)
State New
Headers show
Series None | expand

Commit Message

Martin KaFai Lau Oct. 15, 2024, 12:49 a.m. UTC 
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.

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
Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
---
 include/linux/bpf.h            | 20 +++++++
 kernel/bpf/bpf_local_storage.c |  7 ++-
 kernel/bpf/bpf_task_storage.c  |  5 +-
 kernel/bpf/syscall.c           | 99 ++++++++++++++++++++++++++++++++--
 4 files changed, 124 insertions(+), 7 deletions(-)

Comments

Shakeel Butt Oct. 22, 2024, 11:07 p.m. UTC | #1 
On Mon, Oct 14, 2024 at 05:49:56PM GMT, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <martin.lau@kernel.org>
> 
[...]
> +static void unpin_uptr_kaddr(void *kaddr)
> +{
> +	if (kaddr)
> +		unpin_user_page(virt_to_page(kaddr));
> +}
> +
> +static void __bpf_obj_unpin_uptrs(struct btf_record *rec, u32 cnt, void *obj)
> +{
> +	const struct btf_field *field;
> +	void **uptr_addr;
> +	int i;
> +
> +	for (i = 0, field = rec->fields; i < cnt; i++, field++) {
> +		if (field->type != BPF_UPTR)
> +			continue;
> +
> +		uptr_addr = obj + field->offset;
> +		unpin_uptr_kaddr(*uptr_addr);
> +	}
> +}
> +
> +static void bpf_obj_unpin_uptrs(struct btf_record *rec, void *obj)
> +{
> +	if (!btf_record_has_field(rec, BPF_UPTR))
> +		return;
> +
> +	__bpf_obj_unpin_uptrs(rec, rec->cnt, obj);
> +}
> +
> +static int bpf_obj_pin_uptrs(struct btf_record *rec, void *obj)
> +{
> +	const struct btf_field *field;
> +	const struct btf_type *t;
> +	unsigned long start, end;
> +	struct page *page;
> +	void **uptr_addr;
> +	int i, err;
> +
> +	if (!btf_record_has_field(rec, BPF_UPTR))
> +		return 0;
> +
> +	for (i = 0, field = rec->fields; i < rec->cnt; i++, field++) {
> +		if (field->type != BPF_UPTR)
> +			continue;
> +
> +		uptr_addr = obj + field->offset;
> +		start = *(unsigned long *)uptr_addr;
> +		if (!start)
> +			continue;
> +
> +		t = btf_type_by_id(field->kptr.btf, field->kptr.btf_id);
> +		if (check_add_overflow(start, t->size, &end)) {
> +			err = -EFAULT;
> +			goto unpin_all;
> +		}
> +
> +		/* The uptr's struct cannot span across two pages */
> +		if ((start & PAGE_MASK) != (end & PAGE_MASK)) {
> +			err = -EOPNOTSUPP;
> +			goto unpin_all;
> +		}
> +
> +		err = pin_user_pages_fast(start, 1, FOLL_LONGTERM | FOLL_WRITE, &page);
> +		if (err != 1)
> +			goto unpin_all;
> +
> +		*uptr_addr = page_address(page) + offset_in_page(start);

Please use kmap(page) instead of page_address(page) and then you will
need to kunmap(kptr) on the unpin side.
Shakeel Butt Oct. 23, 2024, 12:57 a.m. UTC | #2 
On Tue, Oct 22, 2024 at 04:07:50PM GMT, Shakeel Butt wrote:
> On Mon, Oct 14, 2024 at 05:49:56PM GMT, Martin KaFai Lau wrote:
> > From: Martin KaFai Lau <martin.lau@kernel.org>
[...]
> > +
> > +		err = pin_user_pages_fast(start, 1, FOLL_LONGTERM | FOLL_WRITE, &page);
> > +		if (err != 1)
> > +			goto unpin_all;
> > +
> > +		*uptr_addr = page_address(page) + offset_in_page(start);
> 
> Please use kmap(page) instead of page_address(page) and then you will
> need to kunmap(kptr) on the unpin side.
> 

This is needed only if you plan to support your feature for HIGHMEM
kernels though. Otherwise you can error out for PageHighMem(page).
Martin KaFai Lau Oct. 24, 2024, 12:44 a.m. UTC | #3 
On 10/22/24 5:57 PM, Shakeel Butt wrote:
> On Tue, Oct 22, 2024 at 04:07:50PM GMT, Shakeel Butt wrote:
>> On Mon, Oct 14, 2024 at 05:49:56PM GMT, Martin KaFai Lau wrote:
>>> From: Martin KaFai Lau <martin.lau@kernel.org>
> [...]
>>> +
>>> +		err = pin_user_pages_fast(start, 1, FOLL_LONGTERM | FOLL_WRITE, &page);
>>> +		if (err != 1)
>>> +			goto unpin_all;
>>> +
>>> +		*uptr_addr = page_address(page) + offset_in_page(start);
>>
>> Please use kmap(page) instead of page_address(page) and then you will
>> need to kunmap(kptr) on the unpin side.
>>
> 
> This is needed only if you plan to support your feature for HIGHMEM
> kernels though. Otherwise you can error out for PageHighMem(page).

I just posted v6 with the PageHighMem(page). Thanks for the review.
diff mbox series

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index cdd0a891ce55..7bf1627019fc 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -424,6 +424,7 @@  static inline void bpf_obj_init_field(const struct btf_field *field, void *addr)
 	case BPF_KPTR_UNREF:
 	case BPF_KPTR_REF:
 	case BPF_KPTR_PERCPU:
+	case BPF_UPTR:
 		break;
 	default:
 		WARN_ON_ONCE(1);
@@ -512,6 +513,25 @@  static inline void copy_map_value_long(struct bpf_map *map, void *dst, void *src
 	bpf_obj_memcpy(map->record, dst, src, map->value_size, true);
 }
 
+static inline void bpf_obj_swap_uptrs(const struct btf_record *rec, void *dst, void *src)
+{
+	unsigned long *src_uptr, *dst_uptr;
+	const struct btf_field *field;
+	int i;
+
+	if (!btf_record_has_field(rec, BPF_UPTR))
+		return;
+
+	for (i = 0, field = rec->fields; i < rec->cnt; i++, field++) {
+		if (field->type != BPF_UPTR)
+			continue;
+
+		src_uptr = src + field->offset;
+		dst_uptr = dst + field->offset;
+		swap(*src_uptr, *dst_uptr);
+	}
+}
+
 static inline void bpf_obj_memzero(struct btf_record *rec, void *dst, u32 size)
 {
 	u32 curr_off = 0;
diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
index ca871be1c42d..7e6a0af0afc1 100644
--- a/kernel/bpf/bpf_local_storage.c
+++ b/kernel/bpf/bpf_local_storage.c
@@ -99,9 +99,12 @@  bpf_selem_alloc(struct bpf_local_storage_map *smap, void *owner,
 	}
 
 	if (selem) {
-		if (value)
+		if (value) {
+			/* No need to call check_and_init_map_value as memory is zero init */
 			copy_map_value(&smap->map, SDATA(selem)->data, value);
-		/* No need to call check_and_init_map_value as memory is zero init */
+			if (swap_uptrs)
+				bpf_obj_swap_uptrs(smap->map.record, SDATA(selem)->data, value);
+		}
 		return selem;
 	}
 
diff --git a/kernel/bpf/bpf_task_storage.c b/kernel/bpf/bpf_task_storage.c
index 45dc3ca334d3..09705f9988f3 100644
--- a/kernel/bpf/bpf_task_storage.c
+++ b/kernel/bpf/bpf_task_storage.c
@@ -129,6 +129,9 @@  static long bpf_pid_task_storage_update_elem(struct bpf_map *map, void *key,
 	struct pid *pid;
 	int fd, err;
 
+	if ((map_flags & BPF_F_LOCK) && btf_record_has_field(map->record, BPF_UPTR))
+		return -EOPNOTSUPP;
+
 	fd = *(int *)key;
 	pid = pidfd_get_pid(fd, &f_flags);
 	if (IS_ERR(pid))
@@ -147,7 +150,7 @@  static long bpf_pid_task_storage_update_elem(struct bpf_map *map, void *key,
 	bpf_task_storage_lock();
 	sdata = bpf_local_storage_update(
 		task, (struct bpf_local_storage_map *)map, value, map_flags,
-		false, GFP_ATOMIC);
+		true, GFP_ATOMIC);
 	bpf_task_storage_unlock();
 
 	err = PTR_ERR_OR_ZERO(sdata);
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 694dbbeb0eb5..eb2b25fc1fa6 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -155,6 +155,82 @@  static void maybe_wait_bpf_programs(struct bpf_map *map)
 		synchronize_rcu();
 }
 
+static void unpin_uptr_kaddr(void *kaddr)
+{
+	if (kaddr)
+		unpin_user_page(virt_to_page(kaddr));
+}
+
+static void __bpf_obj_unpin_uptrs(struct btf_record *rec, u32 cnt, void *obj)
+{
+	const struct btf_field *field;
+	void **uptr_addr;
+	int i;
+
+	for (i = 0, field = rec->fields; i < cnt; i++, field++) {
+		if (field->type != BPF_UPTR)
+			continue;
+
+		uptr_addr = obj + field->offset;
+		unpin_uptr_kaddr(*uptr_addr);
+	}
+}
+
+static void bpf_obj_unpin_uptrs(struct btf_record *rec, void *obj)
+{
+	if (!btf_record_has_field(rec, BPF_UPTR))
+		return;
+
+	__bpf_obj_unpin_uptrs(rec, rec->cnt, obj);
+}
+
+static int bpf_obj_pin_uptrs(struct btf_record *rec, void *obj)
+{
+	const struct btf_field *field;
+	const struct btf_type *t;
+	unsigned long start, end;
+	struct page *page;
+	void **uptr_addr;
+	int i, err;
+
+	if (!btf_record_has_field(rec, BPF_UPTR))
+		return 0;
+
+	for (i = 0, field = rec->fields; i < rec->cnt; i++, field++) {
+		if (field->type != BPF_UPTR)
+			continue;
+
+		uptr_addr = obj + field->offset;
+		start = *(unsigned long *)uptr_addr;
+		if (!start)
+			continue;
+
+		t = btf_type_by_id(field->kptr.btf, field->kptr.btf_id);
+		if (check_add_overflow(start, t->size, &end)) {
+			err = -EFAULT;
+			goto unpin_all;
+		}
+
+		/* The uptr's struct cannot span across two pages */
+		if ((start & PAGE_MASK) != (end & PAGE_MASK)) {
+			err = -EOPNOTSUPP;
+			goto unpin_all;
+		}
+
+		err = pin_user_pages_fast(start, 1, FOLL_LONGTERM | FOLL_WRITE, &page);
+		if (err != 1)
+			goto unpin_all;
+
+		*uptr_addr = page_address(page) + offset_in_page(start);
+	}
+
+	return 0;
+
+unpin_all:
+	__bpf_obj_unpin_uptrs(rec, i, obj);
+	return err;
+}
+
 static int bpf_map_update_value(struct bpf_map *map, struct file *map_file,
 				void *key, void *value, __u64 flags)
 {
@@ -199,9 +275,14 @@  static int bpf_map_update_value(struct bpf_map *map, struct file *map_file,
 		   map->map_type == BPF_MAP_TYPE_BLOOM_FILTER) {
 		err = map->ops->map_push_elem(map, value, flags);
 	} else {
-		rcu_read_lock();
-		err = map->ops->map_update_elem(map, key, value, flags);
-		rcu_read_unlock();
+		err = bpf_obj_pin_uptrs(map->record, value);
+		if (!err) {
+			rcu_read_lock();
+			err = map->ops->map_update_elem(map, key, value, flags);
+			rcu_read_unlock();
+			if (err)
+				bpf_obj_unpin_uptrs(map->record, value);
+		}
 	}
 	bpf_enable_instrumentation();
 
@@ -716,6 +797,10 @@  void bpf_obj_free_fields(const struct btf_record *rec, void *obj)
 				field->kptr.dtor(xchgd_field);
 			}
 			break;
+		case BPF_UPTR:
+			/* The caller ensured that no one is using the uptr */
+			unpin_uptr_kaddr(*(void **)field_ptr);
+			break;
 		case BPF_LIST_HEAD:
 			if (WARN_ON_ONCE(rec->spin_lock_off < 0))
 				continue;
@@ -1107,7 +1192,7 @@  static int map_check_btf(struct bpf_map *map, struct bpf_token *token,
 
 	map->record = btf_parse_fields(btf, value_type,
 				       BPF_SPIN_LOCK | BPF_TIMER | BPF_KPTR | BPF_LIST_HEAD |
-				       BPF_RB_ROOT | BPF_REFCOUNT | BPF_WORKQUEUE,
+				       BPF_RB_ROOT | BPF_REFCOUNT | BPF_WORKQUEUE | BPF_UPTR,
 				       map->value_size);
 	if (!IS_ERR_OR_NULL(map->record)) {
 		int i;
@@ -1163,6 +1248,12 @@  static int map_check_btf(struct bpf_map *map, struct bpf_token *token,
 					goto free_map_tab;
 				}
 				break;
+			case BPF_UPTR:
+				if (map->map_type != BPF_MAP_TYPE_TASK_STORAGE) {
+					ret = -EOPNOTSUPP;
+					goto free_map_tab;
+				}
+				break;
 			case BPF_LIST_HEAD:
 			case BPF_RB_ROOT:
 				if (map->map_type != BPF_MAP_TYPE_HASH &&