diff mbox series

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

Message ID ef87c8dd40c8dac4b946bf77e272d01ca64cc9dd.1734941967.git.tanggeliang@kylinos.cn (mailing list archive)
State Superseded
Headers show
Series [mptcp-next,v5] mptcp: use GENL_REQ_ATTR_CHECK in userspace pm | expand

Checks

Context Check Description
matttbe/build success Build and static analysis OK
matttbe/checkpatch warning total: 0 errors, 1 warnings, 0 checks, 163 lines checked
matttbe/shellcheck success MPTCP selftests files have not been modified
matttbe/KVM_Validation__normal success Success! ✅
matttbe/KVM_Validation__debug warning Unstable: 1 failed test(s): packetdrill_regressions
matttbe/KVM_Validation__btf-normal__only_bpftest_all_ success Success! ✅
matttbe/KVM_Validation__btf-debug__only_bpftest_all_ success Success! ✅

Commit Message

Geliang Tang Dec. 23, 2024, 8:24 a.m. UTC 
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)

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(-)

Comments

MPTCP CI Dec. 23, 2024, 9:17 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: Unstable: 1 failed test(s): packetdrill_regressions
Matthieu Baerts Dec. 28, 2024, 4:16 p.m. UTC | #2 
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
Geliang Tang Dec. 29, 2024, 12:22 a.m. UTC | #3 
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
diff mbox series

Patch

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");
 		return NULL;
 	}
 
@@ -200,16 +200,13 @@  static struct mptcp_sock *mptcp_userspace_pm_get_sock(const struct genl_info *in
 
 int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	struct nlattr *addr = info->attrs[MPTCP_PM_ATTR_ADDR];
 	struct mptcp_pm_addr_entry addr_val;
 	struct mptcp_sock *msk;
 	int err = -EINVAL;
 	struct sock *sk;
 
-	if (!addr) {
-		GENL_SET_ERR_MSG(info, "missing required address");
+	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR))
 		return err;
-	}
 
 	msk = mptcp_userspace_pm_get_sock(info);
 	if (!msk)
@@ -217,7 +214,8 @@  int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 
 	sk = (struct sock *)msk;
 
-	err = mptcp_pm_parse_entry(addr, info, true, &addr_val);
+	err = mptcp_pm_parse_entry(info->attrs[MPTCP_PM_ATTR_ADDR],
+				   info, true, &addr_val);
 	if (err < 0) {
 		GENL_SET_ERR_MSG(info, "error parsing local address");
 		goto announce_err;
@@ -309,19 +307,16 @@  void mptcp_pm_remove_addr_entry(struct mptcp_sock *msk,
 
 int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	struct nlattr *id = info->attrs[MPTCP_PM_ATTR_LOC_ID];
 	struct mptcp_pm_addr_entry *match;
 	struct mptcp_sock *msk;
 	int err = -EINVAL;
 	struct sock *sk;
 	u8 id_val;
 
-	if (!id) {
-		GENL_SET_ERR_MSG(info, "missing required ID");
+	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_LOC_ID))
 		return err;
-	}
 
-	id_val = nla_get_u8(id);
+	id_val = nla_get_u8(info->attrs[MPTCP_PM_ATTR_LOC_ID]);
 
 	msk = mptcp_userspace_pm_get_sock(info);
 	if (!msk)
@@ -362,8 +357,6 @@  int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 
 int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	struct nlattr *raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
-	struct nlattr *laddr = info->attrs[MPTCP_PM_ATTR_ADDR];
 	struct mptcp_pm_addr_entry entry = { 0 };
 	struct mptcp_addr_info addr_r;
 	struct mptcp_pm_local local;
@@ -371,10 +364,9 @@  int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
 	int err = -EINVAL;
 	struct sock *sk;
 
-	if (!laddr || !raddr) {
-		GENL_SET_ERR_MSG(info, "missing required address(es)");
+	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR) ||
+	    GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR_REMOTE))
 		return err;
-	}
 
 	msk = mptcp_userspace_pm_get_sock(info);
 	if (!msk)
@@ -382,9 +374,12 @@  int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
 
 	sk = (struct sock *)msk;
 
-	err = mptcp_pm_parse_entry(laddr, info, true, &entry);
+	err = mptcp_pm_parse_entry(info->attrs[MPTCP_PM_ATTR_ADDR],
+				   info, true, &entry);
 	if (err < 0) {
-		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error parsing local addr");
+		NL_SET_ERR_MSG_ATTR(info->extack,
+				    info->attrs[MPTCP_PM_ATTR_ADDR],
+				    "error parsing local addr");
 		goto create_err;
 	}
 
@@ -395,9 +390,12 @@  int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
 	}
 	entry.flags |= MPTCP_PM_ADDR_FLAG_SUBFLOW;
 
-	err = mptcp_pm_parse_addr(raddr, info, &addr_r);
+	err = mptcp_pm_parse_addr(info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE],
+				  info, &addr_r);
 	if (err < 0) {
-		NL_SET_ERR_MSG_ATTR(info->extack, raddr, "error parsing remote addr");
+		NL_SET_ERR_MSG_ATTR(info->extack,
+				    info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE],
+				    "error parsing remote addr");
 		goto create_err;
 	}
 
@@ -483,18 +481,15 @@  static struct sock *mptcp_nl_find_ssk(struct mptcp_sock *msk,
 
 int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	struct nlattr *raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
-	struct nlattr *laddr = info->attrs[MPTCP_PM_ATTR_ADDR];
 	struct mptcp_pm_addr_entry addr_l;
 	struct mptcp_addr_info addr_r;
 	struct mptcp_sock *msk;
 	struct sock *sk, *ssk;
 	int err = -EINVAL;
 
-	if (!laddr || !raddr) {
-		GENL_SET_ERR_MSG(info, "missing required address(es)");
+	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR) ||
+	    GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR_REMOTE))
 		return err;
-	}
 
 	msk = mptcp_userspace_pm_get_sock(info);
 	if (!msk)
@@ -502,15 +497,21 @@  int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
 
 	sk = (struct sock *)msk;
 
-	err = mptcp_pm_parse_entry(laddr, info, true, &addr_l);
+	err = mptcp_pm_parse_entry(info->attrs[MPTCP_PM_ATTR_ADDR],
+				   info, true, &addr_l);
 	if (err < 0) {
-		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error parsing local addr");
+		NL_SET_ERR_MSG_ATTR(info->extack,
+				    info->attrs[MPTCP_PM_ATTR_ADDR],
+				    "error parsing local addr");
 		goto destroy_err;
 	}
 
-	err = mptcp_pm_parse_addr(raddr, info, &addr_r);
+	err = mptcp_pm_parse_addr(info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE],
+				  info, &addr_r);
 	if (err < 0) {
-		NL_SET_ERR_MSG_ATTR(info->extack, raddr, "error parsing remote addr");
+		NL_SET_ERR_MSG_ATTR(info->extack,
+				    info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE],
+				    "error parsing remote addr");
 		goto destroy_err;
 	}