[PATCH mptcp-next v6 0/7] mptcp: use GENL_REQ_ATTR_CHECK in userspace pm

[PATCH mptcp-next v6 0/7] mptcp: use GENL_REQ_ATTR_CHECK in userspace pm

[Matthieu Baerts (NGI0)]

This is a newer version of the series started by Geliang, now making
them more uniform on how the NL errors are set, and giving more hints to
the userspace about what's wrong.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Geliang Tang (1):
      mptcp: pm: userspace: use GENL_REQ_ATTR_CHECK

Matthieu Baerts (NGI0) (6):
      mptcp: pm: use NL_SET_ERR_MSG_ATTR when possible
      mptcp: pm: more precise error messages
      mptcp: pm: improve error messages
      mptcp: pm: userspace: flags: clearer msg if no remote addr
      mptcp: pm: remove duplicated error messages
      mptcp: pm: mark missing address attributes

 net/mptcp/pm_netlink.c   |  50 +++++++++++-----
 net/mptcp/pm_userspace.c | 144 ++++++++++++++++++++++++++++-------------------
 2 files changed, 121 insertions(+), 73 deletions(-)
---
base-commit: f770d70d123db1907d419f715117245d7060816b
change-id: 20241228-genl_req_attr_check-2accaa49f84f

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

[PATCH mptcp-next v6 1/7] mptcp: pm: use NL_SET_ERR_MSG_ATTR when possible

[Matthieu Baerts (NGI0)]

Instead of only returning a text message with GENL_SET_ERR_MSG(),
NL_SET_ERR_MSG_ATTR() can help the userspace developers by also
reporting which attribute is faulty.

When the error is specific to an attribute, NL_SET_ERR_MSG_ATTR() is now
used. The error messages have not been modified in this commit.

mptcp_userspace_pm_remove_id_zero_address() has been modified to set the
missing attribute.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_netlink.c   | 23 ++++++++++++++---------
 net/mptcp/pm_userspace.c | 27 +++++++++++++++++----------
 2 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 98ac73938bd8196e196d5ee8c264784ba8d37645..49c34494e156ff6c3277b107c3840ee29dd3afbb 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1403,18 +1403,21 @@ int mptcp_pm_nl_add_addr_doit(struct sk_buff *skb, struct genl_info *info)
 		return ret;
 
 	if (addr.addr.port && !address_use_port(&addr)) {
-		GENL_SET_ERR_MSG(info, "flags must have signal and not subflow when using port");
+		NL_SET_ERR_MSG_ATTR(info->extack, attr,
+				    "flags must have signal and not subflow when using port");
 		return -EINVAL;
 	}
 
 	if (addr.flags & MPTCP_PM_ADDR_FLAG_SIGNAL &&
 	    addr.flags & MPTCP_PM_ADDR_FLAG_FULLMESH) {
-		GENL_SET_ERR_MSG(info, "flags mustn't have both signal and fullmesh");
+		NL_SET_ERR_MSG_ATTR(info->extack, attr,
+				    "flags mustn't have both signal and fullmesh");
 		return -EINVAL;
 	}
 
 	if (addr.flags & MPTCP_PM_ADDR_FLAG_IMPLICIT) {
-		GENL_SET_ERR_MSG(info, "can't create IMPLICIT endpoint");
+		NL_SET_ERR_MSG_ATTR(info->extack, attr,
+				    "can't create IMPLICIT endpoint");
 		return -EINVAL;
 	}
 
@@ -1608,7 +1611,7 @@ int mptcp_pm_nl_del_addr_doit(struct sk_buff *skb, struct genl_info *info)
 	spin_lock_bh(&pernet->lock);
 	entry = __lookup_addr_by_id(pernet, addr.addr.id);
 	if (!entry) {
-		GENL_SET_ERR_MSG(info, "address not found");
+		NL_SET_ERR_MSG_ATTR(info->extack, attr, "address not found");
 		spin_unlock_bh(&pernet->lock);
 		return -EINVAL;
 	}
@@ -1790,7 +1793,7 @@ int mptcp_pm_nl_get_addr(struct sk_buff *skb, struct genl_info *info)
 	rcu_read_lock();
 	entry = __lookup_addr_by_id(pernet, addr.addr.id);
 	if (!entry) {
-		GENL_SET_ERR_MSG(info, "address not found");
+		NL_SET_ERR_MSG_ATTR(info->extack, attr, "address not found");
 		ret = -EINVAL;
 		goto unlock_fail;
 	}
@@ -1875,7 +1878,8 @@ static int parse_limit(struct genl_info *info, int id, unsigned int *limit)
 
 	*limit = nla_get_u32(attr);
 	if (*limit > MPTCP_PM_ADDR_MAX) {
-		GENL_SET_ERR_MSG(info, "limit greater than maximum");
+		NL_SET_ERR_MSG_ATTR(info->extack, attr,
+				    "limit greater than maximum");
 		return -EINVAL;
 	}
 	return 0;
@@ -2003,7 +2007,8 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
 	if (addr.addr.family == AF_UNSPEC) {
 		lookup_by_id = 1;
 		if (!addr.addr.id) {
-			GENL_SET_ERR_MSG(info, "missing required inputs");
+			NL_SET_ERR_MSG_ATTR(info->extack, attr,
+					    "missing required inputs");
 			return -EOPNOTSUPP;
 		}
 	}
