On 4/2/24 10:39 AM, Eduard Zingerman wrote:
On Mon, 2024-03-25 at 19:21 -0700, Yonghong Song wrote:
[...]
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 27d733c0f65e..dafc9aa6e192 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
[...]
@@ -1488,21 +1492,90 @@ static int sock_map_prog_lookup(struct bpf_map *map, struct bpf_prog ***pprog,
return 0;
}
+static int sock_map_link_lookup(struct bpf_map *map, struct bpf_link ***plink,
+ struct bpf_link *link, bool skip_check, u32 which)
+{
+ struct sk_psock_progs *progs = sock_map_progs(map);
+
+ switch (which) {
+ case BPF_SK_MSG_VERDICT:
+ if (!skip_check &&
+ ((!link && progs->msg_parser_link) ||
+ (link && link != progs->msg_parser_link)))
+ return -EBUSY;
These checks seem a bit repetitive, maybe factor it out as a single
check at the end of the function? E.g.:
if (!skip_check &&
((!link && **plink) || (link && link != **plink)))
return -EBUSY;
Or inline these checks at call sites for sock_map_link_lookup()?
I tried this on top of this in [1] and all tests seem to pass.
Andrii has a suggestion to do
plink = progs->msg_parser_link;
and later plink can be used for checking. This indeed makes things easier.
[1] https://gist.github.com/eddyz87/38d832b3f1fc74120598d3480bc16ae1
+ *plink = &progs->msg_parser_link;
+ break;
+#if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
+ case BPF_SK_SKB_STREAM_PARSER:
+ if (!skip_check &&
+ ((!link && progs->stream_parser_link) ||
+ (link && link != progs->stream_parser_link)))
+ return -EBUSY;
+ *plink = &progs->stream_parser_link;
+ break;
+#endif
+ case BPF_SK_SKB_STREAM_VERDICT:
+ if (!skip_check &&
+ ((!link && progs->stream_verdict_link) ||
+ (link && link != progs->stream_verdict_link)))
+ return -EBUSY;
+ *plink = &progs->stream_verdict_link;
+ break;
+ case BPF_SK_SKB_VERDICT:
+ if (!skip_check &&
+ ((!link && progs->skb_verdict_link) ||
+ (link && link != progs->skb_verdict_link)))
+ return -EBUSY;
+ *plink = &progs->skb_verdict_link;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
[...]
+/* Handle the following two cases:
+ * case 1: link != NULL, prog != NULL, old != NULL
+ * case 2: link != NULL, prog != NULL, old == NULL
+ */
+static int sock_map_link_update_prog(struct bpf_link *link,
+ struct bpf_prog *prog,
+ struct bpf_prog *old)
+{
+ const struct sockmap_link *sockmap_link = get_sockmap_link(link);
+ struct bpf_prog **pprog;
+ struct bpf_link **plink;
+ int ret = 0;
+
+ mutex_lock(&sockmap_prog_update_mutex);
+
+ /* If old prog not NULL, ensure old prog the same as link->prog. */
+ if (old && link->prog != old) {
+ ret = -EINVAL;
+ goto out;
+ }
+ /* Ensure link->prog has the same type/attach_type as the new prog. */
+ if (link->prog->type != prog->type ||
+ link->prog->expected_attach_type != prog->expected_attach_type) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ ret = sock_map_prog_lookup(sockmap_link->map, &pprog,
+ sockmap_link->attach_type);
+ if (ret)
+ goto out;
+
+ /* Ensure the same link between the one in map and the passed-in. */
+ ret = sock_map_link_lookup(sockmap_link->map, &plink, link, false,
+ sockmap_link->attach_type);
+ if (ret)
+ goto out;
+
+ if (old)
+ return psock_replace_prog(pprog, prog, old);
should this be 'goto out' in order to unlock the mutex?
Good point. I missed a test case with non-NULL old. Will add in the next revision.
+
+ psock_set_prog(pprog, prog);
+
+out:
+ if (!ret)
+ bpf_prog_inc(prog);
+ mutex_unlock(&sockmap_prog_update_mutex);
+ return ret;
+}
[...]
