1. Patchew
  2. MPTCP
  3. [PATCH mptcp-next v6 0/7] mptcp: use GENL_REQ_ATTR_CHECK in userspace pm
  4. View patch

[PATCH mptcp-next v6 2/7] mptcp: pm: more precise error messages

Matthieu Baerts (NGI0) posted 7 patches 1 year, 1 month ago
There is a newer version of this series
[PATCH mptcp-next v6 2/7] mptcp: pm: more precise error messages
Posted by Matthieu Baerts (NGI0) 1 year, 1 month ago 
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 | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 72d4861e497eef5a516fc1c3ec1659807ffedd53..9c0bec588c498e0aa78acf4018db509abe90cad2 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -224,9 +224,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)) {
-		NL_SET_ERR_MSG_ATTR(info->extack, addr,
-				    "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;
 	}
@@ -537,8 +542,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;
 	}
@@ -592,9 +603,16 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
 			goto set_flags_err;
 	}
 
-	if (loc.addr.family == AF_UNSPEC ||
-	    rem.addr.family == AF_UNSPEC) {
-		GENL_SET_ERR_MSG(info, "invalid address families");
+	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 (rem.addr.family == AF_UNSPEC) {
+		NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
+				    "invalid remote address family");
 		ret = -EINVAL;
 		goto set_flags_err;
 	}

-- 
2.47.1
Re: [PATCH mptcp-next v6 2/7] mptcp: pm: more precise error messages
Posted by Geliang Tang 1 year, 1 month ago 
On Mon, 2024-12-30 at 14:24 +0100, Matthieu Baerts (NGI0) wrote:
> 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 | 34 ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index
> 72d4861e497eef5a516fc1c3ec1659807ffedd53..9c0bec588c498e0aa78acf4018d
> b509abe90cad2 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -224,9 +224,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)) {
> -		NL_SET_ERR_MSG_ATTR(info->extack, addr,
> -				    "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;
>  	}
> @@ -537,8 +542,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;
>  	}
> @@ -592,9 +603,16 @@ int mptcp_userspace_pm_set_flags(struct sk_buff
> *skb, struct genl_info *info)
>  			goto set_flags_err;
>  	}
>  
> -	if (loc.addr.family == AF_UNSPEC ||
> -	    rem.addr.family == AF_UNSPEC) {
> -		GENL_SET_ERR_MSG(info, "invalid address families");
> +	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 (rem.addr.family == AF_UNSPEC) {

I moved this check into "if (attr_rem) {" block in v7.

> +		NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
> +				    "invalid remote address
> family");
>  		ret = -EINVAL;
>  		goto set_flags_err;
>  	}
>
Re: [PATCH mptcp-next v6 2/7] mptcp: pm: more precise error messages
Posted by Matthieu Baerts 1 year, 1 month ago 
Hi Geliang,

On 06/01/2025 09:38, Geliang Tang wrote:
> On Mon, 2024-12-30 at 14:24 +0100, Matthieu Baerts (NGI0) wrote:
>> 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 | 34 ++++++++++++++++++++++++++--------
>>  1 file changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>> index
>> 72d4861e497eef5a516fc1c3ec1659807ffedd53..9c0bec588c498e0aa78acf4018d
>> b509abe90cad2 100644
>> --- a/net/mptcp/pm_userspace.c
>> +++ b/net/mptcp/pm_userspace.c

(...)

>> @@ -592,9 +603,16 @@ int mptcp_userspace_pm_set_flags(struct sk_buff
>> *skb, struct genl_info *info)
>>  			goto set_flags_err;
>>  	}
>>  
>> -	if (loc.addr.family == AF_UNSPEC ||
>> -	    rem.addr.family == AF_UNSPEC) {
>> -		GENL_SET_ERR_MSG(info, "invalid address families");
>> +	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 (rem.addr.family == AF_UNSPEC) {
> 
> I moved this check into "if (attr_rem) {" block in v7.

If you do that, you are changing the behaviour: this attribute is then
no longer mandatory with the userspace PM. I don't think we should do
that, or at least, not as part of a refactoring as you did.

If we want to change the behaviour, we need an explicit patch for that.
See my patch description from patch v6 4/7, and also the
GENL_REQ_ATTR_CHECK() check added in patch v6 7/7.

> 
>> +		NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
>> +				    "invalid remote address
>> family");
>>  		ret = -EINVAL;
>>  		goto set_flags_err;
>>  	}
>>
> 

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.