| Message ID | 20241230-genl_req_attr_check-v6-4-3ec9103559e7@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geliang Tang |
| Headers | show |
| Series | mptcp: use GENL_REQ_ATTR_CHECK in userspace pm | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 22 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/build | success | Build and static analysis OK |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf-normal__only_bpftest_all_ | success | Success! ✅ |
| matttbe/KVM_Validation__btf-debug__only_bpftest_all_ | warning | Unstable: 1 failed test(s): bpftest_test_progs-no_alu32_mptcp |
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"); >
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");
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(-)