@@ -2016,13 +2021,13 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
 			       __lookup_addr(pernet, &addr.addr);
 	if (!entry) {
 		spin_unlock_bh(&pernet->lock);
-		GENL_SET_ERR_MSG(info, "address not found");
+		NL_SET_ERR_MSG_ATTR(info->extack, attr, "address not found");
 		return -EINVAL;
 	}
 	if ((addr.flags & MPTCP_PM_ADDR_FLAG_FULLMESH) &&
 	    (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
 		spin_unlock_bh(&pernet->lock);
-		GENL_SET_ERR_MSG(info, "invalid addr flags");
+		NL_SET_ERR_MSG_ATTR(info->extack, attr, "invalid addr flags");
 		return -EINVAL;
 	}
 
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index a3d477059b11c3a5618dbb6256434a8e55845995..72d4861e497eef5a516fc1c3ec1659807ffedd53 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -190,7 +190,8 @@ static struct mptcp_sock *mptcp_userspace_pm_get_sock(const struct genl_info *in
 	}
 
 	if (!mptcp_pm_is_userspace(msk)) {
-		GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected");
+		NL_SET_ERR_MSG_ATTR(info->extack, token,
+				    "invalid request; userspace PM not selected");
 		sock_put((struct sock *)msk);
 		return NULL;
 	}
@@ -224,14 +225,16 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	if (addr_val.addr.id == 0 || !(addr_val.flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
-		GENL_SET_ERR_MSG(info, "invalid addr id or flags");
+		NL_SET_ERR_MSG_ATTR(info->extack, addr,
+				    "invalid addr id or flags");
 		err = -EINVAL;
 		goto announce_err;
 	}
 
 	err = mptcp_userspace_pm_append_new_local_addr(msk, &addr_val, false);
 	if (err < 0) {
-		GENL_SET_ERR_MSG(info, "did not match address and id");
+		NL_SET_ERR_MSG_ATTR(info->extack, addr,
+				    "did not match address and id");
 		goto announce_err;
 	}
 
@@ -254,7 +257,8 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 }
 
 static int mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock *msk,
-						     struct genl_info *info)
+						     struct genl_info *info,
+						     struct nlattr *attr)
 {
 	struct mptcp_rm_list list = { .nr = 0 };
 	struct mptcp_subflow_context *subflow;
@@ -270,7 +274,8 @@ static int mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock *msk,
 		}
 	}
 	if (!has_id_0) {
-		GENL_SET_ERR_MSG(info, "address with id 0 not found");
+		NL_SET_ERR_MSG_ATTR(info->extack, attr,
+				    "address with id 0 not found");
 		goto remove_err;
 	}
 
@@ -330,7 +335,7 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 	sk = (struct sock *)msk;
 
 	if (id_val == 0) {
-		err = mptcp_userspace_pm_remove_id_zero_address(msk, info);
+		err = mptcp_userspace_pm_remove_id_zero_address(msk, info, id);
 		goto out;
 	}
 
@@ -339,7 +344,8 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 	spin_lock_bh(&msk->pm.lock);
 	match = mptcp_userspace_pm_lookup_addr_by_id(msk, id_val);
 	if (!match) {
-		GENL_SET_ERR_MSG(info, "address with specified id not found");
+		NL_SET_ERR_MSG_ATTR(info->extack, id,
+				    "address with specified id not found");
 		spin_unlock_bh(&msk->pm.lock);
 		release_sock(sk);
 		goto out;
@@ -389,7 +395,7 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	if (entry.flags & MPTCP_PM_ADDR_FLAG_SIGNAL) {
-		GENL_SET_ERR_MSG(info, "invalid addr flags");
+		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "invalid addr flags");
 		err = -EINVAL;
 		goto create_err;
 	}
@@ -409,7 +415,8 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
 
 	err = mptcp_userspace_pm_append_new_local_addr(msk, &entry, false);
 	if (err < 0) {
-		GENL_SET_ERR_MSG(info, "did not match address and id");
+		NL_SET_ERR_MSG_ATTR(info->extack, laddr,
+				    "did not match address and id");
 		goto create_err;
 	}
 
@@ -702,7 +709,7 @@ int mptcp_userspace_pm_get_addr(struct sk_buff *skb,
 	spin_lock_bh(&msk->pm.lock);
 	entry = mptcp_userspace_pm_lookup_addr_by_id(msk, addr.addr.id);
 	if (!entry) {
-		GENL_SET_ERR_MSG(info, "address not found");
+		NL_SET_ERR_MSG_ATTR(info->extack, attr, "address not found");
 		ret = -EINVAL;
 		goto unlock_fail;
 	}

-- 
2.47.1

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

[Matthieu Baerts (NGI0)]

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

[PATCH mptcp-next v6 3/7] mptcp: pm: improve error messages

[Matthieu Baerts (NGI0)]

Some error messages were:

 - too generic: "missing input", "invalid request"

 - not precise enough: "limit greater than maximum" but what's the max?

 - missing: subflow not found, or connect error.

This can be easily improved by being more precise, or adding new error
messages.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_netlink.c   | 7 ++++---
 net/mptcp/pm_userspace.c | 9 ++++++++-
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 49c34494e156ff6c3277b107c3840ee29dd3afbb..3bb213e2a967e87d4244dc0f1ba58d3ba21fb371 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1878,8 +1878,9 @@ static int parse_limit(struct genl_info *info, int id, unsigned int *limit)
 
 	*limit = nla_get_u32(attr);
 	if (*limit > MPTCP_PM_ADDR_MAX) {
-		NL_SET_ERR_MSG_ATTR(info->extack, attr,
-				    "limit greater than maximum");
+		NL_SET_ERR_MSG_ATTR_FMT(info->extack, attr,
+					"limit greater than maximum (%u)",
+					MPTCP_PM_ADDR_MAX);
 		return -EINVAL;
 	}
 	return 0;
@@ -2008,7 +2009,7 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
 		lookup_by_id = 1;
 		if (!addr.addr.id) {
 			NL_SET_ERR_MSG_ATTR(info->extack, attr,
-					    "missing required inputs");
+					    "missing address ID");
 			return -EOPNOTSUPP;
 		}
 	}
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 9c0bec588c498e0aa78acf4018db509abe90cad2..a8f73b082460ba00f7fa998cf7646368f8201e2e 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -191,7 +191,7 @@ static struct mptcp_sock *mptcp_userspace_pm_get_sock(const struct genl_info *in
 
 	if (!mptcp_pm_is_userspace(msk)) {
 		NL_SET_ERR_MSG_ATTR(info->extack, token,
-				    "invalid request; userspace PM not selected");
+				    "userspace PM not selected");
 		sock_put((struct sock *)msk);
 		return NULL;
 	}
@@ -433,6 +433,9 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
 	err = __mptcp_subflow_connect(sk, &local, &addr_r);
 	release_sock(sk);
 
+	if (err)
+		GENL_SET_ERR_MSG_FMT(info, "connect error: %d", err);
+
 	spin_lock_bh(&msk->pm.lock);
 	if (err)
 		mptcp_userspace_pm_delete_local_addr(msk, &entry);
@@ -557,6 +560,7 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
 	lock_sock(sk);
 	ssk = mptcp_nl_find_ssk(msk, &addr_l.addr, &addr_r);
 	if (!ssk) {
+		GENL_SET_ERR_MSG(info, "subflow not found");
 		err = -ESRCH;
 		goto release_sock;
 	}
@@ -634,6 +638,9 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
 	ret = mptcp_pm_nl_mp_prio_send_ack(msk, &loc.addr, &rem.addr, bkup);
 	release_sock(sk);
 
