net/mptcp/pm_netlink.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-)
The MPTCP endpoint list is under RCU protection, guarded by the
pernet spinlock. mptcp_nl_cmd_set_flags() traverses the list
without acquiring the spin-lock nor under the RCU critical section.
This change addresses the issue performing the lookup and the endpoint
update under the pernet spinlock.
Fixes: 0f9f696a502e ("mptcp: add set_flags command in PM netlink")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/pm_netlink.c | 37 +++++++++++++++++++++++++++----------
1 file changed, 27 insertions(+), 10 deletions(-)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index f12effa71942..20b9037c4906 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -478,6 +478,20 @@ __lookup_addr_by_id(struct pm_nl_pernet *pernet, unsigned int id)
return NULL;
}
+static struct mptcp_pm_addr_entry *
+__lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info,
+ bool lookup_by_id)
+{
+ struct mptcp_pm_addr_entry *entry;
+
+ list_for_each_entry(entry, &pernet->local_addr_list, list) {
+ if ((!lookup_by_id && addresses_equal(&entry->addr, info, true)) ||
+ (lookup_by_id && entry->addr.id == info->id))
+ return entry;
+ }
+ return NULL;
+}
+
static int
lookup_id_by_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *addr)
{
@@ -1757,18 +1771,21 @@ static int mptcp_nl_cmd_set_flags(struct sk_buff *skb, struct genl_info *info)
return -EOPNOTSUPP;
}
- list_for_each_entry(entry, &pernet->local_addr_list, list) {
- if ((!lookup_by_id && addresses_equal(&entry->addr, &addr.addr, true)) ||
- (lookup_by_id && entry->addr.id == addr.addr.id)) {
- mptcp_nl_addr_backup(net, &entry->addr, bkup);
-
- if (bkup)
- entry->flags |= MPTCP_PM_ADDR_FLAG_BACKUP;
- else
- entry->flags &= ~MPTCP_PM_ADDR_FLAG_BACKUP;
- }
+ spin_lock_bh(&pernet->lock);
+ entry = __lookup_addr(pernet, &addr.addr, lookup_by_id);
+ if (!entry) {
+ spin_unlock_bh(&pernet->lock);
+ return -EINVAL;
}
+ if (bkup)
+ entry->flags |= MPTCP_PM_ADDR_FLAG_BACKUP;
+ else
+ entry->flags &= ~MPTCP_PM_ADDR_FLAG_BACKUP;
+ addr = *entry;
+ spin_unlock_bh(&pernet->lock);
+
+ mptcp_nl_addr_backup(net, &addr.addr, bkup);
return 0;
}
--
2.33.1
On Tue, 11 Jan 2022, Paolo Abeni wrote: > The MPTCP endpoint list is under RCU protection, guarded by the > pernet spinlock. mptcp_nl_cmd_set_flags() traverses the list > without acquiring the spin-lock nor under the RCU critical section. > > This change addresses the issue performing the lookup and the endpoint > update under the pernet spinlock. > > Fixes: 0f9f696a502e ("mptcp: add set_flags command in PM netlink") > Signed-off-by: Paolo Abeni <pabeni@redhat.com> Looks good to me. There's a slight change to the number of loop iterations since the new code only walks through the local_addr_list until the first match is found - but there aren't supposed to be duplicates in the list so this should be an optimization. Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com> > --- > net/mptcp/pm_netlink.c | 37 +++++++++++++++++++++++++++---------- > 1 file changed, 27 insertions(+), 10 deletions(-) > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c > index f12effa71942..20b9037c4906 100644 > --- a/net/mptcp/pm_netlink.c > +++ b/net/mptcp/pm_netlink.c > @@ -478,6 +478,20 @@ __lookup_addr_by_id(struct pm_nl_pernet *pernet, unsigned int id) > return NULL; > } > > +static struct mptcp_pm_addr_entry * > +__lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info, > + bool lookup_by_id) > +{ > + struct mptcp_pm_addr_entry *entry; > + > + list_for_each_entry(entry, &pernet->local_addr_list, list) { > + if ((!lookup_by_id && addresses_equal(&entry->addr, info, true)) || > + (lookup_by_id && entry->addr.id == info->id)) > + return entry; > + } > + return NULL; > +} > + > static int > lookup_id_by_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *addr) > { > @@ -1757,18 +1771,21 @@ static int mptcp_nl_cmd_set_flags(struct sk_buff *skb, struct genl_info *info) > return -EOPNOTSUPP; > } > > - list_for_each_entry(entry, &pernet->local_addr_list, list) { > - if ((!lookup_by_id && addresses_equal(&entry->addr, &addr.addr, true)) || > - (lookup_by_id && entry->addr.id == addr.addr.id)) { > - mptcp_nl_addr_backup(net, &entry->addr, bkup); > - > - if (bkup) > - entry->flags |= MPTCP_PM_ADDR_FLAG_BACKUP; > - else > - entry->flags &= ~MPTCP_PM_ADDR_FLAG_BACKUP; > - } > + spin_lock_bh(&pernet->lock); > + entry = __lookup_addr(pernet, &addr.addr, lookup_by_id); > + if (!entry) { > + spin_unlock_bh(&pernet->lock); > + return -EINVAL; > } > > + if (bkup) > + entry->flags |= MPTCP_PM_ADDR_FLAG_BACKUP; > + else > + entry->flags &= ~MPTCP_PM_ADDR_FLAG_BACKUP; > + addr = *entry; > + spin_unlock_bh(&pernet->lock); > + > + mptcp_nl_addr_backup(net, &addr.addr, bkup); > return 0; > } > > -- > 2.33.1 > > > -- Mat Martineau Intel
Hi Paolo, Mat, On 11/01/2022 13:07, Paolo Abeni wrote: > The MPTCP endpoint list is under RCU protection, guarded by the > pernet spinlock. mptcp_nl_cmd_set_flags() traverses the list > without acquiring the spin-lock nor under the RCU critical section. > > This change addresses the issue performing the lookup and the endpoint > update under the pernet spinlock. Thank you for the patch and the review! Now in our tree (fixes for -net) with Mat's RvB tag: - 41fdb59d2a03: mptcp: fix msk traversal in mptcp_nl_cmd_set_flags() - Results: b755fcc13413..6eab36365cea Builds and tests are now in progress: https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220113T102140 https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export Cheers, Matt -- Tessares | Belgium | Hybrid Access Solutions www.tessares.net
© 2016 - 2024 Red Hat, Inc.