bpf: Remove gfp_flags plumbing from bpf_local_storage_update()

Remove the check that rejects sleepable BPF programs from doing BPF_ANY/BPF_EXIST updates on local storage. This restriction was added in commit b00fa38a9c1c ("bpf: Enable non-atomic allocations in local storage") because kzalloc(GFP_KERNEL) could sleep inside local_storage->lock. This is no longer a concern: all local storage allocations now use kmalloc_nolock() which never sleeps. In addition, since kmalloc_nolock() only accepts __GFP_ACCOUNT, __GFP_ZERO and __GFP_NO_OBJ_EXT, the gfp_flags parameter plumbing from bpf_*_storage_get() to bpf_local_storage_update() becomes dead code. Remove gfp_flags from bpf_selem_alloc(), bpf_local_storage_alloc() and bpf_local_storage_update(). Drop the hidden 5th argument from bpf_*_storage_get helpers, and remove the verifier patching that injected GFP_KERNEL/GFP_ATOMIC into the fifth argument.

Amery Hung committed 7 months ago

Commit 136deea1 parent 5063e777 files · +30 −67Browse files

Unified · Split

7 files

include/linux/bpf_local_storage.h +3 −4
kernel/bpf/bpf_cgrp_storage.c +4 −5
kernel/bpf/bpf_inode_storage.c +4 −5
kernel/bpf/bpf_local_storage.c +6 −10
kernel/bpf/bpf_task_storage.c +4 −5
kernel/bpf/verifier.c +0 −26
net/core/bpf_sk_storage.c +9 −12

include/linux/bpf_local_storage.h+3−4

@@ -188,7 +188,7 @@ int bpf_selem_link_map(struct bpf_local_storage_map *smap, · 187 unmodified lines
188	188
189	189	struct bpf_local_storage_elem *
190	190	bpf_selem_alloc(struct bpf_local_storage_map smap, void owner, void *value,
191		− bool swap_uptrs, gfp_t gfp_flags);
	191	+ bool swap_uptrs);
192	192
193	193	void bpf_selem_free(struct bpf_local_storage_elem *selem,
194	194	bool reuse_now);
@@ -196,12 +196,11 @@ void bpf_selem_free(struct bpf_local_storage_elem *selem, · 1 unmodified line
196	196	int
197	197	bpf_local_storage_alloc(void *owner,
198	198	struct bpf_local_storage_map *smap,
199		− struct bpf_local_storage_elem *first_selem,
200		− gfp_t gfp_flags);
	199	+ struct bpf_local_storage_elem *first_selem);
201	200
202	201	struct bpf_local_storage_data *
203	202	bpf_local_storage_update(void owner, struct bpf_local_storage_map smap,
204		− void *value, u64 map_flags, bool swap_uptrs, gfp_t gfp_flags);
	203	+ void *value, u64 map_flags, bool swap_uptrs);
205	204
206	205	u64 bpf_local_storage_map_mem_usage(const struct bpf_map *map);
207	206

kernel/bpf/bpf_cgrp_storage.c+4−5

@@ -76,7 +76,7 @@ static long bpf_cgrp_storage_update_elem(struct bpf_map map, void key, · 75 unmodified lines
76	76	return PTR_ERR(cgroup);
77	77
78	78	sdata = bpf_local_storage_update(cgroup, (struct bpf_local_storage_map *)map,
79		− value, map_flags, false, GFP_ATOMIC);
	79	+ value, map_flags, false);
80	80	cgroup_put(cgroup);
81	81	return PTR_ERR_OR_ZERO(sdata);
82	82	}
@@ -122,9 +122,8 @@ static void cgroup_storage_map_free(struct bpf_map *map) · 39 unmodified lines
122	122	bpf_local_storage_map_free(map, &cgroup_cache);
123	123	}
124	124
125		−/* gfp_flags is a hidden argument provided by the verifier */
126		−BPF_CALL_5(bpf_cgrp_storage_get, struct bpf_map , map, struct cgroup , cgroup,
127		− void *, value, u64, flags, gfp_t, gfp_flags)
	125	+BPF_CALL_4(bpf_cgrp_storage_get, struct bpf_map , map, struct cgroup , cgroup,
	126	+ void *, value, u64, flags)
