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
--
Sponsored by the NGI0 Core fund.