+	if (ret)
+		GENL_SET_ERR_MSG(info, "subflow not found");
+
 set_flags_err:
 	sock_put(sk);
 	return ret;

-- 
2.47.1

[PATCH mptcp-next v6 4/7] mptcp: pm: userspace: flags: clearer msg if no remote addr

[Matthieu Baerts (NGI0)]

Since its introduction in commit 892f396c8e68 ("mptcp: netlink: issue
MP_PRIO signals from userspace PMs"), it was mandatory to specify the
remote address, because of the 'if (rem->addr.family == AF_UNSPEC)'
check done later one.

In theory, this attribute can be optional, but it sounds better to be
precise to avoid sending the MP_PRIO on the wrong subflow, e.g. if there
are multiple subflows attached to the same local ID. This can be relaxed
later on if there is a need to act on multiple subflows with one
command.

For the moment, the check to see if attr_rem is NULL can be removed,
because mptcp_pm_parse_entry() will do this check as well, no need to do
that differently here.

While at it, move the parsing after the check linked to the local
attribute.

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

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index a8f73b082460ba00f7fa998cf7646368f8201e2e..2f82ac49db35ec34dcc0b9208f4ac12edc6ab254 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -601,12 +601,6 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
 	if (ret < 0)
 		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) {
 		NL_SET_ERR_MSG_ATTR(info->extack, attr,
 				    "invalid local address family");
@@ -614,6 +608,10 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
 		goto set_flags_err;
 	}
 
+	ret = mptcp_pm_parse_entry(attr_rem, info, false, &rem);
+	if (ret < 0)
+		goto set_flags_err;
+
 	if (rem.addr.family == AF_UNSPEC) {
 		NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
 				    "invalid remote address family");

-- 
2.47.1

[PATCH mptcp-next v6 5/7] mptcp: pm: userspace: use GENL_REQ_ATTR_CHECK

[Matthieu Baerts (NGI0)]

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>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_userspace.c | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 2f82ac49db35ec34dcc0b9208f4ac12edc6ab254..d6595cf3d8f8f9d4e2fd51505032ed2dc894738c 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");
@@ -201,16 +200,14 @@ 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;
+	struct nlattr *addr;
 	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)
@@ -218,6 +215,7 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 
 	sk = (struct sock *)msk;
 
+	addr = info->attrs[MPTCP_PM_ATTR_ADDR];
 	err = mptcp_pm_parse_entry(addr, info, true, &addr_val);
 	if (err < 0) {
 		GENL_SET_ERR_MSG(info, "error parsing local address");
@@ -319,18 +317,17 @@ 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;
+	struct nlattr *id;
 	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 = info->attrs[MPTCP_PM_ATTR_LOC_ID];
 	id_val = nla_get_u8(id);
 
 	msk = mptcp_userspace_pm_get_sock(info);
@@ -373,19 +370,17 @@ 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 nlattr *raddr, *laddr;
 	struct mptcp_pm_local local;
 	struct mptcp_sock *msk;
 	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)
@@ -393,6 +388,7 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
 
 	sk = (struct sock *)msk;
 
+	laddr = info->attrs[MPTCP_PM_ATTR_ADDR];
 	err = mptcp_pm_parse_entry(laddr, info, true, &entry);
 	if (err < 0) {
 		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error parsing local addr");
@@ -406,6 +402,7 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
 	}
 	entry.flags |= MPTCP_PM_ADDR_FLAG_SUBFLOW;
 
+	raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
 	err = mptcp_pm_parse_addr(raddr, info, &addr_r);
 	if (err < 0) {
 		NL_SET_ERR_MSG_ATTR(info->extack, raddr, "error parsing remote addr");
@@ -498,18 +495,16 @@ 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 nlattr *raddr, *laddr;
 	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)
@@ -517,12 +512,14 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
 
 	sk = (struct sock *)msk;
 
+	laddr = info->attrs[MPTCP_PM_ATTR_ADDR];
 	err = mptcp_pm_parse_entry(laddr, info, true, &addr_l);
 	if (err < 0) {
 		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error parsing local addr");
 		goto destroy_err;
 	}
 
