| Message ID | 0d06955f697e484c18262a9806520132f07110cf.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 |
| Context | Check | Description |
|---|---|---|
| matttbe/checkpatch | warning | total: 0 errors, 1 warnings, 0 checks, 53 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! ✅ |
Hi Geliang, On 06/01/2025 09:16, Geliang Tang wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > Similar to in-kernel PM, this patch adds address ID support to set_flags() > interface of userspace PM, allowing it to work with either an address or > an address ID. > > When an address ID is used, mptcp_userspace_pm_lookup_addr_by_id() helper > is used to look up the address entry in the local address list instead of > using mptcp_userspace_pm_lookup_addr(). > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> > --- > net/mptcp/pm_userspace.c | 31 ++++++++++++++++++++----------- > 1 file changed, 20 insertions(+), 11 deletions(-) > > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c > index 40fd2f788196..60db72060e6e 100644 > --- a/net/mptcp/pm_userspace.c > +++ b/net/mptcp/pm_userspace.c > @@ -577,6 +577,7 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info) > struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR]; > struct mptcp_pm_addr_entry *entry; > struct mptcp_sock *msk; > + u8 lookup_by_id = 0; > int ret = -EINVAL; > struct sock *sk; > u8 bkup = 0; > @@ -592,10 +593,13 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info) > 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; > + lookup_by_id = 1; > + if (!loc.addr.id) { > + NL_SET_ERR_MSG_ATTR(info->extack, attr, > + "missing address ID"); > + ret = -EOPNOTSUPP; > + goto set_flags_err; > + } > } > > if (attr_rem) { > @@ -615,17 +619,22 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info) > bkup = 1; > > spin_lock_bh(&msk->pm.lock); > - entry = mptcp_userspace_pm_lookup_addr(msk, &loc.addr); > - if (entry) { > - if (bkup) > - entry->flags |= MPTCP_PM_ADDR_FLAG_BACKUP; > - else > - entry->flags &= ~MPTCP_PM_ADDR_FLAG_BACKUP; > + entry = lookup_by_id ? mptcp_userspace_pm_lookup_addr_by_id(msk, loc.addr.id) : > + mptcp_userspace_pm_lookup_addr(msk, &loc.addr); > + if (!entry) { > + spin_unlock_bh(&msk->pm.lock); > + ret = -EINVAL; > + goto set_flags_err; Mmh, you are changing the behaviour here by making it mandatory to have an entry, and I don't think you can: if I'm not mistaken, with the userspace PM, it is possible not to find any entries here, e.g. if a subflow using this address has not been added or the address has not been announced (I guess the initial address is not there then). Also, with the userspace PM, it is possible to have an entry with an ADDR ID == 0. Do you think this patch is worth it? Setting by ID for the in-kernel PM makes sense: unique ID for the netns, easier to type the ID than the full address. While for the userspace PM, it will be managed by a daemon that will have to track addresses anyway. > } > + > + if (bkup) > + entry->flags |= MPTCP_PM_ADDR_FLAG_BACKUP; > + else > + entry->flags &= ~MPTCP_PM_ADDR_FLAG_BACKUP; > spin_unlock_bh(&msk->pm.lock); > > lock_sock(sk); > - ret = mptcp_pm_nl_mp_prio_send_ack(msk, &loc.addr, &rem.addr, bkup); > + ret = mptcp_pm_nl_mp_prio_send_ack(msk, &entry->addr, &rem.addr, bkup); > release_sock(sk); > > set_flags_err: Cheers, Matt
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c index 40fd2f788196..60db72060e6e 100644 --- a/net/mptcp/pm_userspace.c +++ b/net/mptcp/pm_userspace.c @@ -577,6 +577,7 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info) struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR]; struct mptcp_pm_addr_entry *entry; struct mptcp_sock *msk; + u8 lookup_by_id = 0; int ret = -EINVAL; struct sock *sk; u8 bkup = 0; @@ -592,10 +593,13 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info) 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; + lookup_by_id = 1; + if (!loc.addr.id) { + NL_SET_ERR_MSG_ATTR(info->extack, attr, + "missing address ID"); + ret = -EOPNOTSUPP; + goto set_flags_err; + } } if (attr_rem) { @@ -615,17 +619,22 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info) bkup = 1; spin_lock_bh(&msk->pm.lock); - entry = mptcp_userspace_pm_lookup_addr(msk, &loc.addr); - if (entry) { - if (bkup) - entry->flags |= MPTCP_PM_ADDR_FLAG_BACKUP; - else - entry->flags &= ~MPTCP_PM_ADDR_FLAG_BACKUP; + entry = lookup_by_id ? mptcp_userspace_pm_lookup_addr_by_id(msk, loc.addr.id) : + mptcp_userspace_pm_lookup_addr(msk, &loc.addr); + if (!entry) { + spin_unlock_bh(&msk->pm.lock); + ret = -EINVAL; + goto set_flags_err; } + + if (bkup) + entry->flags |= MPTCP_PM_ADDR_FLAG_BACKUP; + else + entry->flags &= ~MPTCP_PM_ADDR_FLAG_BACKUP; spin_unlock_bh(&msk->pm.lock); lock_sock(sk); - ret = mptcp_pm_nl_mp_prio_send_ack(msk, &loc.addr, &rem.addr, bkup); + ret = mptcp_pm_nl_mp_prio_send_ack(msk, &entry->addr, &rem.addr, bkup); release_sock(sk); set_flags_err: