diff mbox series

[mptcp-next,v7,01/11] mptcp: pm: more precise error messages

Message ID 702733ca36c469e621a3d4e9173eeda587dba85c.1736150983.git.tanggeliang@kylinos.cn (mailing list archive)
State Superseded
Delegated to: Matthieu Baerts
Headers show
Series mptcp: use GENL_REQ_ATTR_CHECK in userspace pm | expand

Checks

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

Commit Message

Geliang Tang Jan. 6, 2025, 8:16 a.m. UTC 
From: "Matthieu Baerts (NGI0)" <matttbe@kernel.org>

Some errors reported by the userspace PM were vague: "this or that is
invalid".

It is easier for the userspace to know which part is wrong, instead of
having to guess that.

By splitting some error messages, NL_SET_ERR_MSG_ATTR() can be used
instead of GENL_SET_ERR_MSG() in order to give an additional hint to the
userspace developers about which attribute is wrong.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_userspace.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

Comments

Matthieu Baerts Jan. 6, 2025, 9:17 a.m. UTC | #1 
Hi Geliang,

On 06/01/2025 09:16, Geliang Tang wrote:
> From: "Matthieu Baerts (NGI0)" <matttbe@kernel.org>
> 
> Some errors reported by the userspace PM were vague: "this or that is
> invalid".
> 
> It is easier for the userspace to know which part is wrong, instead of
> having to guess that.
> 
> By splitting some error messages, NL_SET_ERR_MSG_ATTR() can be used
> instead of GENL_SET_ERR_MSG() in order to give an additional hint to the
> userspace developers about which attribute is wrong.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

If you modify the behaviour of a patch, you can add your 'co-dev' +
'sob' tags.

Also, please add an individual changelog, easier for the reviewers to
spot what has changed.

> ---
>  net/mptcp/pm_userspace.c | 39 +++++++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index a3d477059b11..40fd2f788196 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c

(...)

> @@ -579,17 +591,24 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
>  	if (ret < 0)
>  		goto set_flags_err;
>  
> +	if (loc.addr.family == AF_UNSPEC) {
> +		NL_SET_ERR_MSG_ATTR(info->extack, attr,
> +				    "invalid local address family");
> +		ret = -EINVAL;
> +		goto set_flags_err;
> +	}
> +
>  	if (attr_rem) {
>  		ret = mptcp_pm_parse_entry(attr_rem, info, false, &rem);
>  		if (ret < 0)
>  			goto set_flags_err;
> -	}
>  
> -	if (loc.addr.family == AF_UNSPEC ||
> -	    rem.addr.family == AF_UNSPEC) {
> -		GENL_SET_ERR_MSG(info, "invalid address families");
> -		ret = -EINVAL;
> -		goto set_flags_err;
> +		if (rem.addr.family == AF_UNSPEC) {

As I mentioned in a previous version, this is changing the behaviour:
this remote attribute is no longer mandatory. I'm still unsure about
making it optional, because mptcp_pm_nl_mp_prio_send_ack() will pick
only one subflow, not all the ones with the specified local address. It
looks better to keep it mandatory.

At least here, you should not move it under "if (attr_rem)".

(and maybe it makes more sense to have this patch after 07/11 ("mptcp:
pm: use NL_SET_ERR_MSG_ATTR when possible"), but that's a detail, fine here)

> +			NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
> +					    "invalid remote address family");
> +			ret = -EINVAL;
> +			goto set_flags_err;
> +		}
>  	}
>  
>  	if (loc.flags & MPTCP_PM_ADDR_FLAG_BACKUP)

Cheers,
Matt
diff mbox series

Patch

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index a3d477059b11..40fd2f788196 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -223,8 +223,14 @@  int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 		goto announce_err;
 	}
 
-	if (addr_val.addr.id == 0 || !(addr_val.flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
-		GENL_SET_ERR_MSG(info, "invalid addr id or flags");
+	if (addr_val.addr.id == 0) {
+		NL_SET_ERR_MSG_ATTR(info->extack, addr, "invalid addr id");
+		err = -EINVAL;
+		goto announce_err;
+	}
+
+	if (!(addr_val.flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
+		NL_SET_ERR_MSG_ATTR(info->extack, addr, "invalid addr flags");
 		err = -EINVAL;
 		goto announce_err;
 	}
@@ -530,8 +536,14 @@  int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
 		goto destroy_err;
 	}
 
-	if (!addr_l.addr.port || !addr_r.port) {
-		GENL_SET_ERR_MSG(info, "missing local or remote port");
+	if (!addr_l.addr.port) {
+		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "missing local port");
+		err = -EINVAL;
+		goto destroy_err;
+	}
+
+	if (!addr_r.port) {
+		NL_SET_ERR_MSG_ATTR(info->extack, raddr, "missing remote port");
 		err = -EINVAL;
 		goto destroy_err;
 	}
@@ -579,17 +591,24 @@  int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
 	if (ret < 0)
 		goto set_flags_err;
 
+	if (loc.addr.family == AF_UNSPEC) {
+		NL_SET_ERR_MSG_ATTR(info->extack, attr,
+				    "invalid local address family");
+		ret = -EINVAL;
+		goto set_flags_err;
+	}
+
 	if (attr_rem) {
 		ret = mptcp_pm_parse_entry(attr_rem, info, false, &rem);
 		if (ret < 0)
 			goto set_flags_err;
-	}
 
-	if (loc.addr.family == AF_UNSPEC ||
-	    rem.addr.family == AF_UNSPEC) {
-		GENL_SET_ERR_MSG(info, "invalid address families");
-		ret = -EINVAL;
-		goto set_flags_err;
+		if (rem.addr.family == AF_UNSPEC) {
+			NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
+					    "invalid remote address family");
+			ret = -EINVAL;
+			goto set_flags_err;
+		}
 	}
 
 	if (loc.flags & MPTCP_PM_ADDR_FLAG_BACKUP)