+	raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
 	err = mptcp_pm_parse_addr(raddr, info, &addr_r);
 	if (err < 0) {
 		NL_SET_ERR_MSG_ATTR(info->extack, raddr, "error parsing remote addr");

-- 
2.47.1

[PATCH mptcp-next v6 6/7] mptcp: pm: remove duplicated error messages

[Matthieu Baerts (NGI0)]

mptcp_pm_parse_entry() and mptcp_pm_parse_addr() will already set a
error message in case of parsing issue.

Then, no need to override this error message with another less precise
one: "error parsing address".

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

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index d6595cf3d8f8f9d4e2fd51505032ed2dc894738c..51943ca6ddb607e1dfea393525cccf5cbed7cfac 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -217,10 +217,8 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 
 	addr = info->attrs[MPTCP_PM_ATTR_ADDR];
 	err = mptcp_pm_parse_entry(addr, info, true, &addr_val);
-	if (err < 0) {
-		GENL_SET_ERR_MSG(info, "error parsing local address");
+	if (err < 0)
 		goto announce_err;
-	}
 
 	if (addr_val.addr.id == 0) {
 		NL_SET_ERR_MSG_ATTR(info->extack, addr, "invalid addr id");
@@ -390,10 +388,8 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
 
 	laddr = info->attrs[MPTCP_PM_ATTR_ADDR];
 	err = mptcp_pm_parse_entry(laddr, info, true, &entry);
-	if (err < 0) {
-		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error parsing local addr");
+	if (err < 0)
 		goto create_err;
-	}
 
 	if (entry.flags & MPTCP_PM_ADDR_FLAG_SIGNAL) {
 		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "invalid addr flags");
@@ -404,10 +400,8 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
 
 	raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
 	err = mptcp_pm_parse_addr(raddr, info, &addr_r);
-	if (err < 0) {
-		NL_SET_ERR_MSG_ATTR(info->extack, raddr, "error parsing remote addr");
+	if (err < 0)
 		goto create_err;
-	}
 
 	if (!mptcp_pm_addr_families_match(sk, &entry.addr, &addr_r)) {
 		GENL_SET_ERR_MSG(info, "families mismatch");
@@ -514,17 +508,13 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
 
 	laddr = info->attrs[MPTCP_PM_ATTR_ADDR];
 	err = mptcp_pm_parse_entry(laddr, info, true, &addr_l);
-	if (err < 0) {
-		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "error parsing local addr");
+	if (err < 0)
 		goto destroy_err;
-	}
 
 	raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
 	err = mptcp_pm_parse_addr(raddr, info, &addr_r);
-	if (err < 0) {
-		NL_SET_ERR_MSG_ATTR(info->extack, raddr, "error parsing remote addr");
+	if (err < 0)
 		goto destroy_err;
-	}
 
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
 	if (addr_l.addr.family == AF_INET && ipv6_addr_v4mapped(&addr_r.addr6)) {

-- 
2.47.1

[PATCH mptcp-next v6 7/7] mptcp: pm: mark missing address attributes

[Matthieu Baerts (NGI0)]

mptcp_pm_parse_entry() will check if the given attribute is defined. If
not, it will return a generic error: "missing address info".

It might then not be clear for the userspace developer which attribute
is missing, especially when the command takes multiple addresses.

By using GENL_REQ_ATTR_CHECK(), the userspace will get a hint about
which attribute is missing, making thing clearer. Note that this is what
was already done for most of the other MPTCP NL commands, this patch
simply adds the missing ones.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_netlink.c   | 26 +++++++++++++++++++++-----
 net/mptcp/pm_userspace.c | 15 ++++++++++++---
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 3bb213e2a967e87d4244dc0f1ba58d3ba21fb371..1eaaf91eb978b5e85dd5fc11d616ba48809e2915 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1393,11 +1393,15 @@ static bool mptcp_pm_has_addr_attr_id(const struct nlattr *attr,
 
 int mptcp_pm_nl_add_addr_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	struct nlattr *attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
 	struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
 	struct mptcp_pm_addr_entry addr, *entry;
+	struct nlattr *attr;
 	int ret;
 
+	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ENDPOINT_ADDR))
+		return -EINVAL;
+
+	attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
 	ret = mptcp_pm_parse_entry(attr, info, true, &addr);
 	if (ret < 0)
 		return ret;
@@ -1590,12 +1594,16 @@ static int mptcp_nl_remove_id_zero_address(struct net *net,
 
 int mptcp_pm_nl_del_addr_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	struct nlattr *attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
 	struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
 	struct mptcp_pm_addr_entry addr, *entry;
 	unsigned int addr_max;
+	struct nlattr *attr;
 	int ret;
 
+	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ENDPOINT_ADDR))
+		return -EINVAL;
+
+	attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
 	ret = mptcp_pm_parse_entry(attr, info, false, &addr);
 	if (ret < 0)
 		return ret;
@@ -1767,13 +1775,17 @@ int mptcp_nl_fill_addr(struct sk_buff *skb,
 
 int mptcp_pm_nl_get_addr(struct sk_buff *skb, struct genl_info *info)
 {
-	struct nlattr *attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
 	struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
 	struct mptcp_pm_addr_entry addr, *entry;
 	struct sk_buff *msg;
+	struct nlattr *attr;
 	void *reply;
 	int ret;
 
+	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ENDPOINT_ADDR))
+		return -EINVAL;
+
+	attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
 	ret = mptcp_pm_parse_entry(attr, info, false, &addr);
 	if (ret < 0)
 		return ret;
@@ -1989,18 +2001,20 @@ static int mptcp_nl_set_flags(struct net *net,
 int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
 {
 	struct mptcp_pm_addr_entry addr = { .addr = { .family = AF_UNSPEC }, };
-	struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
 	u8 changed, mask = MPTCP_PM_ADDR_FLAG_BACKUP |
 			   MPTCP_PM_ADDR_FLAG_FULLMESH;
 	struct net *net = sock_net(skb->sk);
 	struct mptcp_pm_addr_entry *entry;
 	struct pm_nl_pernet *pernet;
+	struct nlattr *attr;
 	u8 lookup_by_id = 0;
 	u8 bkup = 0;
 	int ret;
 
-	pernet = pm_nl_get_pernet(net);
+	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR))
+		return -EINVAL;
 
+	attr = info->attrs[MPTCP_PM_ATTR_ADDR];
 	ret = mptcp_pm_parse_entry(attr, info, false, &addr);
 	if (ret < 0)
 		return ret;
@@ -2017,6 +2031,8 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
 	if (addr.flags & MPTCP_PM_ADDR_FLAG_BACKUP)
 		bkup = 1;
 
+	pernet = pm_nl_get_pernet(net);
+
 	spin_lock_bh(&pernet->lock);
 	entry = lookup_by_id ? __lookup_addr_by_id(pernet, addr.addr.id) :
 			       __lookup_addr(pernet, &addr.addr);
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 51943ca6ddb607e1dfea393525cccf5cbed7cfac..f5515b2305f41fafa7ef0627ed838b9d0db14aa9 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -570,20 +570,24 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
 {
 	struct mptcp_pm_addr_entry loc = { .addr = { .family = AF_UNSPEC }, };
 	struct mptcp_pm_addr_entry rem = { .addr = { .family = AF_UNSPEC }, };
-	struct nlattr *attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
-	struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
 	struct mptcp_pm_addr_entry *entry;
+	struct nlattr *attr, *attr_rem;
 	struct mptcp_sock *msk;
 	int ret = -EINVAL;
 	struct sock *sk;
 	u8 bkup = 0;
 
+	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR) ||
+	    GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR_REMOTE))
+		return ret;
+
 	msk = mptcp_userspace_pm_get_sock(info);
 	if (!msk)
 		return ret;
 
 	sk = (struct sock *)msk;
 
+	attr = info->attrs[MPTCP_PM_ATTR_ADDR];
 	ret = mptcp_pm_parse_entry(attr, info, false, &loc);
 	if (ret < 0)
 		goto set_flags_err;
@@ -595,6 +599,7 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
 		goto set_flags_err;
 	}
 
+	attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
 	ret = mptcp_pm_parse_entry(attr_rem, info, false, &rem);
 	if (ret < 0)
 		goto set_flags_err;
@@ -683,20 +688,24 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
 int mptcp_userspace_pm_get_addr(struct sk_buff *skb,
 				struct genl_info *info)
 {
-	struct nlattr *attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
 	struct mptcp_pm_addr_entry addr, *entry;
 	struct mptcp_sock *msk;
 	struct sk_buff *msg;
+	struct nlattr *attr;
 	int ret = -EINVAL;
 	struct sock *sk;
 	void *reply;
 
+	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ENDPOINT_ADDR))
+		return ret;
+
 	msk = mptcp_userspace_pm_get_sock(info);
 	if (!msk)
 		return ret;
 
 	sk = (struct sock *)msk;
 
