Message ID | 020b0eb4dff6e4b19dba43609c758e49d1f38a08.1729588019.git.tanggeliang@kylinos.cn (mailing list archive) |
---|---|
State | Superseded, archived |
Delegated to: | Matthieu Baerts |
Headers | show |
Series | BPF path manager | expand |
Context | Check | Description |
---|---|---|
matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 282 lines checked |
matttbe/shellcheck | success | MPTCP selftests files have not been modified |
matttbe/build | warning | Build error with: make C=1 net/mptcp/bpf.o |
matttbe/KVM_Validation__normal | success | Success! ✅ |
matttbe/KVM_Validation__debug | success | Success! ✅ |
matttbe/KVM_Validation__btf-normal__only_bpftest_all_ | success | Success! ✅ |
matttbe/KVM_Validation__btf-debug__only_bpftest_all_ | success | Success! ✅ |
Hi Geliang, On 22/10/2024 11:14, Geliang Tang wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > Each userspace pm netlink function uses nla_get_u32() to get the msk > token value, then pass it to mptcp_token_get_sock() to get the msk. > Finally check whether userspace PM is selected on this msk. It makes > sense to wrap them into a helper, named mptcp_userspace_pm_get_sock(), > to do this. > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> > --- > net/mptcp/pm_userspace.c | 137 +++++++++++++-------------------------- > 1 file changed, 44 insertions(+), 93 deletions(-) Looks like a good refactoring! > > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c > index ce0f7131c701..79ae32c921b7 100644 > --- a/net/mptcp/pm_userspace.c > +++ b/net/mptcp/pm_userspace.c > @@ -169,36 +169,51 @@ bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk, > return backup; > } > > -int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info) > +static struct mptcp_sock *mptcp_userspace_pm_get_sock(const struct genl_info *info) > { > struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN]; > + struct mptcp_sock *msk = NULL; (no need to set it to NULL, see below) > + > + if (!token) { > + GENL_SET_ERR_MSG(info, "missing required inputs"); Maybe we can take this opportunity to mention the token: "missing required token". > + goto out; Detail: simply return 'NULL' here, there is no clean-up to do, no? > + } > + > + msk = mptcp_token_get_sock(genl_info_net(info), nla_get_u32(token)); I see that below, we were also using sock_net(skb->sk) or even sock_net(msg->sk). Please mention such modifications in the commit message, explaining why it is OK to use only genl_info_net(). I guess it is the same info, but better to be sure and not hide that. > + if (!msk) { > + NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token"); > + goto out; Same here. > + } > + > + if (!mptcp_pm_is_userspace(msk)) { > + GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected"); > + sock_put((struct sock *)msk); Mmh, I was not sure if it would be a good idea to do this check and sock_put() here, but I guess that's OK as we just got the msk with the previous call. (no need to change then) > + msk = NULL; I guess you can also simply return NULL here, and remove the 'out' label. > + } > + > +out: > + return msk; > +} > + > +int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info) > +{ > struct nlattr *addr = info->attrs[MPTCP_PM_ATTR_ADDR]; > struct mptcp_pm_addr_entry addr_val; > struct mptcp_sock *msk; > int err = -EINVAL; > struct sock *sk; > - u32 token_val; > > - if (!addr || !token) { > + if (!addr) { > GENL_SET_ERR_MSG(info, "missing required inputs"); Same here, "missing required address". > return err; > } > > - token_val = nla_get_u32(token); > - > - msk = mptcp_token_get_sock(sock_net(skb->sk), token_val); > - if (!msk) { > - NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token"); > + msk = mptcp_userspace_pm_get_sock(info); > + if (!msk) > return err; > - } > > sk = (struct sock *)msk; > > - if (!mptcp_pm_is_userspace(msk)) { > - GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected"); > - goto announce_err; > - } > - > err = mptcp_pm_parse_entry(addr, info, true, &addr_val); > if (err < 0) { > GENL_SET_ERR_MSG(info, "error parsing local address"); > @@ -271,7 +286,6 @@ static int mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock *msk, > > int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info) > { > - struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN]; > struct nlattr *id = info->attrs[MPTCP_PM_ATTR_LOC_ID]; > struct mptcp_pm_addr_entry *match; > struct mptcp_pm_addr_entry *entry; > @@ -279,30 +293,21 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info) > LIST_HEAD(free_list); > int err = -EINVAL; > struct sock *sk; > - u32 token_val; > u8 id_val; > > - if (!id || !token) { > + if (!id) { > GENL_SET_ERR_MSG(info, "missing required inputs"); Same here, "missing required ID". > return err; > } > > id_val = nla_get_u8(id); > - token_val = nla_get_u32(token); > > - msk = mptcp_token_get_sock(sock_net(skb->sk), token_val); > - if (!msk) { > - NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token"); > + msk = mptcp_userspace_pm_get_sock(info); > + if (!msk) > return err; > - } > > sk = (struct sock *)msk; > > - if (!mptcp_pm_is_userspace(msk)) { > - GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected"); > - goto out; > - } > - > if (id_val == 0) { > err = mptcp_userspace_pm_remove_id_zero_address(msk, info); > goto out; > @@ -336,7 +341,6 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info) > int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info) > { > struct nlattr *raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE]; > - struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN]; > struct nlattr *laddr = info->attrs[MPTCP_PM_ATTR_ADDR]; > struct mptcp_pm_addr_entry entry = { 0 }; > struct mptcp_addr_info addr_r; > @@ -344,28 +348,18 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info) > struct mptcp_sock *msk; > int err = -EINVAL; > struct sock *sk; > - u32 token_val; > > - if (!laddr || !raddr || !token) { > + if (!laddr || !raddr) { > GENL_SET_ERR_MSG(info, "missing required inputs"); I guess here, you could have "missing required address(es)". > return err; > } > > - token_val = nla_get_u32(token); > - > - msk = mptcp_token_get_sock(genl_info_net(info), token_val); > - if (!msk) { > - NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token"); > + msk = mptcp_userspace_pm_get_sock(info); > + if (!msk) > return err; > - } > > sk = (struct sock *)msk; > > - if (!mptcp_pm_is_userspace(msk)) { > - GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected"); > - goto create_err; > - } > - > err = mptcp_pm_parse_entry(laddr, info, true, &entry); > if (err < 0) { > NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error parsing local addr"); > @@ -468,35 +462,24 @@ static struct sock *mptcp_nl_find_ssk(struct mptcp_sock *msk, > int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info) > { > struct nlattr *raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE]; > - struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN]; > struct nlattr *laddr = info->attrs[MPTCP_PM_ATTR_ADDR]; > struct mptcp_addr_info addr_l; > struct mptcp_addr_info addr_r; > struct mptcp_sock *msk; > struct sock *sk, *ssk; > int err = -EINVAL; > - u32 token_val; > > - if (!laddr || !raddr || !token) { > + if (!laddr || !raddr) { I guess here, you could have "missing required address(es)". > GENL_SET_ERR_MSG(info, "missing required inputs"); > return err; > } > > - token_val = nla_get_u32(token); > - > - msk = mptcp_token_get_sock(genl_info_net(info), token_val); > - if (!msk) { > - NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token"); > + msk = mptcp_userspace_pm_get_sock(info); > + if (!msk) > return err; > - } > > sk = (struct sock *)msk; > > - if (!mptcp_pm_is_userspace(msk)) { > - GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected"); > - goto destroy_err; > - } > - > err = mptcp_pm_parse_addr(laddr, info, &addr_l); > if (err < 0) { > NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error parsing local addr"); > @@ -559,30 +542,18 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info) > struct mptcp_pm_addr_entry loc = { .addr = { .family = AF_UNSPEC }, }; > struct mptcp_pm_addr_entry rem = { .addr = { .family = AF_UNSPEC }, }; > struct nlattr *attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE]; > - struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN]; > struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR]; > - struct net *net = sock_net(skb->sk); > struct mptcp_sock *msk; > int ret = -EINVAL; > struct sock *sk; > - u32 token_val; > u8 bkup = 0; > > - token_val = nla_get_u32(token); > - > - msk = mptcp_token_get_sock(net, token_val); > - if (!msk) { > - NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token"); > + msk = mptcp_userspace_pm_get_sock(info); > + if (!msk) > return ret; > - } > > sk = (struct sock *)msk; > > - if (!mptcp_pm_is_userspace(msk)) { > - GENL_SET_ERR_MSG(info, "userspace PM not selected"); > - goto set_flags_err; > - } > - > ret = mptcp_pm_parse_entry(attr, info, false, &loc); > if (ret < 0) > goto set_flags_err; > @@ -619,30 +590,20 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg, > DECLARE_BITMAP(map, MPTCP_PM_MAX_ADDR_ID + 1); > } *bitmap; > const struct genl_info *info = genl_info_dump(cb); > - struct net *net = sock_net(msg->sk); > struct mptcp_pm_addr_entry *entry; > struct mptcp_sock *msk; > - struct nlattr *token; > int ret = -EINVAL; > struct sock *sk; > void *hdr; > > bitmap = (struct id_bitmap *)cb->ctx; > - token = info->attrs[MPTCP_PM_ATTR_TOKEN]; > > - msk = mptcp_token_get_sock(net, nla_get_u32(token)); > - if (!msk) { > - NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token"); > + msk = mptcp_userspace_pm_get_sock(info); > + if (!msk) > return ret; > - } > > sk = (struct sock *)msk; > > - if (!mptcp_pm_is_userspace(msk)) { > - GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected"); > - goto out; > - } > - > lock_sock(sk); > spin_lock_bh(&msk->pm.lock); > mptcp_for_each_address(msk, entry) { > @@ -667,7 +628,6 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg, > release_sock(sk); > ret = msg->len; > > -out: > sock_put(sk); > return ret; > } > @@ -676,28 +636,19 @@ int mptcp_userspace_pm_get_addr(struct sk_buff *skb, > struct genl_info *info) > { > struct nlattr *attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR]; > - struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN]; > struct mptcp_pm_addr_entry addr, *entry; > - struct net *net = sock_net(skb->sk); > struct mptcp_sock *msk; > struct sk_buff *msg; > int ret = -EINVAL; > struct sock *sk; > void *reply; > > - msk = mptcp_token_get_sock(net, nla_get_u32(token)); > - if (!msk) { > - NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token"); > + msk = mptcp_userspace_pm_get_sock(info); > + if (!msk) > return ret; > - } > > sk = (struct sock *)msk; > > - if (!mptcp_pm_is_userspace(msk)) { > - GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected"); > - goto out; > - } > - > ret = mptcp_pm_parse_entry(attr, info, false, &addr); > if (ret < 0) > goto out; Cheers, Matt
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c index ce0f7131c701..79ae32c921b7 100644 --- a/net/mptcp/pm_userspace.c +++ b/net/mptcp/pm_userspace.c @@ -169,36 +169,51 @@ bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk, return backup; } -int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info) +static struct mptcp_sock *mptcp_userspace_pm_get_sock(const struct genl_info *info) { struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN]; + struct mptcp_sock *msk = NULL; + + if (!token) { + GENL_SET_ERR_MSG(info, "missing required inputs"); + goto out; + } + + msk = mptcp_token_get_sock(genl_info_net(info), nla_get_u32(token)); + if (!msk) { + NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token"); + goto out; + } + + if (!mptcp_pm_is_userspace(msk)) { + GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected"); + sock_put((struct sock *)msk); + msk = NULL; + } + +out: + return msk; +} + +int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info) +{ struct nlattr *addr = info->attrs[MPTCP_PM_ATTR_ADDR]; struct mptcp_pm_addr_entry addr_val; struct mptcp_sock *msk; int err = -EINVAL; struct sock *sk; - u32 token_val; - if (!addr || !token) { + if (!addr) { GENL_SET_ERR_MSG(info, "missing required inputs"); return err; } - token_val = nla_get_u32(token); - - msk = mptcp_token_get_sock(sock_net(skb->sk), token_val); - if (!msk) { - NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token"); + msk = mptcp_userspace_pm_get_sock(info); + if (!msk) return err; - } sk = (struct sock *)msk; - if (!mptcp_pm_is_userspace(msk)) { - GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected"); - goto announce_err; - } - err = mptcp_pm_parse_entry(addr, info, true, &addr_val); if (err < 0) { GENL_SET_ERR_MSG(info, "error parsing local address"); @@ -271,7 +286,6 @@ static int mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock *msk, int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info) { - struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN]; struct nlattr *id = info->attrs[MPTCP_PM_ATTR_LOC_ID]; struct mptcp_pm_addr_entry *match; struct mptcp_pm_addr_entry *entry; @@ -279,30 +293,21 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info) LIST_HEAD(free_list); int err = -EINVAL; struct sock *sk; - u32 token_val; u8 id_val; - if (!id || !token) { + if (!id) { GENL_SET_ERR_MSG(info, "missing required inputs"); return err; } id_val = nla_get_u8(id); - token_val = nla_get_u32(token); - msk = mptcp_token_get_sock(sock_net(skb->sk), token_val); - if (!msk) { - NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token"); + msk = mptcp_userspace_pm_get_sock(info); + if (!msk) return err; - } sk = (struct sock *)msk; - if (!mptcp_pm_is_userspace(msk)) { - GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected"); - goto out; - } - if (id_val == 0) { err = mptcp_userspace_pm_remove_id_zero_address(msk, info); goto out; @@ -336,7 +341,6 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info) int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info) { struct nlattr *raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE]; - struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN]; struct nlattr *laddr = info->attrs[MPTCP_PM_ATTR_ADDR]; struct mptcp_pm_addr_entry entry = { 0 }; struct mptcp_addr_info addr_r; @@ -344,28 +348,18 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info) struct mptcp_sock *msk; int err = -EINVAL; struct sock *sk; - u32 token_val; - if (!laddr || !raddr || !token) { + if (!laddr || !raddr) { GENL_SET_ERR_MSG(info, "missing required inputs"); return err; } - token_val = nla_get_u32(token); - - msk = mptcp_token_get_sock(genl_info_net(info), token_val); - if (!msk) { - NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token"); + msk = mptcp_userspace_pm_get_sock(info); + if (!msk) return err; - } sk = (struct sock *)msk; - if (!mptcp_pm_is_userspace(msk)) { - GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected"); - goto create_err; - } - err = mptcp_pm_parse_entry(laddr, info, true, &entry); if (err < 0) { NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error parsing local addr"); @@ -468,35 +462,24 @@ static struct sock *mptcp_nl_find_ssk(struct mptcp_sock *msk, int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info) { struct nlattr *raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE]; - struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN]; struct nlattr *laddr = info->attrs[MPTCP_PM_ATTR_ADDR]; struct mptcp_addr_info addr_l; struct mptcp_addr_info addr_r; struct mptcp_sock *msk; struct sock *sk, *ssk; int err = -EINVAL; - u32 token_val; - if (!laddr || !raddr || !token) { + if (!laddr || !raddr) { GENL_SET_ERR_MSG(info, "missing required inputs"); return err; } - token_val = nla_get_u32(token); - - msk = mptcp_token_get_sock(genl_info_net(info), token_val); - if (!msk) { - NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token"); + msk = mptcp_userspace_pm_get_sock(info); + if (!msk) return err; - } sk = (struct sock *)msk; - if (!mptcp_pm_is_userspace(msk)) { - GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected"); - goto destroy_err; - } - err = mptcp_pm_parse_addr(laddr, info, &addr_l); if (err < 0) { NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error parsing local addr"); @@ -559,30 +542,18 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info) struct mptcp_pm_addr_entry loc = { .addr = { .family = AF_UNSPEC }, }; struct mptcp_pm_addr_entry rem = { .addr = { .family = AF_UNSPEC }, }; struct nlattr *attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE]; - struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN]; struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR]; - struct net *net = sock_net(skb->sk); struct mptcp_sock *msk; int ret = -EINVAL; struct sock *sk; - u32 token_val; u8 bkup = 0; - token_val = nla_get_u32(token); - - msk = mptcp_token_get_sock(net, token_val); - if (!msk) { - NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token"); + msk = mptcp_userspace_pm_get_sock(info); + if (!msk) return ret; - } sk = (struct sock *)msk; - if (!mptcp_pm_is_userspace(msk)) { - GENL_SET_ERR_MSG(info, "userspace PM not selected"); - goto set_flags_err; - } - ret = mptcp_pm_parse_entry(attr, info, false, &loc); if (ret < 0) goto set_flags_err; @@ -619,30 +590,20 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg, DECLARE_BITMAP(map, MPTCP_PM_MAX_ADDR_ID + 1); } *bitmap; const struct genl_info *info = genl_info_dump(cb); - struct net *net = sock_net(msg->sk); struct mptcp_pm_addr_entry *entry; struct mptcp_sock *msk; - struct nlattr *token; int ret = -EINVAL; struct sock *sk; void *hdr; bitmap = (struct id_bitmap *)cb->ctx; - token = info->attrs[MPTCP_PM_ATTR_TOKEN]; - msk = mptcp_token_get_sock(net, nla_get_u32(token)); - if (!msk) { - NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token"); + msk = mptcp_userspace_pm_get_sock(info); + if (!msk) return ret; - } sk = (struct sock *)msk; - if (!mptcp_pm_is_userspace(msk)) { - GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected"); - goto out; - } - lock_sock(sk); spin_lock_bh(&msk->pm.lock); mptcp_for_each_address(msk, entry) { @@ -667,7 +628,6 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg, release_sock(sk); ret = msg->len; -out: sock_put(sk); return ret; } @@ -676,28 +636,19 @@ int mptcp_userspace_pm_get_addr(struct sk_buff *skb, struct genl_info *info) { struct nlattr *attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR]; - struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN]; struct mptcp_pm_addr_entry addr, *entry; - struct net *net = sock_net(skb->sk); struct mptcp_sock *msk; struct sk_buff *msg; int ret = -EINVAL; struct sock *sk; void *reply; - msk = mptcp_token_get_sock(net, nla_get_u32(token)); - if (!msk) { - NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token"); + msk = mptcp_userspace_pm_get_sock(info); + if (!msk) return ret; - } sk = (struct sock *)msk; - if (!mptcp_pm_is_userspace(msk)) { - GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected"); - goto out; - } - ret = mptcp_pm_parse_entry(attr, info, false, &addr); if (ret < 0) goto out;