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

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

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
-- 
Sponsored by the NGI0 Core fund.