On 4/6/24 4:18 PM, Eduard Zingerman wrote:
On Sat, 2024-04-06 at 09:04 -0700, Yonghong Song wrote:
[...]
@@ -1454,55 +1466,95 @@ static struct sk_psock_progs *sock_map_progs(struct bpf_map *map)
return NULL;
}
-static int sock_map_prog_lookup(struct bpf_map *map, struct bpf_prog ***pprog,
- u32 which)
+static int sock_map_prog_link_lookup(struct bpf_map *map, struct bpf_prog ***pprog,
+ struct bpf_link ***plink, struct bpf_link *link,
+ bool skip_link_check, u32 which)
{
struct sk_psock_progs *progs = sock_map_progs(map);
+ struct bpf_prog **cur_pprog;
+ struct bpf_link **cur_plink;
if (!progs)
return -EOPNOTSUPP;
switch (which) {
case BPF_SK_MSG_VERDICT:
- *pprog = &progs->msg_parser;
+ cur_pprog = &progs->msg_parser;
+ cur_plink = &progs->msg_parser_link;
break;
#if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
case BPF_SK_SKB_STREAM_PARSER:
- *pprog = &progs->stream_parser;
+ cur_pprog = &progs->stream_parser;
+ cur_plink = &progs->stream_parser_link;
break;
#endif
case BPF_SK_SKB_STREAM_VERDICT:
if (progs->skb_verdict)
return -EBUSY;
- *pprog = &progs->stream_verdict;
+ cur_pprog = &progs->stream_verdict;
+ cur_plink = &progs->stream_verdict_link;
break;
case BPF_SK_SKB_VERDICT:
if (progs->stream_verdict)
return -EBUSY;
- *pprog = &progs->skb_verdict;
+ cur_pprog = &progs->skb_verdict;
+ cur_plink = &progs->skb_verdict_link;
break;
default:
return -EOPNOTSUPP;
}
+ /* The link check will be skipped for link_detach case. */
+ if (!skip_link_check) {
+ /* for prog_attach/prog_detach/link_attach, return error if a bpf_link
+ * exists for that prog.
+ */
+ if (!link && *cur_plink)
+ return -EBUSY;
+
+ /* for bpf_link based prog_update, return error if the stored bpf_link
+ * does not match the incoming bpf_link.
+ */
+ if (link && link != *cur_plink)
+ return -EBUSY;
+ }
I still think that this check should be factored out to callers,
this allows to reduce the number of function parameters,
and better separate unrelated logical error conditions.
E.g. like in the patch at the end of this email
(applied on top of the current patch).
Thanks Eduard. I also bothered with too many arguments for
the function. I guess your suggested change below indeed
better as we still keep the attach type enum in the function
and factored following code in different situations.
Will make the change in the next revision!
[...]
---
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 4af44277568e..a642215faa20 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -1467,8 +1467,7 @@ static struct sk_psock_progs *sock_map_progs(struct bpf_map *map)
}
static int sock_map_prog_link_lookup(struct bpf_map *map, struct bpf_prog ***pprog,
- struct bpf_link ***plink, struct bpf_link *link,
- bool skip_link_check, u32 which)
+ struct bpf_link ***plink, u32 which)
{
struct sk_psock_progs *progs = sock_map_progs(map);
struct bpf_prog **cur_pprog;
@@ -1504,21 +1503,6 @@ static int sock_map_prog_link_lookup(struct bpf_map *map, struct bpf_prog ***ppr
return -EOPNOTSUPP;
}
- /* The link check will be skipped for link_detach case. */
- if (!skip_link_check) {
- /* for prog_attach/prog_detach/link_attach, return error if a bpf_link
- * exists for that prog.
- */
- if (!link && *cur_plink)
- return -EBUSY;
-
- /* for bpf_link based prog_update, return error if the stored bpf_link
- * does not match the incoming bpf_link.
- */
- if (link && link != *cur_plink)
- return -EBUSY;
- }
-
*pprog = cur_pprog;
if (plink)
*plink = cur_plink;
@@ -1539,9 +1523,14 @@ static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
struct bpf_link **plink;
int ret;
- ret = sock_map_prog_link_lookup(map, &pprog, &plink, NULL, link && !prog, which);
+ ret = sock_map_prog_link_lookup(map, &pprog, &plink, which);
if (ret)
- goto out;
+ return ret;
+ /* for prog_attach/prog_detach/link_attach, return error if a bpf_link
+ * exists for that prog.
+ */
+ if ((!link || prog) && *plink)
+ return -EBUSY;
if (old) {
ret = psock_replace_prog(pprog, prog, old);
@@ -1553,8 +1542,7 @@ static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
*plink = link;
}
-out:
- return ret;
+ return 0;
}
int sock_map_bpf_prog_query(const union bpf_attr *attr,
@@ -1579,7 +1567,7 @@ int sock_map_bpf_prog_query(const union bpf_attr *attr,
rcu_read_lock();
- ret = sock_map_prog_link_lookup(map, &pprog, NULL, NULL, true, attr->query.attach_type);
+ ret = sock_map_prog_link_lookup(map, &pprog, NULL, attr->query.attach_type);
if (ret)
goto end;
@@ -1770,10 +1758,15 @@ static int sock_map_link_update_prog(struct bpf_link *link,
goto out;
}
- ret = sock_map_prog_link_lookup(sockmap_link->map, &pprog, &plink, link, false,
+ ret = sock_map_prog_link_lookup(sockmap_link->map, &pprog, &plink,
sockmap_link->attach_type);
if (ret)
goto out;
+ /* for bpf_link based prog_update, return error if the stored bpf_link
+ * does not match the incoming bpf_link.
+ */
+ if (link != *plink)
+ return -EBUSY;
if (old) {
ret = psock_replace_prog(pprog, prog, old);