128	127	{
129	128	struct bpf_local_storage_data *sdata;
130	129
@@ -143,7 +142,7 @@ BPF_CALL_5(bpf_cgrp_storage_get, struct bpf_map , map, struct cgroup , cgroup, · 12 unmodified lines
143	142	if (!percpu_ref_is_dying(&cgroup->self.refcnt) &&
144	143	(flags & BPF_LOCAL_STORAGE_GET_F_CREATE))
145	144	sdata = bpf_local_storage_update(cgroup, (struct bpf_local_storage_map *)map,
146		− value, BPF_NOEXIST, false, gfp_flags);
	145	+ value, BPF_NOEXIST, false);
147	146
148	147	out:
149	148	return IS_ERR_OR_NULL(sdata) ? (unsigned long)NULL : (unsigned long)sdata->data;

kernel/bpf/bpf_inode_storage.c+4−5

@@ -98,7 +98,7 @@ static long bpf_fd_inode_storage_update_elem(struct bpf_map map, void key, · 97 unmodified lines
98	98
99	99	sdata = bpf_local_storage_update(file_inode(fd_file(f)),
100	100	(struct bpf_local_storage_map *)map,
101		− value, map_flags, false, GFP_ATOMIC);
	101	+ value, map_flags, false);
102	102	return PTR_ERR_OR_ZERO(sdata);
103	103	}
104	104
@@ -122,9 +122,8 @@ static long bpf_fd_inode_storage_delete_elem(struct bpf_map map, void key) · 17 unmodified lines
122	122	return inode_storage_delete(file_inode(fd_file(f)), map);
123	123	}
124	124
125		−/* gfp_flags is a hidden argument provided by the verifier */
126		−BPF_CALL_5(bpf_inode_storage_get, struct bpf_map , map, struct inode , inode,
127		− void *, value, u64, flags, gfp_t, gfp_flags)
	125	+BPF_CALL_4(bpf_inode_storage_get, struct bpf_map , map, struct inode , inode,
	126	+ void *, value, u64, flags)
128	127	{
129	128	struct bpf_local_storage_data *sdata;
130	129
@@ -150,7 +149,7 @@ BPF_CALL_5(bpf_inode_storage_get, struct bpf_map , map, struct inode , inode, · 19 unmodified lines
150	149	if (flags & BPF_LOCAL_STORAGE_GET_F_CREATE) {
151	150	sdata = bpf_local_storage_update(
152	151	inode, (struct bpf_local_storage_map *)map, value,
153		− BPF_NOEXIST, false, gfp_flags);
	152	+ BPF_NOEXIST, false);
154	153	return IS_ERR(sdata) ? (unsigned long)NULL :
155	154	(unsigned long)sdata->data;
156	155	}

kernel/bpf/bpf_local_storage.c+6−10

@@ -68,7 +68,7 @@ static bool selem_linked_to_map(const struct bpf_local_storage_elem *selem) · 67 unmodified lines
68	68
69	69	struct bpf_local_storage_elem *
70	70	bpf_selem_alloc(struct bpf_local_storage_map smap, void owner,
71		− void *value, bool swap_uptrs, gfp_t gfp_flags)
	71	+ void *value, bool swap_uptrs)