+	attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
 	ret = mptcp_pm_parse_entry(attr, info, false, &addr);
 	if (ret < 0)
 		goto out;

-- 
2.47.1

Re: [PATCH mptcp-next v6 0/7] mptcp: use GENL_REQ_ATTR_CHECK in userspace pm

[MPTCP CI]

Hi Matthieu,

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): Unstable: 1 failed test(s): bpftest_test_progs-no_alu32_mptcp 🔴
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/12547335119

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


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)

Re: [PATCH mptcp-next v6 0/7] mptcp: use GENL_REQ_ATTR_CHECK in userspace pm

[Geliang Tang]

Hi Matt,

Thanks for this new version.

On Mon, 2024-12-30 at 14:24 +0100, Matthieu Baerts (NGI0) wrote:
> This is a newer version of the series started by Geliang, now making
> them more uniform on how the NL errors are set, and giving more hints
> to
> the userspace about what's wrong.

This series is useful, but it conflicts with "BPF path manager" series
under review, especially with set_flags() interface. So I also added
the patches to update set_flags interface into v7.

> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Geliang Tang (1):
>       mptcp: pm: userspace: use GENL_REQ_ATTR_CHECK
> 
> Matthieu Baerts (NGI0) (6):
>       mptcp: pm: use NL_SET_ERR_MSG_ATTR when possible
>       mptcp: pm: more precise error messages
>       mptcp: pm: improve error messages
>       mptcp: pm: userspace: flags: clearer msg if no remote addr
>       mptcp: pm: remove duplicated error messages
>       mptcp: pm: mark missing address attributes
> 
>  net/mptcp/pm_netlink.c   |  50 +++++++++++-----
>  net/mptcp/pm_userspace.c | 144 ++++++++++++++++++++++++++++---------
> ----------
>  2 files changed, 121 insertions(+), 73 deletions(-)
> ---
> base-commit: f770d70d123db1907d419f715117245d7060816b
> change-id: 20241228-genl_req_attr_check-2accaa49f84f
> 
> Best regards,

Re: [PATCH mptcp-next v6 1/7] mptcp: pm: use NL_SET_ERR_MSG_ATTR when possible

[Geliang Tang]

On Mon, 2024-12-30 at 14:24 +0100, Matthieu Baerts (NGI0) wrote:
> Instead of only returning a text message with GENL_SET_ERR_MSG(),
> NL_SET_ERR_MSG_ATTR() can help the userspace developers by also
> reporting which attribute is faulty.
> 
> When the error is specific to an attribute, NL_SET_ERR_MSG_ATTR() is
> now
> used. The error messages have not been modified in this commit.
> 
> mptcp_userspace_pm_remove_id_zero_address() has been modified to set

A new patch to drop "info" parameter of this function is added in v7.

