Re: [PATCH bpf-next v5 1/5] bpf: Add bpf_link support for sk_msg and sk_skb progs

[Eduard Zingerman <eddyz87@gmail.com>]

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).

[...]

---

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);