Re: [PATCH mptcp-next v3] mptcp: use GENL_REQ_ATTR_CHECK for token

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Mon, 2024-12-16 at 12:24 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 16/12/2024 09:31, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > A more general way to check if MPTCP_PM_ATTR_TOKEN exists in 'info'
> > is to use GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_TOKEN) instead of
> > directly reading info->attrs[MPTCP_PM_ATTR_TOKEN] and then checking
> > if it's NULL.
> > 
> > So this patch uses GENL_REQ_ATTR_CHECK() for 'token' in 'info' in
> > mptcp_userspace_pm_get_sock().
> > 
> > 'Suggested-by: Jakub Kicinski <kuba@kernel.org>'
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> > v3:
> >  - use GENL_REQ_ATTR_CHECK in mptcp_userspace_pm_get_sock only
> >  - drop GENL_SET_ERR_MSG as Matt suggested (thanks)
> 
> Thank you for the v3. While at it, do you want to do a similar change
> everywhere we do:
> 
>   struct nlattr *X = info->attrs[MPTCP_PM_ATTR_Y]:
>   if (!X) {
>       GENL_SET_ERR_MSG(info, "missing ...");
>       return Z;
>   }
> 
> e.g. in mptcp_pm_nl_announce_doit() (addr), mptcp_pm_nl_remove_doit()
> (id), mptcp_pm_nl_subflow_create_doit() (raddr, laddr),
> mptcp_pm_nl_subflow_destroy_doit() (raddr, laddr).
> (and maybe mptcp_pm_parse_pm_addr_attr(), but it needs more changes
> around to pass the attribute ID, probably best not to change that)

Thanks for this suggestion, I updated this in v4.

> 
> Also, do you know if 'pm_nl_ctl' will continue to indicate a helpful
> message if such attribute is missing (that's a message more for

'pm_nl_ctl' can't display nl error msg, this is a bug in nl_error():

 $ sudo ip mptcp endpoint del id 100
 Error: address not found.
 $ sudo ./pm_nl_ctl del 100
 netlink error -22 (Invalid argument)
 ./pm_nl_ctl: bailing out due to netlink error[s]

Look at this test, nl error msg "address not found" in
mptcp_pm_nl_del_addr_doit() can be displayed in 'ip mptcp', but not in
'pm_nl_ctl'. I'll try to fix it.

> userspace app dev)? Or does it need to be modified to display the
> missing argument set by GENL_REQ_ATTR_CHECK()?

I think no need to modify this, since after GENL_REQ_ATTR_CHECK(), we
will also call NL_SET_ERR_MSG_ATTR/GENL_SET_ERR_MSG to set nl error msg
in the error paths.

Thanks,
-Geliang

> 
> Cheers,
> Matt