> the
> missing attribute.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  net/mptcp/pm_netlink.c   | 23 ++++++++++++++---------
>  net/mptcp/pm_userspace.c | 27 +++++++++++++++++----------
>  2 files changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index
> 98ac73938bd8196e196d5ee8c264784ba8d37645..49c34494e156ff6c3277b107c38
> 40ee29dd3afbb 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1403,18 +1403,21 @@ int mptcp_pm_nl_add_addr_doit(struct sk_buff
> *skb, struct genl_info *info)
>  		return ret;
>  
>  	if (addr.addr.port && !address_use_port(&addr)) {
> -		GENL_SET_ERR_MSG(info, "flags must have signal and
> not subflow when using port");
> +		NL_SET_ERR_MSG_ATTR(info->extack, attr,
> +				    "flags must have signal and not
> subflow when using port");
>  		return -EINVAL;
>  	}
>  
>  	if (addr.flags & MPTCP_PM_ADDR_FLAG_SIGNAL &&
>  	    addr.flags & MPTCP_PM_ADDR_FLAG_FULLMESH) {
> -		GENL_SET_ERR_MSG(info, "flags mustn't have both
> signal and fullmesh");
> +		NL_SET_ERR_MSG_ATTR(info->extack, attr,
> +				    "flags mustn't have both signal
> and fullmesh");
>  		return -EINVAL;
>  	}
>  
>  	if (addr.flags & MPTCP_PM_ADDR_FLAG_IMPLICIT) {
> -		GENL_SET_ERR_MSG(info, "can't create IMPLICIT
> endpoint");
> +		NL_SET_ERR_MSG_ATTR(info->extack, attr,
> +				    "can't create IMPLICIT
> endpoint");
>  		return -EINVAL;
>  	}
>  
> @@ -1608,7 +1611,7 @@ int mptcp_pm_nl_del_addr_doit(struct sk_buff
> *skb, struct genl_info *info)
>  	spin_lock_bh(&pernet->lock);
>  	entry = __lookup_addr_by_id(pernet, addr.addr.id);
>  	if (!entry) {
> -		GENL_SET_ERR_MSG(info, "address not found");
> +		NL_SET_ERR_MSG_ATTR(info->extack, attr, "address not
> found");
>  		spin_unlock_bh(&pernet->lock);
>  		return -EINVAL;
>  	}
> @@ -1790,7 +1793,7 @@ int mptcp_pm_nl_get_addr(struct sk_buff *skb,
> struct genl_info *info)
>  	rcu_read_lock();
>  	entry = __lookup_addr_by_id(pernet, addr.addr.id);
>  	if (!entry) {
> -		GENL_SET_ERR_MSG(info, "address not found");
> +		NL_SET_ERR_MSG_ATTR(info->extack, attr, "address not
> found");
>  		ret = -EINVAL;
>  		goto unlock_fail;
>  	}
> @@ -1875,7 +1878,8 @@ static int parse_limit(struct genl_info *info,
> int id, unsigned int *limit)
>  
>  	*limit = nla_get_u32(attr);
>  	if (*limit > MPTCP_PM_ADDR_MAX) {
> -		GENL_SET_ERR_MSG(info, "limit greater than
> maximum");
> +		NL_SET_ERR_MSG_ATTR(info->extack, attr,
> +				    "limit greater than maximum");
>  		return -EINVAL;
>  	}
>  	return 0;
> @@ -2003,7 +2007,8 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb,
> struct genl_info *info)
>  	if (addr.addr.family == AF_UNSPEC) {
>  		lookup_by_id = 1;
>  		if (!addr.addr.id) {
> -			GENL_SET_ERR_MSG(info, "missing required
> inputs");
> +			NL_SET_ERR_MSG_ATTR(info->extack, attr,
> +					    "missing required
> inputs");
>  			return -EOPNOTSUPP;
>  		}
>  	}
> @@ -2016,13 +2021,13 @@ int mptcp_pm_nl_set_flags(struct sk_buff
> *skb, struct genl_info *info)
>  			       __lookup_addr(pernet, &addr.addr);
>  	if (!entry) {
>  		spin_unlock_bh(&pernet->lock);
> -		GENL_SET_ERR_MSG(info, "address not found");
> +		NL_SET_ERR_MSG_ATTR(info->extack, attr, "address not
> found");
>  		return -EINVAL;
>  	}
>  	if ((addr.flags & MPTCP_PM_ADDR_FLAG_FULLMESH) &&
>  	    (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
>  		spin_unlock_bh(&pernet->lock);
> -		GENL_SET_ERR_MSG(info, "invalid addr flags");
> +		NL_SET_ERR_MSG_ATTR(info->extack, attr, "invalid
> addr flags");
>  		return -EINVAL;
>  	}
>  
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index
> a3d477059b11c3a5618dbb6256434a8e55845995..72d4861e497eef5a516fc1c3ec1
> 659807ffedd53 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -190,7 +190,8 @@ static struct mptcp_sock
> *mptcp_userspace_pm_get_sock(const struct genl_info *in
>  	}
>  
>  	if (!mptcp_pm_is_userspace(msk)) {
> -		GENL_SET_ERR_MSG(info, "invalid request; userspace
> PM not selected");
> +		NL_SET_ERR_MSG_ATTR(info->extack, token,
> +				    "invalid request; userspace PM
> not selected");
>  		sock_put((struct sock *)msk);
>  		return NULL;
>  	}
> @@ -224,14 +225,16 @@ int mptcp_pm_nl_announce_doit(struct sk_buff
> *skb, struct genl_info *info)
>  	}
>  
>  	if (addr_val.addr.id == 0 || !(addr_val.flags &
> MPTCP_PM_ADDR_FLAG_SIGNAL)) {
> -		GENL_SET_ERR_MSG(info, "invalid addr id or flags");
> +		NL_SET_ERR_MSG_ATTR(info->extack, addr,
> +				    "invalid addr id or flags");
>  		err = -EINVAL;
>  		goto announce_err;
>  	}
>  
>  	err = mptcp_userspace_pm_append_new_local_addr(msk,
> &addr_val, false);
>  	if (err < 0) {
> -		GENL_SET_ERR_MSG(info, "did not match address and
> id");
> +		NL_SET_ERR_MSG_ATTR(info->extack, addr,
> +				    "did not match address and id");
>  		goto announce_err;
>  	}
>  
> @@ -254,7 +257,8 @@ int mptcp_pm_nl_announce_doit(struct sk_buff
> *skb, struct genl_info *info)
>  }
>  
>  static int mptcp_userspace_pm_remove_id_zero_address(struct
> mptcp_sock *msk,
> -						     struct
> genl_info *info)
> +						     struct
> genl_info *info,
> +						     struct nlattr
> *attr)
>  {
>  	struct mptcp_rm_list list = { .nr = 0 };
>  	struct mptcp_subflow_context *subflow;
> @@ -270,7 +274,8 @@ static int
> mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock *msk,
>  		}
>  	}
>  	if (!has_id_0) {
> -		GENL_SET_ERR_MSG(info, "address with id 0 not
> found");
> +		NL_SET_ERR_MSG_ATTR(info->extack, attr,
> +				    "address with id 0 not found");
>  		goto remove_err;
>  	}
>  
> @@ -330,7 +335,7 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb,
> struct genl_info *info)
>  	sk = (struct sock *)msk;
>  
>  	if (id_val == 0) {
> -		err = mptcp_userspace_pm_remove_id_zero_address(msk,
> info);
> +		err = mptcp_userspace_pm_remove_id_zero_address(msk,
> info, id);
>  		goto out;
>  	}
>  
> @@ -339,7 +344,8 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb,
> struct genl_info *info)
>  	spin_lock_bh(&msk->pm.lock);
>  	match = mptcp_userspace_pm_lookup_addr_by_id(msk, id_val);
>  	if (!match) {
> -		GENL_SET_ERR_MSG(info, "address with specified id
> not found");
> +		NL_SET_ERR_MSG_ATTR(info->extack, id,
> +				    "address with specified id not
> found");
>  		spin_unlock_bh(&msk->pm.lock);
>  		release_sock(sk);
>  		goto out;
> @@ -389,7 +395,7 @@ int mptcp_pm_nl_subflow_create_doit(struct
> sk_buff *skb, struct genl_info *info)
>  	}
>  
>  	if (entry.flags & MPTCP_PM_ADDR_FLAG_SIGNAL) {
> -		GENL_SET_ERR_MSG(info, "invalid addr flags");
> +		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "invalid
> addr flags");
>  		err = -EINVAL;
>  		goto create_err;
>  	}
> @@ -409,7 +415,8 @@ int mptcp_pm_nl_subflow_create_doit(struct
> sk_buff *skb, struct genl_info *info)
>  
>  	err = mptcp_userspace_pm_append_new_local_addr(msk, &entry,
> false);
>  	if (err < 0) {
> -		GENL_SET_ERR_MSG(info, "did not match address and
> id");
> +		NL_SET_ERR_MSG_ATTR(info->extack, laddr,
> +				    "did not match address and id");
>  		goto create_err;
>  	}
>  
> @@ -702,7 +709,7 @@ int mptcp_userspace_pm_get_addr(struct sk_buff
> *skb,
>  	spin_lock_bh(&msk->pm.lock);
>  	entry = mptcp_userspace_pm_lookup_addr_by_id(msk,
> addr.addr.id);
>  	if (!entry) {
> -		GENL_SET_ERR_MSG(info, "address not found");
> +		NL_SET_ERR_MSG_ATTR(info->extack, attr, "address not
> found");
>  		ret = -EINVAL;
>  		goto unlock_fail;
>  	}
> 

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

[Geliang Tang]

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 3/7] mptcp: pm: improve error messages

[Geliang Tang]

On Mon, 2024-12-30 at 14:24 +0100, Matthieu Baerts (NGI0) wrote:
> Some error messages were:
> 
>  - too generic: "missing input", "invalid request"
> 
>  - not precise enough: "limit greater than maximum" but what's the
> max?
> 
>  - missing: subflow not found, or connect error.
> 
> This can be easily improved by being more precise, or adding new
> error
> messages.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  net/mptcp/pm_netlink.c   | 7 ++++---
>  net/mptcp/pm_userspace.c | 9 ++++++++-
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index
> 49c34494e156ff6c3277b107c3840ee29dd3afbb..3bb213e2a967e87d4244dc0f1ba
> 58d3ba21fb371 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1878,8 +1878,9 @@ static int parse_limit(struct genl_info *info,
> int id, unsigned int *limit)
>  
>  	*limit = nla_get_u32(attr);
>  	if (*limit > MPTCP_PM_ADDR_MAX) {
> -		NL_SET_ERR_MSG_ATTR(info->extack, attr,
> -				    "limit greater than maximum");
> +		NL_SET_ERR_MSG_ATTR_FMT(info->extack, attr,
> +					"limit greater than maximum
> (%u)",
> +					MPTCP_PM_ADDR_MAX);
>  		return -EINVAL;
>  	}
>  	return 0;
> @@ -2008,7 +2009,7 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb,
> struct genl_info *info)
>  		lookup_by_id = 1;
>  		if (!addr.addr.id) {
>  			NL_SET_ERR_MSG_ATTR(info->extack, attr,
> -					    "missing required
> inputs");
> +					    "missing address ID");
>  			return -EOPNOTSUPP;
>  		}
>  	}
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index
> 9c0bec588c498e0aa78acf4018db509abe90cad2..a8f73b082460ba00f7fa998cf76
> 46368f8201e2e 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -191,7 +191,7 @@ static struct mptcp_sock
> *mptcp_userspace_pm_get_sock(const struct genl_info *in
>  
>  	if (!mptcp_pm_is_userspace(msk)) {
>  		NL_SET_ERR_MSG_ATTR(info->extack, token,
> -				    "invalid request; userspace PM
> not selected");
> +				    "userspace PM not selected");
>  		sock_put((struct sock *)msk);
>  		return NULL;
>  	}
> @@ -433,6 +433,9 @@ int mptcp_pm_nl_subflow_create_doit(struct
> sk_buff *skb, struct genl_info *info)
>  	err = __mptcp_subflow_connect(sk, &local, &addr_r);
>  	release_sock(sk);
>  
> +	if (err)
> +		GENL_SET_ERR_MSG_FMT(info, "connect error: %d",
> err);
> +
>  	spin_lock_bh(&msk->pm.lock);
>  	if (err)
>  		mptcp_userspace_pm_delete_local_addr(msk, &entry);
> @@ -557,6 +560,7 @@ int mptcp_pm_nl_subflow_destroy_doit(struct
> sk_buff *skb, struct genl_info *info
>  	lock_sock(sk);
>  	ssk = mptcp_nl_find_ssk(msk, &addr_l.addr, &addr_r);
>  	if (!ssk) {
> +		GENL_SET_ERR_MSG(info, "subflow not found");
>  		err = -ESRCH;
>  		goto release_sock;
>  	}
> @@ -634,6 +638,9 @@ int mptcp_userspace_pm_set_flags(struct sk_buff
> *skb, struct genl_info *info)
>  	ret = mptcp_pm_nl_mp_prio_send_ack(msk, &loc.addr,
> &rem.addr, bkup);
>  	release_sock(sk);
>  
> +	if (ret)
> +		GENL_SET_ERR_MSG(info, "subflow not found");

I guess you don't want to use "subflow not found" here. I changed it as
"mp_prio send ack failed" in v7.

> +
>  set_flags_err:
>  	sock_put(sk);
>  	return ret;
> 

Re: [PATCH mptcp-next v6 3/7] mptcp: pm: improve error messages

[Matthieu Baerts]

Hi Geliang,

On 06/01/2025 09:41, Geliang Tang wrote:
> On Mon, 2024-12-30 at 14:24 +0100, Matthieu Baerts (NGI0) wrote:
>> Some error messages were:
>>
>>  - too generic: "missing input", "invalid request"
>>
>>  - not precise enough: "limit greater than maximum" but what's the
>> max?
>>
>>  - missing: subflow not found, or connect error.
>>
>> This can be easily improved by being more precise, or adding new
>> error
>> messages.

(...)

>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>> index
>> 9c0bec588c498e0aa78acf4018db509abe90cad2..a8f73b082460ba00f7fa998cf76
>> 46368f8201e2e 100644
>> --- a/net/mptcp/pm_userspace.c
>> +++ b/net/mptcp/pm_userspace.c

(...)

>> @@ -634,6 +638,9 @@ int mptcp_userspace_pm_set_flags(struct sk_buff
>> *skb, struct genl_info *info)
>>  	ret = mptcp_pm_nl_mp_prio_send_ack(msk, &loc.addr,
>> &rem.addr, bkup);
>>  	release_sock(sk);
>>  
>> +	if (ret)
>> +		GENL_SET_ERR_MSG(info, "subflow not found");
> 
> I guess you don't want to use "subflow not found" here. I changed it as
> "mp_prio send ack failed" in v7.

No, I wanted to use "subflow not found": mptcp_pm_nl_mp_prio_send_ack()
will fail only if it cannot find the subflow matching the local and
remote attributes.

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

Re: [PATCH mptcp-next v6 4/7] mptcp: pm: userspace: flags: clearer msg if no remote addr

[Geliang Tang]

On Mon, 2024-12-30 at 14:24 +0100, Matthieu Baerts (NGI0) wrote:
> Since its introduction in commit 892f396c8e68 ("mptcp: netlink: issue
> MP_PRIO signals from userspace PMs"), it was mandatory to specify the
> remote address, because of the 'if (rem->addr.family == AF_UNSPEC)'
> check done later one.
> 
> In theory, this attribute can be optional, but it sounds better to be
> precise to avoid sending the MP_PRIO on the wrong subflow, e.g. if
> there
> are multiple subflows attached to the same local ID. This can be
> relaxed
> later on if there is a need to act on multiple subflows with one
> command.
> 
> For the moment, the check to see if attr_rem is NULL can be removed,
> because mptcp_pm_parse_entry() will do this check as well, no need to
> do
> that differently here.
> 
> While at it, move the parsing after the check linked to the local
> attribute.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  net/mptcp/pm_userspace.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index
> a8f73b082460ba00f7fa998cf7646368f8201e2e..2f82ac49db35ec34dcc0b9208f4
> ac12edc6ab254 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -601,12 +601,6 @@ int mptcp_userspace_pm_set_flags(struct sk_buff
> *skb, struct genl_info *info)
>  	if (ret < 0)
>  		goto set_flags_err;
>  
> -	if (attr_rem) {
> -		ret = mptcp_pm_parse_entry(attr_rem, info, false,
> &rem);
> -		if (ret < 0)
> -			goto set_flags_err;
> -	}

I did not remove this 'if (attr_rem)' check in v7. I moved this code to
a common place for userspace PM and in-kernel PM. For in-kernel PM,
this check is required.

> -
>  	if (loc.addr.family == AF_UNSPEC) {
>  		NL_SET_ERR_MSG_ATTR(info->extack, attr,
>  				    "invalid local address family");
> @@ -614,6 +608,10 @@ int mptcp_userspace_pm_set_flags(struct sk_buff
> *skb, struct genl_info *info)
>  		goto set_flags_err;
>  	}
>  
> +	ret = mptcp_pm_parse_entry(attr_rem, info, false, &rem);
> +	if (ret < 0)
> +		goto set_flags_err;
> +
>  	if (rem.addr.family == AF_UNSPEC) {
>  		NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
>  				    "invalid remote address
> family");
> 

Re: [PATCH mptcp-next v6 3/7] mptcp: pm: improve error messages

[Geliang Tang]

Happy New Year Matt!

On Mon, 2025-01-06 at 09:45 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 06/01/2025 09:41, Geliang Tang wrote:
> > On Mon, 2024-12-30 at 14:24 +0100, Matthieu Baerts (NGI0) wrote:
> > > Some error messages were:
> > > 
> > >  - too generic: "missing input", "invalid request"
> > > 
> > >  - not precise enough: "limit greater than maximum" but what's
> > > the
> > > max?
> > > 
> > >  - missing: subflow not found, or connect error.
> > > 
> > > This can be easily improved by being more precise, or adding new
> > > error
> > > messages.
> 
> (...)
> 
> > > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> > > index
> > > 9c0bec588c498e0aa78acf4018db509abe90cad2..a8f73b082460ba00f7fa998
> > > cf76
> > > 46368f8201e2e 100644
> > > --- a/net/mptcp/pm_userspace.c
> > > +++ b/net/mptcp/pm_userspace.c
> 
> (...)
> 
> > > @@ -634,6 +638,9 @@ int mptcp_userspace_pm_set_flags(struct
> > > sk_buff
> > > *skb, struct genl_info *info)
> > >  	ret = mptcp_pm_nl_mp_prio_send_ack(msk, &loc.addr,
> > > &rem.addr, bkup);
> > >  	release_sock(sk);
> > >  
> > > +	if (ret)
> > > +		GENL_SET_ERR_MSG(info, "subflow not found");
> > 
> > I guess you don't want to use "subflow not found" here. I changed
> > it as
> > "mp_prio send ack failed" in v7.
> 
> No, I wanted to use "subflow not found":
> mptcp_pm_nl_mp_prio_send_ack()
> will fail only if it cannot find the subflow matching the local and
> remote attributes.

My bad, how about using "subflow not found in set_flags" here, and use
"subflow not found in subflow_destroy" for
mptcp_pm_nl_subflow_destroy_doit?

> 
> Cheers,
> Matt

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

[Matthieu Baerts]

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.

Re: [PATCH mptcp-next v6 3/7] mptcp: pm: improve error messages

[Matthieu Baerts]

On 06/01/2025 09:55, Geliang Tang wrote:
> Happy New Year Matt!

Thank you! :)

Happy New Year as well (even if I should probably say that to you at the
end of January :) )

> On Mon, 2025-01-06 at 09:45 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 06/01/2025 09:41, Geliang Tang wrote:
>>> On Mon, 2024-12-30 at 14:24 +0100, Matthieu Baerts (NGI0) wrote:
>>>> Some error messages were:
>>>>
>>>>  - too generic: "missing input", "invalid request"
>>>>
>>>>  - not precise enough: "limit greater than maximum" but what's
>>>> the
>>>> max?
>>>>
>>>>  - missing: subflow not found, or connect error.
>>>>
>>>> This can be easily improved by being more precise, or adding new
>>>> error
>>>> messages.
>>
>> (...)
>>
>>>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>>>> index
>>>> 9c0bec588c498e0aa78acf4018db509abe90cad2..a8f73b082460ba00f7fa998
>>>> cf76
>>>> 46368f8201e2e 100644
>>>> --- a/net/mptcp/pm_userspace.c
>>>> +++ b/net/mptcp/pm_userspace.c
>>
>> (...)
>>
>>>> @@ -634,6 +638,9 @@ int mptcp_userspace_pm_set_flags(struct
>>>> sk_buff
>>>> *skb, struct genl_info *info)
>>>>  	ret = mptcp_pm_nl_mp_prio_send_ack(msk, &loc.addr,
>>>> &rem.addr, bkup);
>>>>  	release_sock(sk);
>>>>  
>>>> +	if (ret)
>>>> +		GENL_SET_ERR_MSG(info, "subflow not found");
>>>
>>> I guess you don't want to use "subflow not found" here. I changed
>>> it as
>>> "mp_prio send ack failed" in v7.
>>
>> No, I wanted to use "subflow not found":
>> mptcp_pm_nl_mp_prio_send_ack()
>> will fail only if it cannot find the subflow matching the local and
>> remote attributes.
> 
> My bad, how about using "subflow not found in set_flags" here, and use
> "subflow not found in subflow_destroy" for
> mptcp_pm_nl_subflow_destroy_doit?

I don't think we need a different error message: the userspace will send
a specific Netlink command, and an error can be returned. No need to
mention which command it is I think.

If you prefer, feel free to add a comment above this error here:

/* mptcp_pm_nl_mp_prio_send_ack will fail if it cannot find a subflow */

While at it, if you plan to send a v8, please use 'if (ret < 0)' to
clearly show we are looking at errors. (When the variable is called
'err', that's clearer, but no need to change it here.)

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

Re: [PATCH mptcp-next v6 0/7] mptcp: use GENL_REQ_ATTR_CHECK in userspace pm

[Matthieu Baerts]

Hi Geliang,

On 06/01/2025 09:24, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for this new version.
> 
> On Mon, 2024-12-30 at 14:24 +0100, Matthieu Baerts (NGI0) wrote:
>> This is a newer version of the series started by Geliang, now making
>> them more uniform on how the NL errors are set, and giving more hints
>> to
>> the userspace about what's wrong.
> 
> This series is useful, but it conflicts with "BPF path manager" series
> under review, especially with set_flags() interface. So I also added
> the patches to update set_flags interface into v7.

Yes, sorry about that, I noticed that after having sent this series.

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