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

[Geliang Tang <geliang@kernel.org>]

On Sat, 2024-12-28 at 17:16 +0100, Matthieu Baerts wrote:
> 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.

Thanks Matt, that's much better. Please apply v4 for me then.

-Geliang

> 
> (...)
> 
> Cheers,
> Matt