[mptcp-next,v3] mptcp: use GENL_REQ_ATTR_CHECK for token

Commit Message

Geliang Tang Dec. 16, 2024, 8:31 a.m. UTC

From: Geliang Tang <[e-mail]>

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 <[e-mail]>'
Signed-off-by: Geliang Tang <[e-mail]>
---
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 | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

MPTCP CI Dec. 16, 2024, 9:29 a.m. UTC | #1

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/12348979008

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/3a1e74facb64
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=918117


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Matthieu Baerts Dec. 16, 2024, 11:24 a.m. UTC | #2

Hi Geliang,

On 16/12/2024 09:31, Geliang Tang wrote:
> From: Geliang Tang <[e-mail]>
> 
> 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 <[e-mail]>'
> Signed-off-by: Geliang Tang <[e-mail]>
> ---
> 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)

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
userspace app dev)? Or does it need to be modified to display the
missing argument set by GENL_REQ_ATTR_CHECK()?

Cheers,
Matt

Geliang Tang Dec. 20, 2024, 9:50 a.m. UTC | #3

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 <[e-mail]>
> > 
> > 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 <[e-mail]>'
> > Signed-off-by: Geliang Tang <[e-mail]>
> > ---
> > 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

Matthieu Baerts Dec. 21, 2024, 10:39 a.m. UTC | #4

Hi Geliang,

On 20/12/2024 10:50, Geliang Tang wrote:
> 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 <[e-mail]>
>>>
>>> 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 <[e-mail]>'
>>> Signed-off-by: Geliang Tang <[e-mail]>
>>> ---
>>> 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.

Thank you!

>> 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.

It might help developers if pm_nl_ctl could also print errors generated
by GENL_REQ_ATTR_CHECK(), not only the ones generated with
GENL_SET_ERR_MSG().

>> 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.

If GENL_REQ_ATTR_CHECK() is used, it should no longer needed to use
NL_SET_ERR_MSG_ATTR/GENL_SET_ERR_MSG. I guess the userspace should be
able to find the arguments that are missing, no?

This can be done in another series of course.

Cheers,
Matt

Patch

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 740a10d669f8..04405fc5a930 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -175,14 +175,13 @@  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;
+	struct nlattr *token;
 
-	if (!token) {
-		GENL_SET_ERR_MSG(info, "missing required token");
+	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_TOKEN))
 		return NULL;
-	}
 
+	token = info->attrs[MPTCP_PM_ATTR_TOKEN];
 	msk = mptcp_token_get_sock(genl_info_net(info), nla_get_u32(token));
 	if (!msk) {
 		NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token");