* [PATCH bpf-next] RFC: bpf: query effective progs without cgroup_mutex
@ 2023-05-05 18:45 Stanislav Fomichev
2023-05-05 18:47 ` Stanislav Fomichev
0 siblings, 1 reply; 10+ messages in thread
From: Stanislav Fomichev @ 2023-05-05 18:45 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
We're observing some stalls on the heavily loaded machines
in the cgroup_bpf_prog_query path. This is likely due to
being blocked on cgroup_mutex.
IIUC, the cgroup_mutex is there mostly to protect the non-effective
fields (cgrp->bpf.progs) which might be changed by the update path.
For the BPF_F_QUERY_EFFECTIVE case, all we need is to rcu_dereference
a bunch of pointers (and keep them around for consistency), so
let's do it.
Sending out as an RFC because it looks a bit ugly. It would also
be nice to handle non-effective case locklessly as well, but it
might require a larger rework.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
kernel/bpf/cgroup.c | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index a06e118a9be5..c9d4b66e2c15 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -1022,10 +1022,10 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
__u32 __user *prog_attach_flags = u64_to_user_ptr(attr->query.prog_attach_flags);
bool effective_query = attr->query.query_flags & BPF_F_QUERY_EFFECTIVE;
__u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
+ struct bpf_prog_array *effective[MAX_CGROUP_BPF_ATTACH_TYPE];
enum bpf_attach_type type = attr->query.attach_type;
enum cgroup_bpf_attach_type from_atype, to_atype;
enum cgroup_bpf_attach_type atype;
- struct bpf_prog_array *effective;
int cnt, ret = 0, i;
int total_cnt = 0;
u32 flags;
@@ -1051,9 +1051,9 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
for (atype = from_atype; atype <= to_atype; atype++) {
if (effective_query) {
- effective = rcu_dereference_protected(cgrp->bpf.effective[atype],
- lockdep_is_held(&cgroup_mutex));
- total_cnt += bpf_prog_array_length(effective);
+ effective[atype] = rcu_dereference_protected(cgrp->bpf.effective[atype],
+ lockdep_is_held(&cgroup_mutex));
+ total_cnt += bpf_prog_array_length(effective[atype]);
} else {
total_cnt += prog_list_length(&cgrp->bpf.progs[atype]);
}
@@ -1076,10 +1076,8 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
for (atype = from_atype; atype <= to_atype && total_cnt; atype++) {
if (effective_query) {
- effective = rcu_dereference_protected(cgrp->bpf.effective[atype],
- lockdep_is_held(&cgroup_mutex));
- cnt = min_t(int, bpf_prog_array_length(effective), total_cnt);
- ret = bpf_prog_array_copy_to_user(effective, prog_ids, cnt);
+ cnt = min_t(int, bpf_prog_array_length(effective[atype]), total_cnt);
+ ret = bpf_prog_array_copy_to_user(effective[atype], prog_ids, cnt);
} else {
struct hlist_head *progs;
struct bpf_prog_list *pl;
@@ -1118,11 +1116,23 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
static int cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
union bpf_attr __user *uattr)
{
+ __u32 __user *prog_attach_flags = u64_to_user_ptr(attr->query.prog_attach_flags);
+ bool effective_query = attr->query.query_flags & BPF_F_QUERY_EFFECTIVE;
+ bool need_mutex = false;
int ret;
- mutex_lock(&cgroup_mutex);
+ if (effective_query && !prog_attach_flags)
+ need_mutex = false;
+
+ if (need_mutex)
+ mutex_lock(&cgroup_mutex);
+ else
+ rcu_read_lock();
ret = __cgroup_bpf_query(cgrp, attr, uattr);
- mutex_unlock(&cgroup_mutex);
+ if (need_mutex)
+ mutex_unlock(&cgroup_mutex);
+ else
+ rcu_read_unlock();
return ret;
}
--
2.40.1.521.gf1e218fcd8-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next] RFC: bpf: query effective progs without cgroup_mutex
2023-05-05 18:45 Stanislav Fomichev
@ 2023-05-05 18:47 ` Stanislav Fomichev
2023-05-05 21:35 ` Stanislav Fomichev
0 siblings, 1 reply; 10+ messages in thread
From: Stanislav Fomichev @ 2023-05-05 18:47 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, haoluo, jolsa
On Fri, May 5, 2023 at 11:45 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> We're observing some stalls on the heavily loaded machines
> in the cgroup_bpf_prog_query path. This is likely due to
> being blocked on cgroup_mutex.
>
> IIUC, the cgroup_mutex is there mostly to protect the non-effective
> fields (cgrp->bpf.progs) which might be changed by the update path.
> For the BPF_F_QUERY_EFFECTIVE case, all we need is to rcu_dereference
> a bunch of pointers (and keep them around for consistency), so
> let's do it.
>
> Sending out as an RFC because it looks a bit ugly. It would also
> be nice to handle non-effective case locklessly as well, but it
> might require a larger rework.
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
> kernel/bpf/cgroup.c | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index a06e118a9be5..c9d4b66e2c15 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -1022,10 +1022,10 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
> __u32 __user *prog_attach_flags = u64_to_user_ptr(attr->query.prog_attach_flags);
> bool effective_query = attr->query.query_flags & BPF_F_QUERY_EFFECTIVE;
> __u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
> + struct bpf_prog_array *effective[MAX_CGROUP_BPF_ATTACH_TYPE];
> enum bpf_attach_type type = attr->query.attach_type;
> enum cgroup_bpf_attach_type from_atype, to_atype;
> enum cgroup_bpf_attach_type atype;
> - struct bpf_prog_array *effective;
> int cnt, ret = 0, i;
> int total_cnt = 0;
> u32 flags;
> @@ -1051,9 +1051,9 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
>
> for (atype = from_atype; atype <= to_atype; atype++) {
> if (effective_query) {
> - effective = rcu_dereference_protected(cgrp->bpf.effective[atype],
> - lockdep_is_held(&cgroup_mutex));
> - total_cnt += bpf_prog_array_length(effective);
> + effective[atype] = rcu_dereference_protected(cgrp->bpf.effective[atype],
> + lockdep_is_held(&cgroup_mutex));
> + total_cnt += bpf_prog_array_length(effective[atype]);
> } else {
> total_cnt += prog_list_length(&cgrp->bpf.progs[atype]);
> }
> @@ -1076,10 +1076,8 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
>
> for (atype = from_atype; atype <= to_atype && total_cnt; atype++) {
> if (effective_query) {
> - effective = rcu_dereference_protected(cgrp->bpf.effective[atype],
> - lockdep_is_held(&cgroup_mutex));
> - cnt = min_t(int, bpf_prog_array_length(effective), total_cnt);
> - ret = bpf_prog_array_copy_to_user(effective, prog_ids, cnt);
> + cnt = min_t(int, bpf_prog_array_length(effective[atype]), total_cnt);
> + ret = bpf_prog_array_copy_to_user(effective[atype], prog_ids, cnt);
> } else {
> struct hlist_head *progs;
> struct bpf_prog_list *pl;
> @@ -1118,11 +1116,23 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
> static int cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
> union bpf_attr __user *uattr)
> {
> + __u32 __user *prog_attach_flags = u64_to_user_ptr(attr->query.prog_attach_flags);
> + bool effective_query = attr->query.query_flags & BPF_F_QUERY_EFFECTIVE;
> + bool need_mutex = false;
Oops, this has to be true, but you get the idea...
> int ret;
>
> - mutex_lock(&cgroup_mutex);
> + if (effective_query && !prog_attach_flags)
> + need_mutex = false;
> +
> + if (need_mutex)
> + mutex_lock(&cgroup_mutex);
> + else
> + rcu_read_lock();
> ret = __cgroup_bpf_query(cgrp, attr, uattr);
> - mutex_unlock(&cgroup_mutex);
> + if (need_mutex)
> + mutex_unlock(&cgroup_mutex);
> + else
> + rcu_read_unlock();
> return ret;
> }
>
> --
> 2.40.1.521.gf1e218fcd8-goog
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next] RFC: bpf: query effective progs without cgroup_mutex
2023-05-05 18:47 ` Stanislav Fomichev
@ 2023-05-05 21:35 ` Stanislav Fomichev
0 siblings, 0 replies; 10+ messages in thread
From: Stanislav Fomichev @ 2023-05-05 21:35 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, haoluo, jolsa
On 05/05, Stanislav Fomichev wrote:
> On Fri, May 5, 2023 at 11:45 AM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > We're observing some stalls on the heavily loaded machines
> > in the cgroup_bpf_prog_query path. This is likely due to
> > being blocked on cgroup_mutex.
> >
> > IIUC, the cgroup_mutex is there mostly to protect the non-effective
> > fields (cgrp->bpf.progs) which might be changed by the update path.
> > For the BPF_F_QUERY_EFFECTIVE case, all we need is to rcu_dereference
> > a bunch of pointers (and keep them around for consistency), so
> > let's do it.
> >
> > Sending out as an RFC because it looks a bit ugly. It would also
> > be nice to handle non-effective case locklessly as well, but it
> > might require a larger rework.
> >
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
Ah, that wont work, can't copy to user while holding rcu.
Maybe we can even go as far as converting the progs list to rcu..
This should allow as to drop mutex altogether during the query path.
As long as we return EAGAIN on racy deatach we should be fine it seems.
Will try to play with this idea.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH bpf-next] RFC: bpf: query effective progs without cgroup_mutex
@ 2023-05-11 15:57 Stanislav Fomichev
2023-05-11 15:57 ` [PATCH bpf-next v5 0/4] bpf: Don't EFAULT for {g,s}setsockopt with wrong optlen Stanislav Fomichev
` (5 more replies)
0 siblings, 6 replies; 10+ messages in thread
From: Stanislav Fomichev @ 2023-05-11 15:57 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
We're observing some stalls on the heavily loaded machines
in the cgroup_bpf_prog_query path. This is likely due to
being blocked on cgroup_mutex.
IIUC, the cgroup_mutex is there mostly to protect the non-effective
fields (cgrp->bpf.progs) which might be changed by the update path.
For the BPF_F_QUERY_EFFECTIVE case, all we need is to rcu_dereference
a bunch of pointers (and keep them around for consistency), so
let's do it.
Sending out as an RFC because it looks a bit ugly. It would also
be nice to handle non-effective case locklessly as well, but it
might require a larger rework.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
include/linux/bpf-cgroup-defs.h | 2 +-
include/linux/bpf-cgroup.h | 1 +
kernel/bpf/cgroup.c | 80 +++++++++++++++++----------------
3 files changed, 44 insertions(+), 39 deletions(-)
diff --git a/include/linux/bpf-cgroup-defs.h b/include/linux/bpf-cgroup-defs.h
index 7b121bd780eb..df0b8faa1a17 100644
--- a/include/linux/bpf-cgroup-defs.h
+++ b/include/linux/bpf-cgroup-defs.h
@@ -56,7 +56,7 @@ struct cgroup_bpf {
* have either zero or one element
* when BPF_F_ALLOW_MULTI the list can have up to BPF_CGROUP_MAX_PROGS
*/
- struct hlist_head progs[MAX_CGROUP_BPF_ATTACH_TYPE];
+ struct hlist_head __rcu progs[MAX_CGROUP_BPF_ATTACH_TYPE];
u8 flags[MAX_CGROUP_BPF_ATTACH_TYPE];
/* list of cgroup shared storages */
diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
index 57e9e109257e..555e9cbb3a05 100644
--- a/include/linux/bpf-cgroup.h
+++ b/include/linux/bpf-cgroup.h
@@ -106,6 +106,7 @@ struct bpf_prog_list {
struct bpf_prog *prog;
struct bpf_cgroup_link *link;
struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE];
+ struct rcu_head rcu;
};
int cgroup_bpf_inherit(struct cgroup *cgrp);
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index a06e118a9be5..92a1b33dcc06 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -285,12 +285,15 @@ static void cgroup_bpf_release(struct work_struct *work)
mutex_lock(&cgroup_mutex);
for (atype = 0; atype < ARRAY_SIZE(cgrp->bpf.progs); atype++) {
- struct hlist_head *progs = &cgrp->bpf.progs[atype];
+ struct hlist_head *progs;
struct bpf_prog_list *pl;
struct hlist_node *pltmp;
+ progs = rcu_dereference_protected(&cgrp->bpf.progs[atype],
+ lockdep_is_held(&cgroup_mutex));
+
hlist_for_each_entry_safe(pl, pltmp, progs, node) {
- hlist_del(&pl->node);
+ hlist_del_rcu(&pl->node);
if (pl->prog) {
if (pl->prog->expected_attach_type == BPF_LSM_CGROUP)
bpf_trampoline_unlink_cgroup_shim(pl->prog);
@@ -301,7 +304,7 @@ static void cgroup_bpf_release(struct work_struct *work)
bpf_trampoline_unlink_cgroup_shim(pl->link->link.prog);
bpf_cgroup_link_auto_detach(pl->link);
}
- kfree(pl);
+ kfree_rcu(pl, rcu);
static_branch_dec(&cgroup_bpf_enabled_key[atype]);
}
old_array = rcu_dereference_protected(
@@ -352,12 +355,12 @@ static struct bpf_prog *prog_list_prog(struct bpf_prog_list *pl)
/* count number of elements in the list.
* it's slow but the list cannot be long
*/
-static u32 prog_list_length(struct hlist_head *head)
+static u32 prog_list_length(struct hlist_head __rcu *head)
{
struct bpf_prog_list *pl;
u32 cnt = 0;
- hlist_for_each_entry(pl, head, node) {
+ hlist_for_each_entry_rcu(pl, head, node) {
if (!prog_list_prog(pl))
continue;
cnt++;
@@ -553,7 +556,7 @@ static int update_effective_progs(struct cgroup *cgrp,
#define BPF_CGROUP_MAX_PROGS 64
-static struct bpf_prog_list *find_attach_entry(struct hlist_head *progs,
+static struct bpf_prog_list *find_attach_entry(struct hlist_head __rcu *progs,
struct bpf_prog *prog,
struct bpf_cgroup_link *link,
struct bpf_prog *replace_prog,
@@ -565,10 +568,10 @@ static struct bpf_prog_list *find_attach_entry(struct hlist_head *progs,
if (!allow_multi) {
if (hlist_empty(progs))
return NULL;
- return hlist_entry(progs->first, typeof(*pl), node);
+ return hlist_entry(rcu_dereference_raw(progs)->first, typeof(*pl), node);
}
- hlist_for_each_entry(pl, progs, node) {
+ hlist_for_each_entry_rcu(pl, progs, node) {
if (prog && pl->prog == prog && prog != replace_prog)
/* disallow attaching the same prog twice */
return ERR_PTR(-EINVAL);
@@ -579,7 +582,7 @@ static struct bpf_prog_list *find_attach_entry(struct hlist_head *progs,
/* direct prog multi-attach w/ replacement case */
if (replace_prog) {
- hlist_for_each_entry(pl, progs, node) {
+ hlist_for_each_entry_rcu(pl, progs, node) {
if (pl->prog == replace_prog)
/* a match found */
return pl;
@@ -615,8 +618,8 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
struct bpf_cgroup_storage *new_storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {};
struct bpf_prog *new_prog = prog ? : link->link.prog;
enum cgroup_bpf_attach_type atype;
+ struct hlist_head __rcu *progs;
struct bpf_prog_list *pl;
- struct hlist_head *progs;
int err;
if (((flags & BPF_F_ALLOW_OVERRIDE) && (flags & BPF_F_ALLOW_MULTI)) ||
@@ -658,6 +661,7 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
prog ? : link->link.prog, cgrp))
return -ENOMEM;
+ WRITE_ONCE(cgrp->bpf.flags[atype], saved_flags);
if (pl) {
old_prog = pl->prog;
} else {
@@ -669,12 +673,12 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
return -ENOMEM;
}
if (hlist_empty(progs))
- hlist_add_head(&pl->node, progs);
+ hlist_add_head_rcu(&pl->node, progs);
else
- hlist_for_each(last, progs) {
+ __hlist_for_each_rcu(last, progs) {
if (last->next)
continue;
- hlist_add_behind(&pl->node, last);
+ hlist_add_behind_rcu(&pl->node, last);
break;
}
}
@@ -682,7 +686,6 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
pl->prog = prog;
pl->link = link;
bpf_cgroup_storages_assign(pl->storage, storage);
- cgrp->bpf.flags[atype] = saved_flags;
if (type == BPF_LSM_CGROUP) {
err = bpf_trampoline_link_cgroup_shim(new_prog, atype);
@@ -796,7 +799,7 @@ static int __cgroup_bpf_replace(struct cgroup *cgrp,
enum cgroup_bpf_attach_type atype;
struct bpf_prog *old_prog;
struct bpf_prog_list *pl;
- struct hlist_head *progs;
+ struct hlist_head __rcu *progs;
bool found = false;
atype = bpf_cgroup_atype_find(link->type, new_prog->aux->attach_btf_id);
@@ -808,7 +811,7 @@ static int __cgroup_bpf_replace(struct cgroup *cgrp,
if (link->link.prog->type != new_prog->type)
return -EINVAL;
- hlist_for_each_entry(pl, progs, node) {
+ hlist_for_each_entry_rcu(pl, progs, node) {
if (pl->link == link) {
found = true;
break;
@@ -847,7 +850,7 @@ static int cgroup_bpf_replace(struct bpf_link *link, struct bpf_prog *new_prog,
return ret;
}
-static struct bpf_prog_list *find_detach_entry(struct hlist_head *progs,
+static struct bpf_prog_list *find_detach_entry(struct hlist_head __rcu *progs,
struct bpf_prog *prog,
struct bpf_cgroup_link *link,
bool allow_multi)
@@ -862,7 +865,7 @@ static struct bpf_prog_list *find_detach_entry(struct hlist_head *progs,
/* to maintain backward compatibility NONE and OVERRIDE cgroups
* allow detaching with invalid FD (prog==NULL) in legacy mode
*/
- return hlist_entry(progs->first, typeof(*pl), node);
+ return hlist_entry(rcu_dereference_raw(progs)->first, typeof(*pl), node);
}
if (!prog && !link)
@@ -872,7 +875,7 @@ static struct bpf_prog_list *find_detach_entry(struct hlist_head *progs,
return ERR_PTR(-EINVAL);
/* find the prog or link and detach it */
- hlist_for_each_entry(pl, progs, node) {
+ hlist_for_each_entry_rcu(pl, progs, node) {
if (pl->prog == prog && pl->link == link)
return pl;
}
@@ -894,9 +897,9 @@ static void purge_effective_progs(struct cgroup *cgrp, struct bpf_prog *prog,
enum cgroup_bpf_attach_type atype)
{
struct cgroup_subsys_state *css;
+ struct hlist_head __rcu *head;
struct bpf_prog_array *progs;
struct bpf_prog_list *pl;
- struct hlist_head *head;
struct cgroup *cg;
int pos;
@@ -913,7 +916,7 @@ static void purge_effective_progs(struct cgroup *cgrp, struct bpf_prog *prog,
continue;
head = &cg->bpf.progs[atype];
- hlist_for_each_entry(pl, head, node) {
+ hlist_for_each_entry_rcu(pl, head, node) {
if (!prog_list_prog(pl))
continue;
if (pl->prog == prog && pl->link == link)
@@ -950,9 +953,9 @@ static int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog,
struct bpf_cgroup_link *link, enum bpf_attach_type type)
{
enum cgroup_bpf_attach_type atype;
+ struct hlist_head __rcu *progs;
struct bpf_prog *old_prog;
struct bpf_prog_list *pl;
- struct hlist_head *progs;
u32 attach_btf_id = 0;
u32 flags;
@@ -989,12 +992,12 @@ static int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog,
}
/* now can actually delete it from this cgroup list */
- hlist_del(&pl->node);
+ hlist_del_rcu(&pl->node);
kfree(pl);
if (hlist_empty(progs))
/* last program was detached, reset flags to zero */
- cgrp->bpf.flags[atype] = 0;
+ WRITE_ONCE(cgrp->bpf.flags[atype], 0);
if (old_prog) {
if (type == BPF_LSM_CGROUP)
bpf_trampoline_unlink_cgroup_shim(old_prog);
@@ -1022,10 +1025,10 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
__u32 __user *prog_attach_flags = u64_to_user_ptr(attr->query.prog_attach_flags);
bool effective_query = attr->query.query_flags & BPF_F_QUERY_EFFECTIVE;
__u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
+ struct bpf_prog_array *effective[MAX_CGROUP_BPF_ATTACH_TYPE];
enum bpf_attach_type type = attr->query.attach_type;
enum cgroup_bpf_attach_type from_atype, to_atype;
enum cgroup_bpf_attach_type atype;
- struct bpf_prog_array *effective;
int cnt, ret = 0, i;
int total_cnt = 0;
u32 flags;
@@ -1046,14 +1049,15 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
if (from_atype < 0)
return -EINVAL;
to_atype = from_atype;
- flags = cgrp->bpf.flags[from_atype];
+ flags = READ_ONCE(cgrp->bpf.flags[from_atype]);
}
for (atype = from_atype; atype <= to_atype; atype++) {
if (effective_query) {
- effective = rcu_dereference_protected(cgrp->bpf.effective[atype],
- lockdep_is_held(&cgroup_mutex));
- total_cnt += bpf_prog_array_length(effective);
+ effective[atype] = rcu_dereference_protected(
+ cgrp->bpf.effective[atype],
+ lockdep_is_held(&cgroup_mutex));
+ total_cnt += bpf_prog_array_length(effective[atype]);
} else {
total_cnt += prog_list_length(&cgrp->bpf.progs[atype]);
}
@@ -1076,12 +1080,10 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
for (atype = from_atype; atype <= to_atype && total_cnt; atype++) {
if (effective_query) {
- effective = rcu_dereference_protected(cgrp->bpf.effective[atype],
- lockdep_is_held(&cgroup_mutex));
- cnt = min_t(int, bpf_prog_array_length(effective), total_cnt);
- ret = bpf_prog_array_copy_to_user(effective, prog_ids, cnt);
+ cnt = min_t(int, bpf_prog_array_length(effective[atype]), total_cnt);
+ ret = bpf_prog_array_copy_to_user(effective[atype], prog_ids, cnt);
} else {
- struct hlist_head *progs;
+ struct hlist_head __rcu *progs;
struct bpf_prog_list *pl;
struct bpf_prog *prog;
u32 id;
@@ -1089,7 +1091,7 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
progs = &cgrp->bpf.progs[atype];
cnt = min_t(int, prog_list_length(progs), total_cnt);
i = 0;
- hlist_for_each_entry(pl, progs, node) {
+ hlist_for_each_entry_rcu(pl, progs, node) {
prog = prog_list_prog(pl);
id = prog->aux->id;
if (copy_to_user(prog_ids + i, &id, sizeof(id)))
@@ -1099,7 +1101,7 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
}
if (prog_attach_flags) {
- flags = cgrp->bpf.flags[atype];
+ flags = READ_ONCE(cgrp->bpf.flags[atype]);
for (i = 0; i < cnt; i++)
if (copy_to_user(prog_attach_flags + i,
@@ -1112,6 +1114,8 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
prog_ids += cnt;
total_cnt -= cnt;
}
+ if (total_cnt != 0)
+ return -EAGAIN; /* raced with the detach */
return ret;
}
@@ -1120,9 +1124,9 @@ static int cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
{
int ret;
- mutex_lock(&cgroup_mutex);
+ rcu_read_lock();
ret = __cgroup_bpf_query(cgrp, attr, uattr);
- mutex_unlock(&cgroup_mutex);
+ rcu_read_unlock();
return ret;
}
--
2.40.1.521.gf1e218fcd8-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH bpf-next v5 0/4] bpf: Don't EFAULT for {g,s}setsockopt with wrong optlen
2023-05-11 15:57 [PATCH bpf-next] RFC: bpf: query effective progs without cgroup_mutex Stanislav Fomichev
@ 2023-05-11 15:57 ` Stanislav Fomichev
2023-05-11 15:57 ` [PATCH bpf-next v5 1/4] " Stanislav Fomichev
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Stanislav Fomichev @ 2023-05-11 15:57 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
optval larger than PAGE_SIZE leads to EFAULT if the BPF program
isn't careful enough. This is often overlooked and might break
completely unrelated socket options. Instead of EFAULT,
let's ignore BPF program buffer changes. See the first patch for
more info.
In addition, clearly document this corner case and reset optlen
in our selftests (in case somebody copy-pastes from them).
v5:
- goto in the selftest (Martin)
- set IP_TOS to zero to avoid endianness complications (Martin)
v4:
- ignore retval as well when optlen > PAGE_SIZE (Martin)
v3:
- don't hard-code PAGE_SIZE (Martin)
- reset orig_optlen in getsockopt when kernel part succeeds (Martin)
Stanislav Fomichev (4):
bpf: Don't EFAULT for {g,s}setsockopt with wrong optlen
selftests/bpf: Update EFAULT {g,s}etsockopt selftests
selftests/bpf: Correctly handle optlen > 4096
bpf: Document EFAULT changes for sockopt
Documentation/bpf/prog_cgroup_sockopt.rst | 57 ++++++++-
kernel/bpf/cgroup.c | 15 +++
.../bpf/prog_tests/cgroup_getset_retval.c | 20 ++++
.../selftests/bpf/prog_tests/sockopt.c | 99 +++++++++++++++-
.../bpf/prog_tests/sockopt_inherit.c | 59 +++-------
.../selftests/bpf/prog_tests/sockopt_multi.c | 108 +++++-------------
.../bpf/prog_tests/sockopt_qos_to_cc.c | 2 +
.../progs/cgroup_getset_retval_getsockopt.c | 13 +++
.../progs/cgroup_getset_retval_setsockopt.c | 17 +++
.../selftests/bpf/progs/sockopt_inherit.c | 18 ++-
.../selftests/bpf/progs/sockopt_multi.c | 26 ++++-
.../selftests/bpf/progs/sockopt_qos_to_cc.c | 10 +-
.../testing/selftests/bpf/progs/sockopt_sk.c | 25 ++--
13 files changed, 330 insertions(+), 139 deletions(-)
--
2.40.1.521.gf1e218fcd8-goog
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH bpf-next v5 1/4] bpf: Don't EFAULT for {g,s}setsockopt with wrong optlen
2023-05-11 15:57 [PATCH bpf-next] RFC: bpf: query effective progs without cgroup_mutex Stanislav Fomichev
2023-05-11 15:57 ` [PATCH bpf-next v5 0/4] bpf: Don't EFAULT for {g,s}setsockopt with wrong optlen Stanislav Fomichev
@ 2023-05-11 15:57 ` Stanislav Fomichev
2023-05-11 15:57 ` [PATCH bpf-next v5 2/4] selftests/bpf: Update EFAULT {g,s}etsockopt selftests Stanislav Fomichev
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Stanislav Fomichev @ 2023-05-11 15:57 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, Martin KaFai Lau
With the way the hooks implemented right now, we have a special
condition: optval larger than PAGE_SIZE will expose only first 4k into
BPF; any modifications to the optval are ignored. If the BPF program
doesn't handle this condition by resetting optlen to 0,
the userspace will get EFAULT.
The intention of the EFAULT was to make it apparent to the
developers that the program is doing something wrong.
However, this inadvertently might affect production workloads
with the BPF programs that are not too careful (i.e., returning EFAULT
for perfectly valid setsockopt/getsockopt calls).
Let's try to minimize the chance of BPF program screwing up userspace
by ignoring the output of those BPF programs (instead of returning
EFAULT to the userspace). pr_info_once those cases to
the dmesg to help with figuring out what's going wrong.
Fixes: 0d01da6afc54 ("bpf: implement getsockopt and setsockopt hooks")
Suggested-by: Martin KaFai Lau <martin.lau@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
kernel/bpf/cgroup.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index a06e118a9be5..14c870595428 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -1826,6 +1826,12 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
ret = 1;
} else if (ctx.optlen > max_optlen || ctx.optlen < -1) {
/* optlen is out of bounds */
+ if (*optlen > PAGE_SIZE && ctx.optlen >= 0) {
+ pr_info_once("bpf setsockopt: ignoring program buffer with optlen=%d (max_optlen=%d)\n",
+ ctx.optlen, max_optlen);
+ ret = 0;
+ goto out;
+ }
ret = -EFAULT;
} else {
/* optlen within bounds, run kernel handler */
@@ -1881,8 +1887,10 @@ int __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level,
.optname = optname,
.current_task = current,
};
+ int orig_optlen;
int ret;
+ orig_optlen = max_optlen;
ctx.optlen = max_optlen;
max_optlen = sockopt_alloc_buf(&ctx, max_optlen, &buf);
if (max_optlen < 0)
@@ -1905,6 +1913,7 @@ int __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level,
ret = -EFAULT;
goto out;
}
+ orig_optlen = ctx.optlen;
if (copy_from_user(ctx.optval, optval,
min(ctx.optlen, max_optlen)) != 0) {
@@ -1922,6 +1931,12 @@ int __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level,
goto out;
if (optval && (ctx.optlen > max_optlen || ctx.optlen < 0)) {
+ if (orig_optlen > PAGE_SIZE && ctx.optlen >= 0) {
+ pr_info_once("bpf getsockopt: ignoring program buffer with optlen=%d (max_optlen=%d)\n",
+ ctx.optlen, max_optlen);
+ ret = retval;
+ goto out;
+ }
ret = -EFAULT;
goto out;
}
--
2.40.1.521.gf1e218fcd8-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH bpf-next v5 2/4] selftests/bpf: Update EFAULT {g,s}etsockopt selftests
2023-05-11 15:57 [PATCH bpf-next] RFC: bpf: query effective progs without cgroup_mutex Stanislav Fomichev
2023-05-11 15:57 ` [PATCH bpf-next v5 0/4] bpf: Don't EFAULT for {g,s}setsockopt with wrong optlen Stanislav Fomichev
2023-05-11 15:57 ` [PATCH bpf-next v5 1/4] " Stanislav Fomichev
@ 2023-05-11 15:57 ` Stanislav Fomichev
2023-05-11 15:57 ` [PATCH bpf-next v5 3/4] selftests/bpf: Correctly handle optlen > 4096 Stanislav Fomichev
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Stanislav Fomichev @ 2023-05-11 15:57 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
Instead of assuming EFAULT, let's assume the BPF program's
output is ignored.
Remove "getsockopt: deny arbitrary ctx->retval" because it
was actually testing optlen. We have separate set of tests
for retval.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
.../selftests/bpf/prog_tests/sockopt.c | 99 +++++++++++++++++--
1 file changed, 93 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c
index aa4debf62fc6..bd09f1a33341 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockopt.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c
@@ -5,10 +5,15 @@
static char bpf_log_buf[4096];
static bool verbose;
+#ifndef PAGE_SIZE
+#define PAGE_SIZE 4096
+#endif
+
enum sockopt_test_error {
OK = 0,
DENY_LOAD,
DENY_ATTACH,
+ EOPNOTSUPP_GETSOCKOPT,
EPERM_GETSOCKOPT,
EFAULT_GETSOCKOPT,
EPERM_SETSOCKOPT,
@@ -273,10 +278,31 @@ static struct sockopt_test {
.error = EFAULT_GETSOCKOPT,
},
{
- .descr = "getsockopt: deny arbitrary ctx->retval",
+ .descr = "getsockopt: ignore >PAGE_SIZE optlen",
.insns = {
- /* ctx->retval = 123 */
- BPF_MOV64_IMM(BPF_REG_0, 123),
+ /* write 0xFF to the first optval byte */
+
+ /* r6 = ctx->optval */
+ BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1,
+ offsetof(struct bpf_sockopt, optval)),
+ /* r2 = ctx->optval */
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_6),
+ /* r6 = ctx->optval + 1 */
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, 1),
+
+ /* r7 = ctx->optval_end */
+ BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_1,
+ offsetof(struct bpf_sockopt, optval_end)),
+
+ /* if (ctx->optval + 1 <= ctx->optval_end) { */
+ BPF_JMP_REG(BPF_JGT, BPF_REG_6, BPF_REG_7, 1),
+ /* ctx->optval[0] = 0xF0 */
+ BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0xFF),
+ /* } */
+
+ /* retval changes are ignored */
+ /* ctx->retval = 5 */
+ BPF_MOV64_IMM(BPF_REG_0, 5),
BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_0,
offsetof(struct bpf_sockopt, retval)),
@@ -287,9 +313,11 @@ static struct sockopt_test {
.attach_type = BPF_CGROUP_GETSOCKOPT,
.expected_attach_type = BPF_CGROUP_GETSOCKOPT,
- .get_optlen = 64,
-
- .error = EFAULT_GETSOCKOPT,
+ .get_level = 1234,
+ .get_optname = 5678,
+ .get_optval = {}, /* the changes are ignored */
+ .get_optlen = PAGE_SIZE + 1,
+ .error = EOPNOTSUPP_GETSOCKOPT,
},
{
.descr = "getsockopt: support smaller ctx->optlen",
@@ -648,6 +676,45 @@ static struct sockopt_test {
.error = EFAULT_SETSOCKOPT,
},
+ {
+ .descr = "setsockopt: ignore >PAGE_SIZE optlen",
+ .insns = {
+ /* write 0xFF to the first optval byte */
+
+ /* r6 = ctx->optval */
+ BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1,
+ offsetof(struct bpf_sockopt, optval)),
+ /* r2 = ctx->optval */
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_6),
+ /* r6 = ctx->optval + 1 */
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, 1),
+
+ /* r7 = ctx->optval_end */
+ BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_1,
+ offsetof(struct bpf_sockopt, optval_end)),
+
+ /* if (ctx->optval + 1 <= ctx->optval_end) { */
+ BPF_JMP_REG(BPF_JGT, BPF_REG_6, BPF_REG_7, 1),
+ /* ctx->optval[0] = 0xF0 */
+ BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0xF0),
+ /* } */
+
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ },
+ .attach_type = BPF_CGROUP_SETSOCKOPT,
+ .expected_attach_type = BPF_CGROUP_SETSOCKOPT,
+
+ .set_level = SOL_IP,
+ .set_optname = IP_TOS,
+ .set_optval = {},
+ .set_optlen = PAGE_SIZE + 1,
+
+ .get_level = SOL_IP,
+ .get_optname = IP_TOS,
+ .get_optval = {}, /* the changes are ignored */
+ .get_optlen = 4,
+ },
{
.descr = "setsockopt: allow changing ctx->optlen within bounds",
.insns = {
@@ -906,6 +973,13 @@ static int run_test(int cgroup_fd, struct sockopt_test *test)
}
if (test->set_optlen) {
+ if (test->set_optlen >= PAGE_SIZE) {
+ int num_pages = test->set_optlen / PAGE_SIZE;
+ int remainder = test->set_optlen % PAGE_SIZE;
+
+ test->set_optlen = num_pages * sysconf(_SC_PAGESIZE) + remainder;
+ }
+
err = setsockopt(sock_fd, test->set_level, test->set_optname,
test->set_optval, test->set_optlen);
if (err) {
@@ -921,7 +995,15 @@ static int run_test(int cgroup_fd, struct sockopt_test *test)
}
if (test->get_optlen) {
+ if (test->get_optlen >= PAGE_SIZE) {
+ int num_pages = test->get_optlen / PAGE_SIZE;
+ int remainder = test->get_optlen % PAGE_SIZE;
+
+ test->get_optlen = num_pages * sysconf(_SC_PAGESIZE) + remainder;
+ }
+
optval = malloc(test->get_optlen);
+ memset(optval, 0, test->get_optlen);
socklen_t optlen = test->get_optlen;
socklen_t expected_get_optlen = test->get_optlen_ret ?:
test->get_optlen;
@@ -929,6 +1011,8 @@ static int run_test(int cgroup_fd, struct sockopt_test *test)
err = getsockopt(sock_fd, test->get_level, test->get_optname,
optval, &optlen);
if (err) {
+ if (errno == EOPNOTSUPP && test->error == EOPNOTSUPP_GETSOCKOPT)
+ goto free_optval;
if (errno == EPERM && test->error == EPERM_GETSOCKOPT)
goto free_optval;
if (errno == EFAULT && test->error == EFAULT_GETSOCKOPT)
@@ -946,6 +1030,9 @@ static int run_test(int cgroup_fd, struct sockopt_test *test)
goto free_optval;
}
+ if (optlen > sizeof(test->get_optval))
+ optlen = sizeof(test->get_optval);
+
if (memcmp(optval, test->get_optval, optlen) != 0) {
errno = 0;
log_err("getsockopt returned unexpected optval");
--
2.40.1.521.gf1e218fcd8-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH bpf-next v5 3/4] selftests/bpf: Correctly handle optlen > 4096
2023-05-11 15:57 [PATCH bpf-next] RFC: bpf: query effective progs without cgroup_mutex Stanislav Fomichev
` (2 preceding siblings ...)
2023-05-11 15:57 ` [PATCH bpf-next v5 2/4] selftests/bpf: Update EFAULT {g,s}etsockopt selftests Stanislav Fomichev
@ 2023-05-11 15:57 ` Stanislav Fomichev
2023-05-11 15:57 ` [PATCH bpf-next v5 4/4] bpf: Document EFAULT changes for sockopt Stanislav Fomichev
2023-05-11 15:59 ` [PATCH bpf-next] RFC: bpf: query effective progs without cgroup_mutex Stanislav Fomichev
5 siblings, 0 replies; 10+ messages in thread
From: Stanislav Fomichev @ 2023-05-11 15:57 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
Even though it's not relevant in selftests, the people
might still copy-paste from them. So let's take care
of optlen > 4096 cases explicitly.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
.../bpf/prog_tests/cgroup_getset_retval.c | 20 ++++
.../bpf/prog_tests/sockopt_inherit.c | 59 +++-------
.../selftests/bpf/prog_tests/sockopt_multi.c | 108 +++++-------------
.../bpf/prog_tests/sockopt_qos_to_cc.c | 2 +
.../progs/cgroup_getset_retval_getsockopt.c | 13 +++
.../progs/cgroup_getset_retval_setsockopt.c | 17 +++
.../selftests/bpf/progs/sockopt_inherit.c | 18 ++-
.../selftests/bpf/progs/sockopt_multi.c | 26 ++++-
.../selftests/bpf/progs/sockopt_qos_to_cc.c | 10 +-
.../testing/selftests/bpf/progs/sockopt_sk.c | 25 ++--
10 files changed, 166 insertions(+), 132 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_getset_retval.c b/tools/testing/selftests/bpf/prog_tests/cgroup_getset_retval.c
index 4d2fa99273d8..2bb5773d6f99 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgroup_getset_retval.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_getset_retval.c
@@ -25,6 +25,8 @@ static void test_setsockopt_set(int cgroup_fd, int sock_fd)
if (!ASSERT_OK_PTR(obj, "skel-load"))
return;
+ obj->bss->page_size = sysconf(_SC_PAGESIZE);
+
/* Attach setsockopt that sets EUNATCH, assert that
* we actually get that error when we run setsockopt()
*/
@@ -59,6 +61,8 @@ static void test_setsockopt_set_and_get(int cgroup_fd, int sock_fd)
if (!ASSERT_OK_PTR(obj, "skel-load"))
return;
+ obj->bss->page_size = sysconf(_SC_PAGESIZE);
+
/* Attach setsockopt that sets EUNATCH, and one that gets the
* previously set errno. Assert that we get the same errno back.
*/
@@ -100,6 +104,8 @@ static void test_setsockopt_default_zero(int cgroup_fd, int sock_fd)
if (!ASSERT_OK_PTR(obj, "skel-load"))
return;
+ obj->bss->page_size = sysconf(_SC_PAGESIZE);
+
/* Attach setsockopt that gets the previously set errno.
* Assert that, without anything setting one, we get 0.
*/
@@ -134,6 +140,8 @@ static void test_setsockopt_default_zero_and_set(int cgroup_fd, int sock_fd)
if (!ASSERT_OK_PTR(obj, "skel-load"))
return;
+ obj->bss->page_size = sysconf(_SC_PAGESIZE);
+
/* Attach setsockopt that gets the previously set errno, and then
* one that sets the errno to EUNATCH. Assert that the get does not
* see EUNATCH set later, and does not prevent EUNATCH from being set.
@@ -177,6 +185,8 @@ static void test_setsockopt_override(int cgroup_fd, int sock_fd)
if (!ASSERT_OK_PTR(obj, "skel-load"))
return;
+ obj->bss->page_size = sysconf(_SC_PAGESIZE);
+
/* Attach setsockopt that sets EUNATCH, then one that sets EISCONN,
* and then one that gets the exported errno. Assert both the syscall
* and the helper sees the last set errno.
@@ -224,6 +234,8 @@ static void test_setsockopt_legacy_eperm(int cgroup_fd, int sock_fd)
if (!ASSERT_OK_PTR(obj, "skel-load"))
return;
+ obj->bss->page_size = sysconf(_SC_PAGESIZE);
+
/* Attach setsockopt that return a reject without setting errno
* (legacy reject), and one that gets the errno. Assert that for
* backward compatibility the syscall result in EPERM, and this
@@ -268,6 +280,8 @@ static void test_setsockopt_legacy_no_override(int cgroup_fd, int sock_fd)
if (!ASSERT_OK_PTR(obj, "skel-load"))
return;
+ obj->bss->page_size = sysconf(_SC_PAGESIZE);
+
/* Attach setsockopt that sets EUNATCH, then one that return a reject
* without setting errno, and then one that gets the exported errno.
* Assert both the syscall and the helper's errno are unaffected by
@@ -319,6 +333,8 @@ static void test_getsockopt_get(int cgroup_fd, int sock_fd)
if (!ASSERT_OK_PTR(obj, "skel-load"))
return;
+ obj->bss->page_size = sysconf(_SC_PAGESIZE);
+
/* Attach getsockopt that gets previously set errno. Assert that the
* error from kernel is in both ctx_retval_value and retval_value.
*/
@@ -359,6 +375,8 @@ static void test_getsockopt_override(int cgroup_fd, int sock_fd)
if (!ASSERT_OK_PTR(obj, "skel-load"))
return;
+ obj->bss->page_size = sysconf(_SC_PAGESIZE);
+
/* Attach getsockopt that sets retval to -EISCONN. Assert that this
* overrides the value from kernel.
*/
@@ -396,6 +414,8 @@ static void test_getsockopt_retval_sync(int cgroup_fd, int sock_fd)
if (!ASSERT_OK_PTR(obj, "skel-load"))
return;
+ obj->bss->page_size = sysconf(_SC_PAGESIZE);
+
/* Attach getsockopt that sets retval to -EISCONN, and one that clears
* ctx retval. Assert that the clearing ctx retval is synced to helper
* and clears any errors both from kernel and BPF..
diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt_inherit.c b/tools/testing/selftests/bpf/prog_tests/sockopt_inherit.c
index 60c17a8e2789..917f486db826 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockopt_inherit.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockopt_inherit.c
@@ -2,6 +2,8 @@
#include <test_progs.h>
#include "cgroup_helpers.h"
+#include "sockopt_inherit.skel.h"
+
#define SOL_CUSTOM 0xdeadbeef
#define CUSTOM_INHERIT1 0
#define CUSTOM_INHERIT2 1
@@ -132,58 +134,30 @@ static int start_server(void)
return fd;
}
-static int prog_attach(struct bpf_object *obj, int cgroup_fd, const char *title,
- const char *prog_name)
-{
- enum bpf_attach_type attach_type;
- enum bpf_prog_type prog_type;
- struct bpf_program *prog;
- int err;
-
- err = libbpf_prog_type_by_name(title, &prog_type, &attach_type);
- if (err) {
- log_err("Failed to deduct types for %s BPF program", prog_name);
- return -1;
- }
-
- prog = bpf_object__find_program_by_name(obj, prog_name);
- if (!prog) {
- log_err("Failed to find %s BPF program", prog_name);
- return -1;
- }
-
- err = bpf_prog_attach(bpf_program__fd(prog), cgroup_fd,
- attach_type, 0);
- if (err) {
- log_err("Failed to attach %s BPF program", prog_name);
- return -1;
- }
-
- return 0;
-}
-
static void run_test(int cgroup_fd)
{
+ struct bpf_link *link_getsockopt = NULL;
+ struct bpf_link *link_setsockopt = NULL;
int server_fd = -1, client_fd;
- struct bpf_object *obj;
+ struct sockopt_inherit *obj;
void *server_err;
pthread_t tid;
int err;
- obj = bpf_object__open_file("sockopt_inherit.bpf.o", NULL);
- if (!ASSERT_OK_PTR(obj, "obj_open"))
+ obj = sockopt_inherit__open_and_load();
+ if (!ASSERT_OK_PTR(obj, "skel-load"))
return;
- err = bpf_object__load(obj);
- if (!ASSERT_OK(err, "obj_load"))
- goto close_bpf_object;
+ obj->bss->page_size = sysconf(_SC_PAGESIZE);
- err = prog_attach(obj, cgroup_fd, "cgroup/getsockopt", "_getsockopt");
- if (!ASSERT_OK(err, "prog_attach _getsockopt"))
+ link_getsockopt = bpf_program__attach_cgroup(obj->progs._getsockopt,
+ cgroup_fd);
+ if (!ASSERT_OK_PTR(link_getsockopt, "cg-attach-getsockopt"))
goto close_bpf_object;
- err = prog_attach(obj, cgroup_fd, "cgroup/setsockopt", "_setsockopt");
- if (!ASSERT_OK(err, "prog_attach _setsockopt"))
+ link_setsockopt = bpf_program__attach_cgroup(obj->progs._setsockopt,
+ cgroup_fd);
+ if (!ASSERT_OK_PTR(link_setsockopt, "cg-attach-setsockopt"))
goto close_bpf_object;
server_fd = start_server();
@@ -217,7 +191,10 @@ static void run_test(int cgroup_fd)
close_server_fd:
close(server_fd);
close_bpf_object:
- bpf_object__close(obj);
+ bpf_link__destroy(link_getsockopt);
+ bpf_link__destroy(link_setsockopt);
+
+ sockopt_inherit__destroy(obj);
}
void test_sockopt_inherit(void)
diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt_multi.c b/tools/testing/selftests/bpf/prog_tests/sockopt_multi.c
index 7f5659349011..759bbb6f8c5f 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockopt_multi.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockopt_multi.c
@@ -2,61 +2,13 @@
#include <test_progs.h>
#include "cgroup_helpers.h"
-static int prog_attach(struct bpf_object *obj, int cgroup_fd, const char *title, const char *name)
-{
- enum bpf_attach_type attach_type;
- enum bpf_prog_type prog_type;
- struct bpf_program *prog;
- int err;
-
- err = libbpf_prog_type_by_name(title, &prog_type, &attach_type);
- if (err) {
- log_err("Failed to deduct types for %s BPF program", title);
- return -1;
- }
-
- prog = bpf_object__find_program_by_name(obj, name);
- if (!prog) {
- log_err("Failed to find %s BPF program", name);
- return -1;
- }
-
- err = bpf_prog_attach(bpf_program__fd(prog), cgroup_fd,
- attach_type, BPF_F_ALLOW_MULTI);
- if (err) {
- log_err("Failed to attach %s BPF program", name);
- return -1;
- }
-
- return 0;
-}
+#include "sockopt_multi.skel.h"
-static int prog_detach(struct bpf_object *obj, int cgroup_fd, const char *title, const char *name)
-{
- enum bpf_attach_type attach_type;
- enum bpf_prog_type prog_type;
- struct bpf_program *prog;
- int err;
-
- err = libbpf_prog_type_by_name(title, &prog_type, &attach_type);
- if (err)
- return -1;
-
- prog = bpf_object__find_program_by_name(obj, name);
- if (!prog)
- return -1;
-
- err = bpf_prog_detach2(bpf_program__fd(prog), cgroup_fd,
- attach_type);
- if (err)
- return -1;
-
- return 0;
-}
-
-static int run_getsockopt_test(struct bpf_object *obj, int cg_parent,
+static int run_getsockopt_test(struct sockopt_multi *obj, int cg_parent,
int cg_child, int sock_fd)
{
+ struct bpf_link *link_parent = NULL;
+ struct bpf_link *link_child = NULL;
socklen_t optlen;
__u8 buf;
int err;
@@ -89,8 +41,9 @@ static int run_getsockopt_test(struct bpf_object *obj, int cg_parent,
* - child: 0x80 -> 0x90
*/
- err = prog_attach(obj, cg_child, "cgroup/getsockopt", "_getsockopt_child");
- if (err)
+ link_child = bpf_program__attach_cgroup(obj->progs._getsockopt_child,
+ cg_child);
+ if (!ASSERT_OK_PTR(link_child, "cg-attach-getsockopt_child"))
goto detach;
buf = 0x00;
@@ -113,8 +66,9 @@ static int run_getsockopt_test(struct bpf_object *obj, int cg_parent,
* - parent: 0x90 -> 0xA0
*/
- err = prog_attach(obj, cg_parent, "cgroup/getsockopt", "_getsockopt_parent");
- if (err)
+ link_parent = bpf_program__attach_cgroup(obj->progs._getsockopt_parent,
+ cg_parent);
+ if (!ASSERT_OK_PTR(link_parent, "cg-attach-getsockopt_parent"))
goto detach;
buf = 0x00;
@@ -157,11 +111,8 @@ static int run_getsockopt_test(struct bpf_object *obj, int cg_parent,
* - parent: unexpected 0x40, EPERM
*/
- err = prog_detach(obj, cg_child, "cgroup/getsockopt", "_getsockopt_child");
- if (err) {
- log_err("Failed to detach child program");
- goto detach;
- }
+ bpf_link__destroy(link_child);
+ link_child = NULL;
buf = 0x00;
optlen = 1;
@@ -198,15 +149,17 @@ static int run_getsockopt_test(struct bpf_object *obj, int cg_parent,
}
detach:
- prog_detach(obj, cg_child, "cgroup/getsockopt", "_getsockopt_child");
- prog_detach(obj, cg_parent, "cgroup/getsockopt", "_getsockopt_parent");
+ bpf_link__destroy(link_child);
+ bpf_link__destroy(link_parent);
return err;
}
-static int run_setsockopt_test(struct bpf_object *obj, int cg_parent,
+static int run_setsockopt_test(struct sockopt_multi *obj, int cg_parent,
int cg_child, int sock_fd)
{
+ struct bpf_link *link_parent = NULL;
+ struct bpf_link *link_child = NULL;
socklen_t optlen;
__u8 buf;
int err;
@@ -236,8 +189,9 @@ static int run_setsockopt_test(struct bpf_object *obj, int cg_parent,
/* Attach child program and make sure it adds 0x10. */
- err = prog_attach(obj, cg_child, "cgroup/setsockopt", "_setsockopt");
- if (err)
+ link_child = bpf_program__attach_cgroup(obj->progs._setsockopt,
+ cg_child);
+ if (!ASSERT_OK_PTR(link_child, "cg-attach-setsockopt_child"))
goto detach;
buf = 0x80;
@@ -263,8 +217,9 @@ static int run_setsockopt_test(struct bpf_object *obj, int cg_parent,
/* Attach parent program and make sure it adds another 0x10. */
- err = prog_attach(obj, cg_parent, "cgroup/setsockopt", "_setsockopt");
- if (err)
+ link_parent = bpf_program__attach_cgroup(obj->progs._setsockopt,
+ cg_parent);
+ if (!ASSERT_OK_PTR(link_parent, "cg-attach-setsockopt_parent"))
goto detach;
buf = 0x80;
@@ -289,8 +244,8 @@ static int run_setsockopt_test(struct bpf_object *obj, int cg_parent,
}
detach:
- prog_detach(obj, cg_child, "cgroup/setsockopt", "_setsockopt");
- prog_detach(obj, cg_parent, "cgroup/setsockopt", "_setsockopt");
+ bpf_link__destroy(link_child);
+ bpf_link__destroy(link_parent);
return err;
}
@@ -298,9 +253,8 @@ static int run_setsockopt_test(struct bpf_object *obj, int cg_parent,
void test_sockopt_multi(void)
{
int cg_parent = -1, cg_child = -1;
- struct bpf_object *obj = NULL;
+ struct sockopt_multi *obj = NULL;
int sock_fd = -1;
- int err = -1;
cg_parent = test__join_cgroup("/parent");
if (!ASSERT_GE(cg_parent, 0, "join_cgroup /parent"))
@@ -310,13 +264,11 @@ void test_sockopt_multi(void)
if (!ASSERT_GE(cg_child, 0, "join_cgroup /parent/child"))
goto out;
- obj = bpf_object__open_file("sockopt_multi.bpf.o", NULL);
- if (!ASSERT_OK_PTR(obj, "obj_load"))
+ obj = sockopt_multi__open_and_load();
+ if (!ASSERT_OK_PTR(obj, "skel-load"))
goto out;
- err = bpf_object__load(obj);
- if (!ASSERT_OK(err, "obj_load"))
- goto out;
+ obj->bss->page_size = sysconf(_SC_PAGESIZE);
sock_fd = socket(AF_INET, SOCK_STREAM, 0);
if (!ASSERT_GE(sock_fd, 0, "socket"))
@@ -327,7 +279,7 @@ void test_sockopt_multi(void)
out:
close(sock_fd);
- bpf_object__close(obj);
+ sockopt_multi__destroy(obj);
close(cg_child);
close(cg_parent);
}
diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt_qos_to_cc.c b/tools/testing/selftests/bpf/prog_tests/sockopt_qos_to_cc.c
index 6b53b3cb8dad..6b2d300e9fd4 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockopt_qos_to_cc.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockopt_qos_to_cc.c
@@ -42,6 +42,8 @@ void test_sockopt_qos_to_cc(void)
if (!ASSERT_OK_PTR(skel, "skel"))
goto done;
+ skel->bss->page_size = sysconf(_SC_PAGESIZE);
+
sock_fd = socket(AF_INET6, SOCK_STREAM, 0);
if (!ASSERT_GE(sock_fd, 0, "v6 socket open"))
goto done;
diff --git a/tools/testing/selftests/bpf/progs/cgroup_getset_retval_getsockopt.c b/tools/testing/selftests/bpf/progs/cgroup_getset_retval_getsockopt.c
index b2a409e6382a..932b8ecd4ae3 100644
--- a/tools/testing/selftests/bpf/progs/cgroup_getset_retval_getsockopt.c
+++ b/tools/testing/selftests/bpf/progs/cgroup_getset_retval_getsockopt.c
@@ -12,6 +12,7 @@ __u32 invocations = 0;
__u32 assertion_error = 0;
__u32 retval_value = 0;
__u32 ctx_retval_value = 0;
+__u32 page_size = 0;
SEC("cgroup/getsockopt")
int get_retval(struct bpf_sockopt *ctx)
@@ -20,6 +21,10 @@ int get_retval(struct bpf_sockopt *ctx)
ctx_retval_value = ctx->retval;
__sync_fetch_and_add(&invocations, 1);
+ /* optval larger than PAGE_SIZE use kernel's buffer. */
+ if (ctx->optlen > page_size)
+ ctx->optlen = 0;
+
return 1;
}
@@ -31,6 +36,10 @@ int set_eisconn(struct bpf_sockopt *ctx)
if (bpf_set_retval(-EISCONN))
assertion_error = 1;
+ /* optval larger than PAGE_SIZE use kernel's buffer. */
+ if (ctx->optlen > page_size)
+ ctx->optlen = 0;
+
return 1;
}
@@ -41,5 +50,9 @@ int clear_retval(struct bpf_sockopt *ctx)
ctx->retval = 0;
+ /* optval larger than PAGE_SIZE use kernel's buffer. */
+ if (ctx->optlen > page_size)
+ ctx->optlen = 0;
+
return 1;
}
diff --git a/tools/testing/selftests/bpf/progs/cgroup_getset_retval_setsockopt.c b/tools/testing/selftests/bpf/progs/cgroup_getset_retval_setsockopt.c
index d6e5903e06ba..b7fa8804e19d 100644
--- a/tools/testing/selftests/bpf/progs/cgroup_getset_retval_setsockopt.c
+++ b/tools/testing/selftests/bpf/progs/cgroup_getset_retval_setsockopt.c
@@ -11,6 +11,7 @@
__u32 invocations = 0;
__u32 assertion_error = 0;
__u32 retval_value = 0;
+__u32 page_size = 0;
SEC("cgroup/setsockopt")
int get_retval(struct bpf_sockopt *ctx)
@@ -18,6 +19,10 @@ int get_retval(struct bpf_sockopt *ctx)
retval_value = bpf_get_retval();
__sync_fetch_and_add(&invocations, 1);
+ /* optval larger than PAGE_SIZE use kernel's buffer. */
+ if (ctx->optlen > page_size)
+ ctx->optlen = 0;
+
return 1;
}
@@ -29,6 +34,10 @@ int set_eunatch(struct bpf_sockopt *ctx)
if (bpf_set_retval(-EUNATCH))
assertion_error = 1;
+ /* optval larger than PAGE_SIZE use kernel's buffer. */
+ if (ctx->optlen > page_size)
+ ctx->optlen = 0;
+
return 0;
}
@@ -40,6 +49,10 @@ int set_eisconn(struct bpf_sockopt *ctx)
if (bpf_set_retval(-EISCONN))
assertion_error = 1;
+ /* optval larger than PAGE_SIZE use kernel's buffer. */
+ if (ctx->optlen > page_size)
+ ctx->optlen = 0;
+
return 0;
}
@@ -48,5 +61,9 @@ int legacy_eperm(struct bpf_sockopt *ctx)
{
__sync_fetch_and_add(&invocations, 1);
+ /* optval larger than PAGE_SIZE use kernel's buffer. */
+ if (ctx->optlen > page_size)
+ ctx->optlen = 0;
+
return 0;
}
diff --git a/tools/testing/selftests/bpf/progs/sockopt_inherit.c b/tools/testing/selftests/bpf/progs/sockopt_inherit.c
index 9fb241b97291..c8f59caa4639 100644
--- a/tools/testing/selftests/bpf/progs/sockopt_inherit.c
+++ b/tools/testing/selftests/bpf/progs/sockopt_inherit.c
@@ -9,6 +9,8 @@ char _license[] SEC("license") = "GPL";
#define CUSTOM_INHERIT2 1
#define CUSTOM_LISTENER 2
+__u32 page_size = 0;
+
struct sockopt_inherit {
__u8 val;
};
@@ -55,7 +57,7 @@ int _getsockopt(struct bpf_sockopt *ctx)
__u8 *optval = ctx->optval;
if (ctx->level != SOL_CUSTOM)
- return 1; /* only interested in SOL_CUSTOM */
+ goto out; /* only interested in SOL_CUSTOM */
if (optval + 1 > optval_end)
return 0; /* EPERM, bounds check */
@@ -70,6 +72,12 @@ int _getsockopt(struct bpf_sockopt *ctx)
ctx->optlen = 1;
return 1;
+
+out:
+ /* optval larger than PAGE_SIZE use kernel's buffer. */
+ if (ctx->optlen > page_size)
+ ctx->optlen = 0;
+ return 1;
}
SEC("cgroup/setsockopt")
@@ -80,7 +88,7 @@ int _setsockopt(struct bpf_sockopt *ctx)
__u8 *optval = ctx->optval;
if (ctx->level != SOL_CUSTOM)
- return 1; /* only interested in SOL_CUSTOM */
+ goto out; /* only interested in SOL_CUSTOM */
if (optval + 1 > optval_end)
return 0; /* EPERM, bounds check */
@@ -93,4 +101,10 @@ int _setsockopt(struct bpf_sockopt *ctx)
ctx->optlen = -1;
return 1;
+
+out:
+ /* optval larger than PAGE_SIZE use kernel's buffer. */
+ if (ctx->optlen > page_size)
+ ctx->optlen = 0;
+ return 1;
}
diff --git a/tools/testing/selftests/bpf/progs/sockopt_multi.c b/tools/testing/selftests/bpf/progs/sockopt_multi.c
index 177a59069dae..96f29fce050b 100644
--- a/tools/testing/selftests/bpf/progs/sockopt_multi.c
+++ b/tools/testing/selftests/bpf/progs/sockopt_multi.c
@@ -5,6 +5,8 @@
char _license[] SEC("license") = "GPL";
+__u32 page_size = 0;
+
SEC("cgroup/getsockopt")
int _getsockopt_child(struct bpf_sockopt *ctx)
{
@@ -12,7 +14,7 @@ int _getsockopt_child(struct bpf_sockopt *ctx)
__u8 *optval = ctx->optval;
if (ctx->level != SOL_IP || ctx->optname != IP_TOS)
- return 1;
+ goto out;
if (optval + 1 > optval_end)
return 0; /* EPERM, bounds check */
@@ -26,6 +28,12 @@ int _getsockopt_child(struct bpf_sockopt *ctx)
ctx->optlen = 1;
return 1;
+
+out:
+ /* optval larger than PAGE_SIZE use kernel's buffer. */
+ if (ctx->optlen > page_size)
+ ctx->optlen = 0;
+ return 1;
}
SEC("cgroup/getsockopt")
@@ -35,7 +43,7 @@ int _getsockopt_parent(struct bpf_sockopt *ctx)
__u8 *optval = ctx->optval;
if (ctx->level != SOL_IP || ctx->optname != IP_TOS)
- return 1;
+ goto out;
if (optval + 1 > optval_end)
return 0; /* EPERM, bounds check */
@@ -49,6 +57,12 @@ int _getsockopt_parent(struct bpf_sockopt *ctx)
ctx->optlen = 1;
return 1;
+
+out:
+ /* optval larger than PAGE_SIZE use kernel's buffer. */
+ if (ctx->optlen > page_size)
+ ctx->optlen = 0;
+ return 1;
}
SEC("cgroup/setsockopt")
@@ -58,7 +72,7 @@ int _setsockopt(struct bpf_sockopt *ctx)
__u8 *optval = ctx->optval;
if (ctx->level != SOL_IP || ctx->optname != IP_TOS)
- return 1;
+ goto out;
if (optval + 1 > optval_end)
return 0; /* EPERM, bounds check */
@@ -67,4 +81,10 @@ int _setsockopt(struct bpf_sockopt *ctx)
ctx->optlen = 1;
return 1;
+
+out:
+ /* optval larger than PAGE_SIZE use kernel's buffer. */
+ if (ctx->optlen > page_size)
+ ctx->optlen = 0;
+ return 1;
}
diff --git a/tools/testing/selftests/bpf/progs/sockopt_qos_to_cc.c b/tools/testing/selftests/bpf/progs/sockopt_qos_to_cc.c
index 1bce83b6e3a7..dbe235ede7f3 100644
--- a/tools/testing/selftests/bpf/progs/sockopt_qos_to_cc.c
+++ b/tools/testing/selftests/bpf/progs/sockopt_qos_to_cc.c
@@ -9,6 +9,8 @@
char _license[] SEC("license") = "GPL";
+__u32 page_size = 0;
+
SEC("cgroup/setsockopt")
int sockopt_qos_to_cc(struct bpf_sockopt *ctx)
{
@@ -19,7 +21,7 @@ int sockopt_qos_to_cc(struct bpf_sockopt *ctx)
char cc_cubic[TCP_CA_NAME_MAX] = "cubic";
if (ctx->level != SOL_IPV6 || ctx->optname != IPV6_TCLASS)
- return 1;
+ goto out;
if (optval + 1 > optval_end)
return 0; /* EPERM, bounds check */
@@ -36,4 +38,10 @@ int sockopt_qos_to_cc(struct bpf_sockopt *ctx)
return 0;
}
return 1;
+
+out:
+ /* optval larger than PAGE_SIZE use kernel's buffer. */
+ if (ctx->optlen > page_size)
+ ctx->optlen = 0;
+ return 1;
}
diff --git a/tools/testing/selftests/bpf/progs/sockopt_sk.c b/tools/testing/selftests/bpf/progs/sockopt_sk.c
index fe1df4cd206e..cb990a7d3d45 100644
--- a/tools/testing/selftests/bpf/progs/sockopt_sk.c
+++ b/tools/testing/selftests/bpf/progs/sockopt_sk.c
@@ -37,7 +37,7 @@ int _getsockopt(struct bpf_sockopt *ctx)
/* Bypass AF_NETLINK. */
sk = ctx->sk;
if (sk && sk->family == AF_NETLINK)
- return 1;
+ goto out;
/* Make sure bpf_get_netns_cookie is callable.
*/
@@ -52,8 +52,7 @@ int _getsockopt(struct bpf_sockopt *ctx)
* let next BPF program in the cgroup chain or kernel
* handle it.
*/
- ctx->optlen = 0; /* bypass optval>PAGE_SIZE */
- return 1;
+ goto out;
}
if (ctx->level == SOL_SOCKET && ctx->optname == SO_SNDBUF) {
@@ -61,7 +60,7 @@ int _getsockopt(struct bpf_sockopt *ctx)
* let next BPF program in the cgroup chain or kernel
* handle it.
*/
- return 1;
+ goto out;
}
if (ctx->level == SOL_TCP && ctx->optname == TCP_CONGESTION) {
@@ -69,7 +68,7 @@ int _getsockopt(struct bpf_sockopt *ctx)
* let next BPF program in the cgroup chain or kernel
* handle it.
*/
- return 1;
+ goto out;
}
if (ctx->level == SOL_TCP && ctx->optname == TCP_ZEROCOPY_RECEIVE) {
@@ -85,7 +84,7 @@ int _getsockopt(struct bpf_sockopt *ctx)
if (((struct tcp_zerocopy_receive *)optval)->address != 0)
return 0; /* unexpected data */
- return 1;
+ goto out;
}
if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) {
@@ -129,6 +128,12 @@ int _getsockopt(struct bpf_sockopt *ctx)
ctx->optlen = 1;
return 1;
+
+out:
+ /* optval larger than PAGE_SIZE use kernel's buffer. */
+ if (ctx->optlen > page_size)
+ ctx->optlen = 0;
+ return 1;
}
SEC("cgroup/setsockopt")
@@ -142,7 +147,7 @@ int _setsockopt(struct bpf_sockopt *ctx)
/* Bypass AF_NETLINK. */
sk = ctx->sk;
if (sk && sk->family == AF_NETLINK)
- return 1;
+ goto out;
/* Make sure bpf_get_netns_cookie is callable.
*/
@@ -224,4 +229,10 @@ int _setsockopt(struct bpf_sockopt *ctx)
*/
return 1;
+
+out:
+ /* optval larger than PAGE_SIZE use kernel's buffer. */
+ if (ctx->optlen > page_size)
+ ctx->optlen = 0;
+ return 1;
}
--
2.40.1.521.gf1e218fcd8-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH bpf-next v5 4/4] bpf: Document EFAULT changes for sockopt
2023-05-11 15:57 [PATCH bpf-next] RFC: bpf: query effective progs without cgroup_mutex Stanislav Fomichev
` (3 preceding siblings ...)
2023-05-11 15:57 ` [PATCH bpf-next v5 3/4] selftests/bpf: Correctly handle optlen > 4096 Stanislav Fomichev
@ 2023-05-11 15:57 ` Stanislav Fomichev
2023-05-11 15:59 ` [PATCH bpf-next] RFC: bpf: query effective progs without cgroup_mutex Stanislav Fomichev
5 siblings, 0 replies; 10+ messages in thread
From: Stanislav Fomichev @ 2023-05-11 15:57 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
And add examples for how to correctly handle large optlens.
This is less relevant now when we don't EFAULT anymore, but
that's still the correct thing to do.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
Documentation/bpf/prog_cgroup_sockopt.rst | 57 ++++++++++++++++++++++-
1 file changed, 56 insertions(+), 1 deletion(-)
diff --git a/Documentation/bpf/prog_cgroup_sockopt.rst b/Documentation/bpf/prog_cgroup_sockopt.rst
index 172f957204bf..1226a94af07a 100644
--- a/Documentation/bpf/prog_cgroup_sockopt.rst
+++ b/Documentation/bpf/prog_cgroup_sockopt.rst
@@ -98,10 +98,65 @@ When the ``optval`` is greater than the ``PAGE_SIZE``, the BPF program
indicates that the kernel should use BPF's trimmed ``optval``.
When the BPF program returns with the ``optlen`` greater than
-``PAGE_SIZE``, the userspace will receive ``EFAULT`` errno.
+``PAGE_SIZE``, the userspace will receive original kernel
+buffers without any modifications that the BPF program might have
+applied.
Example
=======
+Recommended way to handle BPF programs is as follows:
+
+.. code-block:: c
+
+ SEC("cgroup/getsockopt")
+ int getsockopt(struct bpf_sockopt *ctx)
+ {
+ /* Custom socket option. */
+ if (ctx->level == MY_SOL && ctx->optname == MY_OPTNAME) {
+ ctx->retval = 0;
+ optval[0] = ...;
+ ctx->optlen = 1;
+ return 1;
+ }
+
+ /* Modify kernel's socket option. */
+ if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) {
+ ctx->retval = 0;
+ optval[0] = ...;
+ ctx->optlen = 1;
+ return 1;
+ }
+
+ /* optval larger than PAGE_SIZE use kernel's buffer. */
+ if (ctx->optlen > PAGE_SIZE)
+ ctx->optlen = 0;
+
+ return 1;
+ }
+
+ SEC("cgroup/setsockopt")
+ int setsockopt(struct bpf_sockopt *ctx)
+ {
+ /* Custom socket option. */
+ if (ctx->level == MY_SOL && ctx->optname == MY_OPTNAME) {
+ /* do something */
+ ctx->optlen = -1;
+ return 1;
+ }
+
+ /* Modify kernel's socket option. */
+ if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) {
+ optval[0] = ...;
+ return 1;
+ }
+
+ /* optval larger than PAGE_SIZE use kernel's buffer. */
+ if (ctx->optlen > PAGE_SIZE)
+ ctx->optlen = 0;
+
+ return 1;
+ }
+
See ``tools/testing/selftests/bpf/progs/sockopt_sk.c`` for an example
of BPF program that handles socket options.
--
2.40.1.521.gf1e218fcd8-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next] RFC: bpf: query effective progs without cgroup_mutex
2023-05-11 15:57 [PATCH bpf-next] RFC: bpf: query effective progs without cgroup_mutex Stanislav Fomichev
` (4 preceding siblings ...)
2023-05-11 15:57 ` [PATCH bpf-next v5 4/4] bpf: Document EFAULT changes for sockopt Stanislav Fomichev
@ 2023-05-11 15:59 ` Stanislav Fomichev
5 siblings, 0 replies; 10+ messages in thread
From: Stanislav Fomichev @ 2023-05-11 15:59 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, haoluo, jolsa
On Thu, May 11, 2023 at 8:57 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> We're observing some stalls on the heavily loaded machines
> in the cgroup_bpf_prog_query path. This is likely due to
> being blocked on cgroup_mutex.
>
> IIUC, the cgroup_mutex is there mostly to protect the non-effective
> fields (cgrp->bpf.progs) which might be changed by the update path.
> For the BPF_F_QUERY_EFFECTIVE case, all we need is to rcu_dereference
> a bunch of pointers (and keep them around for consistency), so
> let's do it.
>
> Sending out as an RFC because it looks a bit ugly. It would also
> be nice to handle non-effective case locklessly as well, but it
> might require a larger rework.
Oops, please ignore this one. Somehow got into my unrelated sockopt series..
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
> include/linux/bpf-cgroup-defs.h | 2 +-
> include/linux/bpf-cgroup.h | 1 +
> kernel/bpf/cgroup.c | 80 +++++++++++++++++----------------
> 3 files changed, 44 insertions(+), 39 deletions(-)
>
> diff --git a/include/linux/bpf-cgroup-defs.h b/include/linux/bpf-cgroup-defs.h
> index 7b121bd780eb..df0b8faa1a17 100644
> --- a/include/linux/bpf-cgroup-defs.h
> +++ b/include/linux/bpf-cgroup-defs.h
> @@ -56,7 +56,7 @@ struct cgroup_bpf {
> * have either zero or one element
> * when BPF_F_ALLOW_MULTI the list can have up to BPF_CGROUP_MAX_PROGS
> */
> - struct hlist_head progs[MAX_CGROUP_BPF_ATTACH_TYPE];
> + struct hlist_head __rcu progs[MAX_CGROUP_BPF_ATTACH_TYPE];
> u8 flags[MAX_CGROUP_BPF_ATTACH_TYPE];
>
> /* list of cgroup shared storages */
> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> index 57e9e109257e..555e9cbb3a05 100644
> --- a/include/linux/bpf-cgroup.h
> +++ b/include/linux/bpf-cgroup.h
> @@ -106,6 +106,7 @@ struct bpf_prog_list {
> struct bpf_prog *prog;
> struct bpf_cgroup_link *link;
> struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE];
> + struct rcu_head rcu;
> };
>
> int cgroup_bpf_inherit(struct cgroup *cgrp);
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index a06e118a9be5..92a1b33dcc06 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -285,12 +285,15 @@ static void cgroup_bpf_release(struct work_struct *work)
> mutex_lock(&cgroup_mutex);
>
> for (atype = 0; atype < ARRAY_SIZE(cgrp->bpf.progs); atype++) {
> - struct hlist_head *progs = &cgrp->bpf.progs[atype];
> + struct hlist_head *progs;
> struct bpf_prog_list *pl;
> struct hlist_node *pltmp;
>
> + progs = rcu_dereference_protected(&cgrp->bpf.progs[atype],
> + lockdep_is_held(&cgroup_mutex));
> +
> hlist_for_each_entry_safe(pl, pltmp, progs, node) {
> - hlist_del(&pl->node);
> + hlist_del_rcu(&pl->node);
> if (pl->prog) {
> if (pl->prog->expected_attach_type == BPF_LSM_CGROUP)
> bpf_trampoline_unlink_cgroup_shim(pl->prog);
> @@ -301,7 +304,7 @@ static void cgroup_bpf_release(struct work_struct *work)
> bpf_trampoline_unlink_cgroup_shim(pl->link->link.prog);
> bpf_cgroup_link_auto_detach(pl->link);
> }
> - kfree(pl);
> + kfree_rcu(pl, rcu);
> static_branch_dec(&cgroup_bpf_enabled_key[atype]);
> }
> old_array = rcu_dereference_protected(
> @@ -352,12 +355,12 @@ static struct bpf_prog *prog_list_prog(struct bpf_prog_list *pl)
> /* count number of elements in the list.
> * it's slow but the list cannot be long
> */
> -static u32 prog_list_length(struct hlist_head *head)
> +static u32 prog_list_length(struct hlist_head __rcu *head)
> {
> struct bpf_prog_list *pl;
> u32 cnt = 0;
>
> - hlist_for_each_entry(pl, head, node) {
> + hlist_for_each_entry_rcu(pl, head, node) {
> if (!prog_list_prog(pl))
> continue;
> cnt++;
> @@ -553,7 +556,7 @@ static int update_effective_progs(struct cgroup *cgrp,
>
> #define BPF_CGROUP_MAX_PROGS 64
>
> -static struct bpf_prog_list *find_attach_entry(struct hlist_head *progs,
> +static struct bpf_prog_list *find_attach_entry(struct hlist_head __rcu *progs,
> struct bpf_prog *prog,
> struct bpf_cgroup_link *link,
> struct bpf_prog *replace_prog,
> @@ -565,10 +568,10 @@ static struct bpf_prog_list *find_attach_entry(struct hlist_head *progs,
> if (!allow_multi) {
> if (hlist_empty(progs))
> return NULL;
> - return hlist_entry(progs->first, typeof(*pl), node);
> + return hlist_entry(rcu_dereference_raw(progs)->first, typeof(*pl), node);
> }
>
> - hlist_for_each_entry(pl, progs, node) {
> + hlist_for_each_entry_rcu(pl, progs, node) {
> if (prog && pl->prog == prog && prog != replace_prog)
> /* disallow attaching the same prog twice */
> return ERR_PTR(-EINVAL);
> @@ -579,7 +582,7 @@ static struct bpf_prog_list *find_attach_entry(struct hlist_head *progs,
>
> /* direct prog multi-attach w/ replacement case */
> if (replace_prog) {
> - hlist_for_each_entry(pl, progs, node) {
> + hlist_for_each_entry_rcu(pl, progs, node) {
> if (pl->prog == replace_prog)
> /* a match found */
> return pl;
> @@ -615,8 +618,8 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
> struct bpf_cgroup_storage *new_storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {};
> struct bpf_prog *new_prog = prog ? : link->link.prog;
> enum cgroup_bpf_attach_type atype;
> + struct hlist_head __rcu *progs;
> struct bpf_prog_list *pl;
> - struct hlist_head *progs;
> int err;
>
> if (((flags & BPF_F_ALLOW_OVERRIDE) && (flags & BPF_F_ALLOW_MULTI)) ||
> @@ -658,6 +661,7 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
> prog ? : link->link.prog, cgrp))
> return -ENOMEM;
>
> + WRITE_ONCE(cgrp->bpf.flags[atype], saved_flags);
> if (pl) {
> old_prog = pl->prog;
> } else {
> @@ -669,12 +673,12 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
> return -ENOMEM;
> }
> if (hlist_empty(progs))
> - hlist_add_head(&pl->node, progs);
> + hlist_add_head_rcu(&pl->node, progs);
> else
> - hlist_for_each(last, progs) {
> + __hlist_for_each_rcu(last, progs) {
> if (last->next)
> continue;
> - hlist_add_behind(&pl->node, last);
> + hlist_add_behind_rcu(&pl->node, last);
> break;
> }
> }
> @@ -682,7 +686,6 @@ static int __cgroup_bpf_attach(struct cgroup *cgrp,
> pl->prog = prog;
> pl->link = link;
> bpf_cgroup_storages_assign(pl->storage, storage);
> - cgrp->bpf.flags[atype] = saved_flags;
>
> if (type == BPF_LSM_CGROUP) {
> err = bpf_trampoline_link_cgroup_shim(new_prog, atype);
> @@ -796,7 +799,7 @@ static int __cgroup_bpf_replace(struct cgroup *cgrp,
> enum cgroup_bpf_attach_type atype;
> struct bpf_prog *old_prog;
> struct bpf_prog_list *pl;
> - struct hlist_head *progs;
> + struct hlist_head __rcu *progs;
> bool found = false;
>
> atype = bpf_cgroup_atype_find(link->type, new_prog->aux->attach_btf_id);
> @@ -808,7 +811,7 @@ static int __cgroup_bpf_replace(struct cgroup *cgrp,
> if (link->link.prog->type != new_prog->type)
> return -EINVAL;
>
> - hlist_for_each_entry(pl, progs, node) {
> + hlist_for_each_entry_rcu(pl, progs, node) {
> if (pl->link == link) {
> found = true;
> break;
> @@ -847,7 +850,7 @@ static int cgroup_bpf_replace(struct bpf_link *link, struct bpf_prog *new_prog,
> return ret;
> }
>
> -static struct bpf_prog_list *find_detach_entry(struct hlist_head *progs,
> +static struct bpf_prog_list *find_detach_entry(struct hlist_head __rcu *progs,
> struct bpf_prog *prog,
> struct bpf_cgroup_link *link,
> bool allow_multi)
> @@ -862,7 +865,7 @@ static struct bpf_prog_list *find_detach_entry(struct hlist_head *progs,
> /* to maintain backward compatibility NONE and OVERRIDE cgroups
> * allow detaching with invalid FD (prog==NULL) in legacy mode
> */
> - return hlist_entry(progs->first, typeof(*pl), node);
> + return hlist_entry(rcu_dereference_raw(progs)->first, typeof(*pl), node);
> }
>
> if (!prog && !link)
> @@ -872,7 +875,7 @@ static struct bpf_prog_list *find_detach_entry(struct hlist_head *progs,
> return ERR_PTR(-EINVAL);
>
> /* find the prog or link and detach it */
> - hlist_for_each_entry(pl, progs, node) {
> + hlist_for_each_entry_rcu(pl, progs, node) {
> if (pl->prog == prog && pl->link == link)
> return pl;
> }
> @@ -894,9 +897,9 @@ static void purge_effective_progs(struct cgroup *cgrp, struct bpf_prog *prog,
> enum cgroup_bpf_attach_type atype)
> {
> struct cgroup_subsys_state *css;
> + struct hlist_head __rcu *head;
> struct bpf_prog_array *progs;
> struct bpf_prog_list *pl;
> - struct hlist_head *head;
> struct cgroup *cg;
> int pos;
>
> @@ -913,7 +916,7 @@ static void purge_effective_progs(struct cgroup *cgrp, struct bpf_prog *prog,
> continue;
>
> head = &cg->bpf.progs[atype];
> - hlist_for_each_entry(pl, head, node) {
> + hlist_for_each_entry_rcu(pl, head, node) {
> if (!prog_list_prog(pl))
> continue;
> if (pl->prog == prog && pl->link == link)
> @@ -950,9 +953,9 @@ static int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog,
> struct bpf_cgroup_link *link, enum bpf_attach_type type)
> {
> enum cgroup_bpf_attach_type atype;
> + struct hlist_head __rcu *progs;
> struct bpf_prog *old_prog;
> struct bpf_prog_list *pl;
> - struct hlist_head *progs;
> u32 attach_btf_id = 0;
> u32 flags;
>
> @@ -989,12 +992,12 @@ static int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog,
> }
>
> /* now can actually delete it from this cgroup list */
> - hlist_del(&pl->node);
> + hlist_del_rcu(&pl->node);
>
> kfree(pl);
> if (hlist_empty(progs))
> /* last program was detached, reset flags to zero */
> - cgrp->bpf.flags[atype] = 0;
> + WRITE_ONCE(cgrp->bpf.flags[atype], 0);
> if (old_prog) {
> if (type == BPF_LSM_CGROUP)
> bpf_trampoline_unlink_cgroup_shim(old_prog);
> @@ -1022,10 +1025,10 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
> __u32 __user *prog_attach_flags = u64_to_user_ptr(attr->query.prog_attach_flags);
> bool effective_query = attr->query.query_flags & BPF_F_QUERY_EFFECTIVE;
> __u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
> + struct bpf_prog_array *effective[MAX_CGROUP_BPF_ATTACH_TYPE];
> enum bpf_attach_type type = attr->query.attach_type;
> enum cgroup_bpf_attach_type from_atype, to_atype;
> enum cgroup_bpf_attach_type atype;
> - struct bpf_prog_array *effective;
> int cnt, ret = 0, i;
> int total_cnt = 0;
> u32 flags;
> @@ -1046,14 +1049,15 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
> if (from_atype < 0)
> return -EINVAL;
> to_atype = from_atype;
> - flags = cgrp->bpf.flags[from_atype];
> + flags = READ_ONCE(cgrp->bpf.flags[from_atype]);
> }
>
> for (atype = from_atype; atype <= to_atype; atype++) {
> if (effective_query) {
> - effective = rcu_dereference_protected(cgrp->bpf.effective[atype],
> - lockdep_is_held(&cgroup_mutex));
> - total_cnt += bpf_prog_array_length(effective);
> + effective[atype] = rcu_dereference_protected(
> + cgrp->bpf.effective[atype],
> + lockdep_is_held(&cgroup_mutex));
> + total_cnt += bpf_prog_array_length(effective[atype]);
> } else {
> total_cnt += prog_list_length(&cgrp->bpf.progs[atype]);
> }
> @@ -1076,12 +1080,10 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
>
> for (atype = from_atype; atype <= to_atype && total_cnt; atype++) {
> if (effective_query) {
> - effective = rcu_dereference_protected(cgrp->bpf.effective[atype],
> - lockdep_is_held(&cgroup_mutex));
> - cnt = min_t(int, bpf_prog_array_length(effective), total_cnt);
> - ret = bpf_prog_array_copy_to_user(effective, prog_ids, cnt);
> + cnt = min_t(int, bpf_prog_array_length(effective[atype]), total_cnt);
> + ret = bpf_prog_array_copy_to_user(effective[atype], prog_ids, cnt);
> } else {
> - struct hlist_head *progs;
> + struct hlist_head __rcu *progs;
> struct bpf_prog_list *pl;
> struct bpf_prog *prog;
> u32 id;
> @@ -1089,7 +1091,7 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
> progs = &cgrp->bpf.progs[atype];
> cnt = min_t(int, prog_list_length(progs), total_cnt);
> i = 0;
> - hlist_for_each_entry(pl, progs, node) {
> + hlist_for_each_entry_rcu(pl, progs, node) {
> prog = prog_list_prog(pl);
> id = prog->aux->id;
> if (copy_to_user(prog_ids + i, &id, sizeof(id)))
> @@ -1099,7 +1101,7 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
> }
>
> if (prog_attach_flags) {
> - flags = cgrp->bpf.flags[atype];
> + flags = READ_ONCE(cgrp->bpf.flags[atype]);
>
> for (i = 0; i < cnt; i++)
> if (copy_to_user(prog_attach_flags + i,
> @@ -1112,6 +1114,8 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
> prog_ids += cnt;
> total_cnt -= cnt;
> }
> + if (total_cnt != 0)
> + return -EAGAIN; /* raced with the detach */
> return ret;
> }
>
> @@ -1120,9 +1124,9 @@ static int cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
> {
> int ret;
>
> - mutex_lock(&cgroup_mutex);
> + rcu_read_lock();
> ret = __cgroup_bpf_query(cgrp, attr, uattr);
> - mutex_unlock(&cgroup_mutex);
> + rcu_read_unlock();
> return ret;
> }
>
> --
> 2.40.1.521.gf1e218fcd8-goog
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-05-11 16:00 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-11 15:57 [PATCH bpf-next] RFC: bpf: query effective progs without cgroup_mutex Stanislav Fomichev
2023-05-11 15:57 ` [PATCH bpf-next v5 0/4] bpf: Don't EFAULT for {g,s}setsockopt with wrong optlen Stanislav Fomichev
2023-05-11 15:57 ` [PATCH bpf-next v5 1/4] " Stanislav Fomichev
2023-05-11 15:57 ` [PATCH bpf-next v5 2/4] selftests/bpf: Update EFAULT {g,s}etsockopt selftests Stanislav Fomichev
2023-05-11 15:57 ` [PATCH bpf-next v5 3/4] selftests/bpf: Correctly handle optlen > 4096 Stanislav Fomichev
2023-05-11 15:57 ` [PATCH bpf-next v5 4/4] bpf: Document EFAULT changes for sockopt Stanislav Fomichev
2023-05-11 15:59 ` [PATCH bpf-next] RFC: bpf: query effective progs without cgroup_mutex Stanislav Fomichev
-- strict thread matches above, loose matches on Subject: below --
2023-05-05 18:45 Stanislav Fomichev
2023-05-05 18:47 ` Stanislav Fomichev
2023-05-05 21:35 ` Stanislav Fomichev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox