Re: [PATCH mptcp-next v5] mptcp: use GENL_REQ_ATTR_CHECK in userspace pm

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 23/12/2024 09:24, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> A more general way to check if MPTCP_PM_ATTR_* exists in 'info'
> is to use GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_*) instead of
> directly reading info->attrs[MPTCP_PM_ATTR_*] and then checking
> if it's NULL.
> 
> So this patch uses GENL_REQ_ATTR_CHECK() for userspace PM in
> mptcp_pm_nl_announce_doit(), mptcp_pm_nl_remove_doit(),
> mptcp_pm_nl_subflow_create_doit(), mptcp_pm_nl_subflow_destroy_doit()
> and mptcp_userspace_pm_get_sock().
> 
> 'Suggested-by: Jakub Kicinski <kuba@kernel.org>'
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> v5:
>  - drop 'struct nlattr *' variables as Matt suggested (thanks)

Thank you for the v5!

> v4:
>  - use GENL_REQ_ATTR_CHECK in more functions as Matt suggested (thanks)
> 
> v3:
>  - use GENL_REQ_ATTR_CHECK in mptcp_userspace_pm_get_sock only
>  - drop GENL_SET_ERR_MSG as Matt suggested (thanks)
> 
> v2:
>  - use GENL_REQ_ATTR_CHECK in get_addr(), dump_addr() and set_flags()
>    too.
> ---
>  net/mptcp/pm_userspace.c | 69 ++++++++++++++++++++--------------------
>  1 file changed, 35 insertions(+), 34 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 740a10d669f8..42dda8877372 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -175,17 +175,17 @@ bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk,
>  
>  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;
>  
> -	if (!token) {
> -		GENL_SET_ERR_MSG(info, "missing required token");
> +	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_TOKEN))
>  		return NULL;
> -	}
>  
> -	msk = mptcp_token_get_sock(genl_info_net(info), nla_get_u32(token));
> +	msk = mptcp_token_get_sock(genl_info_net(info),
> +				   nla_get_u32(info->attrs[MPTCP_PM_ATTR_TOKEN]));
>  	if (!msk) {
> -		NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token");
> +		NL_SET_ERR_MSG_ATTR(info->extack,
> +				    info->attrs[MPTCP_PM_ATTR_TOKEN],
> +				    "invalid token");

Arf, sorry, I didn't notice the 'struct nlattr *' variables were used in
multiple places: I guess I only looked at mptcp_pm_nl_remove_doit()
where in the v4, we had:

 	id = info->attrs[MPTCP_PM_ATTR_LOC_ID];
 	id_val = nla_get_u8(id);

... and 'id' was only used there. But maybe that's not normal to use
'id' only once: probably because we used GENL_SET_ERR_MSG() instead of
NL_SET_ERR_MSG_ATTR() which seems better, no?
(same in mptcp_pm_nl_announce_doit())

I can send a quick patch for that, and place your v4 on top of it.

(...)

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