72	72	{
73	73	struct bpf_local_storage_elem *selem;
74	74
@@ -475,8 +475,7 @@ static int check_flags(const struct bpf_local_storage_data *old_sdata, · 400 unmodified lines
475	475
476	476	int bpf_local_storage_alloc(void *owner,
477	477	struct bpf_local_storage_map *smap,
478		− struct bpf_local_storage_elem *first_selem,
479		− gfp_t gfp_flags)
	478	+ struct bpf_local_storage_elem *first_selem)
480	479	{
481	480	struct bpf_local_storage prev_storage, storage;
482	481	struct bpf_local_storage **owner_storage_ptr;
@@ -546,7 +545,7 @@ uncharge: · 63 unmodified lines
546	545	*/
547	546	struct bpf_local_storage_data *
548	547	bpf_local_storage_update(void owner, struct bpf_local_storage_map smap,
549		− void *value, u64 map_flags, bool swap_uptrs, gfp_t gfp_flags)
	548	+ void *value, u64 map_flags, bool swap_uptrs)
550	549	{
551	550	struct bpf_local_storage_data *old_sdata = NULL;
552	551	struct bpf_local_storage_elem alloc_selem, selem = NULL;
@@ -563,9 +562,6 @@ bpf_local_storage_update(void owner, struct bpf_local_storage_map smap, · 10 unmodified lines
563	562	!btf_record_has_field(smap->map.record, BPF_SPIN_LOCK)))
564	563	return ERR_PTR(-EINVAL);
565	564
566		− if (gfp_flags == GFP_KERNEL && (map_flags & ~BPF_F_LOCK) != BPF_NOEXIST)
567		− return ERR_PTR(-EINVAL);
568		−
569	565	local_storage = rcu_dereference_check(*owner_storage(smap, owner),
570	566	bpf_rcu_lock_held());
571	567	if (!local_storage \|\| hlist_empty(&local_storage->list)) {
@@ -574,11 +570,11 @@ bpf_local_storage_update(void owner, struct bpf_local_storage_map smap, · 2 unmodified lines
574	570	if (err)
575	571	return ERR_PTR(err);
576	572
577		− selem = bpf_selem_alloc(smap, owner, value, swap_uptrs, gfp_flags);
	573	+ selem = bpf_selem_alloc(smap, owner, value, swap_uptrs);
578	574	if (!selem)
579	575	return ERR_PTR(-ENOMEM);
580	576
581		− err = bpf_local_storage_alloc(owner, smap, selem, gfp_flags);
	577	+ err = bpf_local_storage_alloc(owner, smap, selem);
582	578	if (err) {
583	579	bpf_selem_free(selem, true);
584	580	mem_uncharge(smap, owner, smap->elem_size);
@@ -608,7 +604,7 @@ bpf_local_storage_update(void owner, struct bpf_local_storage_map smap, · 23 unmodified lines
608	604	/* A lookup has just been done before and concluded a new selem is
609	605	* needed. The chance of an unnecessary alloc is unlikely.
610	606	*/
611		− alloc_selem = selem = bpf_selem_alloc(smap, owner, value, swap_uptrs, gfp_flags);
	607	+ alloc_selem = selem = bpf_selem_alloc(smap, owner, value, swap_uptrs);
612	608	if (!alloc_selem)
613	609	return ERR_PTR(-ENOMEM);
614	610

kernel/bpf/bpf_task_storage.c+4−5

@@ -118,7 +118,7 @@ static long bpf_pid_task_storage_update_elem(struct bpf_map map, void key, · 117 unmodified lines
118	118
119	119	sdata = bpf_local_storage_update(
120	120	task, (struct bpf_local_storage_map *)map, value, map_flags,
121		− true, GFP_ATOMIC);
	121	+ true);
122	122
123	123	err = PTR_ERR_OR_ZERO(sdata);
124	124	out:
@@ -165,9 +165,8 @@ out: · 40 unmodified lines
165	165	return err;
166	166	}
167	167
168		−/* gfp_flags is a hidden argument provided by the verifier */
169		−BPF_CALL_5(bpf_task_storage_get, struct bpf_map , map, struct task_struct ,
170		− task, void *, value, u64, flags, gfp_t, gfp_flags)
	168	+BPF_CALL_4(bpf_task_storage_get, struct bpf_map , map, struct task_struct ,
	169	+ task, void *, value, u64, flags)
171	170	{
172	171	struct bpf_local_storage_data *sdata;
173	172
@@ -184,7 +183,7 @@ BPF_CALL_5(bpf_task_storage_get, struct bpf_map , map, struct task_struct , · 10 unmodified lines
184	183	(flags & BPF_LOCAL_STORAGE_GET_F_CREATE)) {
185	184	sdata = bpf_local_storage_update(
186	185	task, (struct bpf_local_storage_map *)map, value,
187		− BPF_NOEXIST, false, gfp_flags);
	186	+ BPF_NOEXIST, false);
188	187	return IS_ERR(sdata) ? (unsigned long)NULL : (unsigned long)sdata->data;
189	188	}
190	189

kernel/bpf/verifier.c+0−26

@@ -589,14 +589,6 @@ static bool is_may_goto_insn_at(struct bpf_verifier_env *env, int insn_idx) · 588 unmodified lines
589	589	return bpf_is_may_goto_insn(&env->prog->insnsi[insn_idx]);
590	590	}
591	591
592		−static bool is_storage_get_function(enum bpf_func_id func_id)
593		−{
594		− return func_id == BPF_FUNC_sk_storage_get \|\|
595		− func_id == BPF_FUNC_inode_storage_get \|\|
596		− func_id == BPF_FUNC_task_storage_get \|\|
597		− func_id == BPF_FUNC_cgrp_storage_get;
598		−}
599		−
600	592	static bool helper_multiple_ref_obj_use(enum bpf_func_id func_id,
601	593	const struct bpf_map *map)
602	594	{
@@ -24429,24 +24421,6 @@ static int do_misc_fixups(struct bpf_verifier_env *env) · 23826 unmodified lines
24429	24421	goto patch_call_imm;
24430	24422	}
24431	24423
24432		− if (is_storage_get_function(insn->imm)) {
24433		− if (env->insn_aux_data[i + delta].non_sleepable)
24434		− insn_buf[0] = BPF_MOV64_IMM(BPF_REG_5, (__force __s32)GFP_ATOMIC);
24435		− else
24436		− insn_buf[0] = BPF_MOV64_IMM(BPF_REG_5, (__force __s32)GFP_KERNEL);
24437		− insn_buf[1] = *insn;
24438		− cnt = 2;
24439		−
24440		− new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
24441		− if (!new_prog)
24442		− return -ENOMEM;
24443		−
24444		− delta += cnt - 1;
24445		− env->prog = prog = new_prog;
24446		− insn = new_prog->insnsi + i + delta;
24447		− goto patch_call_imm;
24448		− }
24449		−
24450	24424	/* bpf_per_cpu_ptr() and bpf_this_cpu_ptr() */
24451	24425	if (env->insn_aux_data[i + delta].call_with_percpu_alloc_ptr) {
24452	24426	/* patch with 'r1 = (u64 )(r1 + 0)' since for percpu data,

net/core/bpf_sk_storage.c+9−12

@@ -106,7 +106,7 @@ static long bpf_fd_sk_storage_update_elem(struct bpf_map map, void key, · 105 unmodified lines
106	106	if (sock) {
107	107	sdata = bpf_local_storage_update(
108	108	sock->sk, (struct bpf_local_storage_map *)map, value,
109		− map_flags, false, GFP_ATOMIC);
	109	+ map_flags, false);
110	110	sockfd_put(sock);
111	111	return PTR_ERR_OR_ZERO(sdata);
112	112	}
@@ -137,7 +137,7 @@ bpf_sk_storage_clone_elem(struct sock *newsk, · 24 unmodified lines
137	137	{
138	138	struct bpf_local_storage_elem *copy_selem;
139	139
140		− copy_selem = bpf_selem_alloc(smap, newsk, NULL, false, GFP_ATOMIC);
	140	+ copy_selem = bpf_selem_alloc(smap, newsk, NULL, false);
141	141	if (!copy_selem)
142	142	return NULL;
143	143
@@ -202,7 +202,7 @@ int bpf_sk_storage_clone(const struct sock sk, struct sock newsk) · 58 unmodified lines
202	202	}
203	203	bpf_selem_link_storage_nolock(new_sk_storage, copy_selem);
204	204	} else {
205		− ret = bpf_local_storage_alloc(newsk, smap, copy_selem, GFP_ATOMIC);
	205	+ ret = bpf_local_storage_alloc(newsk, smap, copy_selem);
206	206	if (ret) {
207	207	bpf_selem_free(copy_selem, true);
208	208	atomic_sub(smap->elem_size,
@@ -227,9 +227,8 @@ out: · 18 unmodified lines
227	227	return ret;
228	228	}
229	229
230		−/* gfp_flags is a hidden argument provided by the verifier */
231		−BPF_CALL_5(bpf_sk_storage_get, struct bpf_map , map, struct sock , sk,
232		− void *, value, u64, flags, gfp_t, gfp_flags)
	230	+BPF_CALL_4(bpf_sk_storage_get, struct bpf_map , map, struct sock , sk,
	231	+ void *, value, u64, flags)
233	232	{
234	233	struct bpf_local_storage_data *sdata;
235	234
@@ -250,7 +249,7 @@ BPF_CALL_5(bpf_sk_storage_get, struct bpf_map , map, struct sock , sk, · 14 unmodified lines
250	249	refcount_inc_not_zero(&sk->sk_refcnt)) {
251	250	sdata = bpf_local_storage_update(
252	251	sk, (struct bpf_local_storage_map *)map, value,
253		− BPF_NOEXIST, false, gfp_flags);
	252	+ BPF_NOEXIST, false);
254	253	/* sk must be a fullsock (guaranteed by verifier),
255	254	* so sock_gen_put() is unnecessary.
256	255	*/
@@ -383,16 +382,14 @@ static bool bpf_sk_storage_tracing_allowed(const struct bpf_prog *prog) · 126 unmodified lines
383	382	return false;
384	383	}
385	384
386		−/* gfp_flags is a hidden argument provided by the verifier */
387		−BPF_CALL_5(bpf_sk_storage_get_tracing, struct bpf_map , map, struct sock , sk,
388		− void *, value, u64, flags, gfp_t, gfp_flags)
	385	+BPF_CALL_4(bpf_sk_storage_get_tracing, struct bpf_map , map, struct sock , sk,
	386	+ void *, value, u64, flags)
389	387	{
390	388	WARN_ON_ONCE(!bpf_rcu_lock_held());
391	389	if (in_hardirq() \|\| in_nmi())
392	390	return (unsigned long)NULL;
393	391
394		− return (unsigned long)____bpf_sk_storage_get(map, sk, value, flags,
395		− gfp_flags);
	392	+ return (unsigned long)____bpf_sk_storage_get(map, sk, value, flags);
396	393	}
397	394
398	395	BPF_CALL_2(bpf_sk_storage_delete_tracing, struct bpf_map *, map,