diff mbox series

[net-next,01/16] mptcp: use WRITE_ONCE/READ_ONCE for the pernet *_max

Message ID 20210129011115.133953-2-mathew.j.martineau@linux.intel.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series mptcp: ADD_ADDR enhancements | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count fail Series longer than 15 patches
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 1 maintainers not CCed: matthieu.baerts@tessares.net
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 2 this patch: 2
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 76 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 2 this patch: 2
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Mat Martineau Jan. 29, 2021, 1:11 a.m. UTC 
From: Geliang Tang <geliangtang@gmail.com>

This patch uses WRITE_ONCE() and READ_ONCE for all the pernet
add_addr_signal_max, add_addr_accept_max, local_addr_max and
subflows_max fields in struct pm_nl_pernet to avoid concurrency
issues.

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
---
 net/mptcp/pm_netlink.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

Comments

Jakub Kicinski Jan. 30, 2021, 5:54 a.m. UTC | #1 
On Thu, 28 Jan 2021 17:11:00 -0800 Mat Martineau wrote:
> +	if (entry->addr.flags & MPTCP_PM_ADDR_FLAG_SIGNAL) {
> +		addr_max = READ_ONCE(pernet->add_addr_signal_max);
> +		WRITE_ONCE(pernet->add_addr_signal_max, addr_max + 1);
> +	}

This is an odd construct.

READ_ONCE() is used when the value can change underneath the reader,
not in writers. If we want to increment a variable, there must either
be a writer side lock, or the variable has to be switched to atomic_t.

I'm guessing the former is the case here, so there can be no concurrent
writers. Please omit the READ_ONCE():

	if (entry->addr.flags & MPTCP_PM_ADDR_FLAG_SIGNAL)
		WRITE_ONCE(pernet->add_addr_signal_max, 
			   pernet->add_addr_signal_max + 1);


Same for other 3 cases.
Jakub Kicinski Jan. 30, 2021, 5:55 a.m. UTC | #2 
On Thu, 28 Jan 2021 17:11:00 -0800 Mat Martineau wrote:
>  	spin_lock_bh(&pernet->lock);
> -	rcv_addrs = pernet->add_addr_accept_max;
> +	rcv_addrs = READ_ONCE(pernet->add_addr_accept_max);

Oh, this reader is also under the lock, what's the concurrency issue
you speak of?
diff mbox series

Patch

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 83976b9ee99b..7d6081d9a1db 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -572,6 +572,7 @@  static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
 					     struct mptcp_pm_addr_entry *entry)
 {
 	struct mptcp_pm_addr_entry *cur;
+	unsigned int addr_max;
 	int ret = -EINVAL;
 
 	spin_lock_bh(&pernet->lock);
@@ -614,10 +615,14 @@  static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
 	if (entry->addr.id > pernet->next_id)
 		pernet->next_id = entry->addr.id;
 
-	if (entry->addr.flags & MPTCP_PM_ADDR_FLAG_SIGNAL)
-		pernet->add_addr_signal_max++;
-	if (entry->addr.flags & MPTCP_PM_ADDR_FLAG_SUBFLOW)
-		pernet->local_addr_max++;
+	if (entry->addr.flags & MPTCP_PM_ADDR_FLAG_SIGNAL) {
+		addr_max = READ_ONCE(pernet->add_addr_signal_max);
+		WRITE_ONCE(pernet->add_addr_signal_max, addr_max + 1);
+	}
+	if (entry->addr.flags & MPTCP_PM_ADDR_FLAG_SUBFLOW) {
+		addr_max = READ_ONCE(pernet->local_addr_max);
+		WRITE_ONCE(pernet->local_addr_max, addr_max + 1);
+	}
 
 	pernet->addrs++;
 	list_add_tail_rcu(&entry->list, &pernet->local_addr_list);
@@ -912,6 +917,7 @@  static int mptcp_nl_cmd_del_addr(struct sk_buff *skb, struct genl_info *info)
 	struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
 	struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
 	struct mptcp_pm_addr_entry addr, *entry;
+	unsigned int addr_max;
 	int ret;
 
 	ret = mptcp_pm_parse_addr(attr, info, false, &addr);
@@ -925,10 +931,14 @@  static int mptcp_nl_cmd_del_addr(struct sk_buff *skb, struct genl_info *info)
 		spin_unlock_bh(&pernet->lock);
 		return -EINVAL;
 	}
-	if (entry->addr.flags & MPTCP_PM_ADDR_FLAG_SIGNAL)
-		pernet->add_addr_signal_max--;
-	if (entry->addr.flags & MPTCP_PM_ADDR_FLAG_SUBFLOW)
-		pernet->local_addr_max--;
+	if (entry->addr.flags & MPTCP_PM_ADDR_FLAG_SIGNAL) {
+		addr_max = READ_ONCE(pernet->add_addr_signal_max);
+		WRITE_ONCE(pernet->add_addr_signal_max, addr_max - 1);
+	}
+	if (entry->addr.flags & MPTCP_PM_ADDR_FLAG_SUBFLOW) {
+		addr_max = READ_ONCE(pernet->local_addr_max);
+		WRITE_ONCE(pernet->local_addr_max, addr_max - 1);
+	}
 
 	pernet->addrs--;
 	list_del_rcu(&entry->list);
@@ -956,9 +966,9 @@  static void __flush_addrs(struct net *net, struct list_head *list)
 
 static void __reset_counters(struct pm_nl_pernet *pernet)
 {
-	pernet->add_addr_signal_max = 0;
-	pernet->add_addr_accept_max = 0;
-	pernet->local_addr_max = 0;
+	WRITE_ONCE(pernet->add_addr_signal_max, 0);
+	WRITE_ONCE(pernet->add_addr_accept_max, 0);
+	WRITE_ONCE(pernet->local_addr_max, 0);
 	pernet->addrs = 0;
 }
 
@@ -1130,12 +1140,12 @@  mptcp_nl_cmd_set_limits(struct sk_buff *skb, struct genl_info *info)
 	int ret;
 
 	spin_lock_bh(&pernet->lock);
-	rcv_addrs = pernet->add_addr_accept_max;
+	rcv_addrs = READ_ONCE(pernet->add_addr_accept_max);
 	ret = parse_limit(info, MPTCP_PM_ATTR_RCV_ADD_ADDRS, &rcv_addrs);
 	if (ret)
 		goto unlock;
 
-	subflows = pernet->subflows_max;
+	subflows = READ_ONCE(pernet->subflows_max);
 	ret = parse_limit(info, MPTCP_PM_ATTR_SUBFLOWS, &subflows);
 	if (ret)
 		goto unlock;