Re: [PATCH mptcp-next v2 3/8] mptcp: add id parameter for get_addr

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

Sorry, I just noticed this will cause conflicts with the series I just sent:


https://lore.kernel.org/all/20241230-genl_req_attr_check-v6-0-3ec9103559e7@kernel.org/T/

On 13/12/2024 08:35, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The address id is parsed both in mptcp_pm_nl_get_addr() and
> mptcp_userspace_pm_get_addr(), this makes the code somewhat repetitive.
> So this patch adds a new parameter 'id' for all get_addr() interfaces.
> The address id is only parsed in mptcp_pm_nl_get_addr_doit(), then pass
> it to both mptcp_pm_nl_get_addr() and mptcp_userspace_pm_get_addr().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm.c           | 16 ++++++++++++----
>  net/mptcp/pm_netlink.c   | 11 +++--------
>  net/mptcp/pm_userspace.c | 11 +++--------
>  net/mptcp/protocol.h     |  4 ++--
>  4 files changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 526e5bca1fa1..c7d323c7c7aa 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -434,16 +434,24 @@ bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
>  	return mptcp_pm_nl_is_backup(msk, &skc_local);
>  }
>  
> -static int mptcp_pm_get_addr(struct genl_info *info)
> +static int mptcp_pm_get_addr(u8 id, struct genl_info *info)
>  {
>  	if (info->attrs[MPTCP_PM_ATTR_TOKEN])
> -		return mptcp_userspace_pm_get_addr(info);
> -	return mptcp_pm_nl_get_addr(info);
> +		return mptcp_userspace_pm_get_addr(id, info);
> +	return mptcp_pm_nl_get_addr(id, info);
>  }
>  
>  int mptcp_pm_nl_get_addr_doit(struct sk_buff *skb, struct genl_info *info)
>  {
> -	return mptcp_pm_get_addr(info);
> +	struct nlattr *attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
> +	struct mptcp_pm_addr_entry addr;
> +	int ret;
> +
> +	ret = mptcp_pm_parse_entry(attr, info, false, &addr);

Could you first check if the attribute is set (to report a more precise
error), before parsing it please?

  if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ENDPOINT_ADDR))
        return -EINVAL;

  attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
  ret = mptcp_pm_parse_entry(attr, info, false, &addr);
  (...)

So similar to this:


https://lore.kernel.org/all/20241230-genl_req_attr_check-v6-7-3ec9103559e7@kernel.org/

> +	if (ret < 0)
> +		return ret;
> +
> +	return mptcp_pm_get_addr(addr.addr.id, info);
>  }
>  
>  static int mptcp_pm_dump_addr(struct sk_buff *msg, struct netlink_callback *cb)
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 92b4dcc310d3..de6a8e7a4a1a 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1762,19 +1762,14 @@ int mptcp_nl_fill_addr(struct sk_buff *skb,
>  	return -EMSGSIZE;
>  }
>  
> -int mptcp_pm_nl_get_addr(struct genl_info *info)
> +int mptcp_pm_nl_get_addr(u8 id, struct genl_info *info)
>  {
> -	struct nlattr *attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
>  	struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
> -	struct mptcp_pm_addr_entry addr, *entry;
> +	struct mptcp_pm_addr_entry *entry;
>  	struct sk_buff *msg;
>  	void *reply;
>  	int ret;
>  
> -	ret = mptcp_pm_parse_entry(attr, info, false, &addr);
> -	if (ret < 0)
> -		return ret;
> -
>  	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>  	if (!msg)
>  		return -ENOMEM;
> @@ -1788,7 +1783,7 @@ int mptcp_pm_nl_get_addr(struct genl_info *info)
>  	}
>  
>  	rcu_read_lock();
> -	entry = __lookup_addr_by_id(pernet, addr.addr.id);
> +	entry = __lookup_addr_by_id(pernet, id);
>  	if (!entry) {
>  		GENL_SET_ERR_MSG(info, "address not found");

Mmh, I switched this to NL_SET_ERR_MSG_ATTR() in:


https://patchwork.kernel.org/project/mptcp/patch/20241230-genl_req_attr_check-v6-1-3ec9103559e7@kernel.org/

... which requires 'attr'. We could not do the conversion here in the
patch I sent. But because you are also moving this part to pm.c in the
next patch, I suggest to temporally pass the pointer to 'attr' here in
this patch 3, and remove it in patch 4.

WDYT?

>  		ret = -EINVAL;
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index ab92efec6618..40a018be243e 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -663,10 +663,9 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
>  	return ret;
>  }
>  
> -int mptcp_userspace_pm_get_addr(struct genl_info *info)
> +int mptcp_userspace_pm_get_addr(u8 id, struct genl_info *info)
>  {
> -	struct nlattr *attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
> -	struct mptcp_pm_addr_entry addr, *entry;
> +	struct mptcp_pm_addr_entry *entry;
>  	struct mptcp_sock *msk;
>  	struct sk_buff *msg;
>  	int ret = -EINVAL;
> @@ -679,10 +678,6 @@ int mptcp_userspace_pm_get_addr(struct genl_info *info)
>  
>  	sk = (struct sock *)msk;
>  
> -	ret = mptcp_pm_parse_entry(attr, info, false, &addr);
> -	if (ret < 0)
> -		goto out;
> -
>  	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>  	if (!msg) {
>  		ret = -ENOMEM;
> @@ -699,7 +694,7 @@ int mptcp_userspace_pm_get_addr(struct genl_info *info)
>  
>  	lock_sock(sk);
>  	spin_lock_bh(&msk->pm.lock);
> -	entry = mptcp_userspace_pm_lookup_addr_by_id(msk, addr.addr.id);
> +	entry = mptcp_userspace_pm_lookup_addr_by_id(msk, id);
>  	if (!entry) {
>  		GENL_SET_ERR_MSG(info, "address not found");

Same here for GENL_SET_ERR_MSG() vs NL_SET_ERR_MSG_ATTR().

>  		ret = -EINVAL;
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index dd673b41f0ce..76a0cfe54723 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1131,8 +1131,8 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
>  			  struct netlink_callback *cb);
>  int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
>  				 struct netlink_callback *cb);
> -int mptcp_pm_nl_get_addr(struct genl_info *info);
> -int mptcp_userspace_pm_get_addr(struct genl_info *info);
> +int mptcp_pm_nl_get_addr(u8 id, struct genl_info *info);
> +int mptcp_userspace_pm_get_addr(u8 id, struct genl_info *info);
>  
>  static inline u8 subflow_get_local_id(const struct mptcp_subflow_context *subflow)
>  {

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.