* [PATCH net-next v3 0/2] mctp: Fix incorrect refs for extended addr
@ 2022-02-22 4:17 Matt Johnston
2022-02-22 4:17 ` [PATCH net-next v3 1/2] mctp: make __mctp_dev_get() take a refcount hold Matt Johnston
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Matt Johnston @ 2022-02-22 4:17 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Jakub Kicinski, Jeremy Kerr
This fixes an incorrect netdev unref and also addresses the race
condition identified by Jakub in v2. Thanks for the review.
Cheers,
Matt
Matt Johnston (2):
mctp: make __mctp_dev_get() take a refcount hold
mctp: Fix incorrect netdev unref for extended addr
net/mctp/device.c | 21 ++++++++++++++++++---
net/mctp/route.c | 13 ++++++-------
net/mctp/test/utils.c | 1 -
3 files changed, 24 insertions(+), 11 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net-next v3 1/2] mctp: make __mctp_dev_get() take a refcount hold
2022-02-22 4:17 [PATCH net-next v3 0/2] mctp: Fix incorrect refs for extended addr Matt Johnston
@ 2022-02-22 4:17 ` Matt Johnston
2022-02-23 3:59 ` Jakub Kicinski
2022-02-22 4:17 ` [PATCH net-next v3 2/2] mctp: Fix incorrect netdev unref for extended addr Matt Johnston
2022-02-23 12:30 ` [PATCH net-next v3 0/2] mctp: Fix incorrect refs " patchwork-bot+netdevbpf
2 siblings, 1 reply; 6+ messages in thread
From: Matt Johnston @ 2022-02-22 4:17 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Jakub Kicinski, Jeremy Kerr
Previously there was a race that could allow the mctp_dev refcount
to hit zero:
rcu_read_lock();
mdev = __mctp_dev_get(dev);
// mctp_unregister() happens here, mdev->refs hits zero
mctp_dev_hold(dev);
rcu_read_unlock();
Now we make __mctp_dev_get() take the hold itself. It is safe to test
against the zero refcount because __mctp_dev_get() is called holding
rcu_read_lock and mctp_dev uses kfree_rcu().
Reported-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
---
net/mctp/device.c | 21 ++++++++++++++++++---
net/mctp/route.c | 5 ++++-
net/mctp/test/utils.c | 1 -
3 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/net/mctp/device.c b/net/mctp/device.c
index 9e097e61f23a..b754c31162b1 100644
--- a/net/mctp/device.c
+++ b/net/mctp/device.c
@@ -25,12 +25,25 @@ struct mctp_dump_cb {
size_t a_idx;
};
-/* unlocked: caller must hold rcu_read_lock */
+/* unlocked: caller must hold rcu_read_lock.
+ * Returned mctp_dev has its refcount incremented, or NULL if unset.
+ */
struct mctp_dev *__mctp_dev_get(const struct net_device *dev)
{
- return rcu_dereference(dev->mctp_ptr);
+ struct mctp_dev *mdev = rcu_dereference(dev->mctp_ptr);
+
+ /* RCU guarantees that any mdev is still live.
+ * Zero refcount implies a pending free, return NULL.
+ */
+ if (mdev)
+ if (!refcount_inc_not_zero(&mdev->refs))
+ return NULL;
+ return mdev;
}
+/* Returned mctp_dev does not have refcount incremented. The returned pointer
+ * remains live while rtnl_lock is held, as that prevents mctp_unregister()
+ */
struct mctp_dev *mctp_dev_get_rtnl(const struct net_device *dev)
{
return rtnl_dereference(dev->mctp_ptr);
@@ -124,6 +137,7 @@ static int mctp_dump_addrinfo(struct sk_buff *skb, struct netlink_callback *cb)
if (mdev) {
rc = mctp_dump_dev_addrinfo(mdev,
skb, cb);
+ mctp_dev_put(mdev);
// Error indicates full buffer, this
// callback will get retried.
if (rc < 0)
@@ -298,7 +312,7 @@ void mctp_dev_hold(struct mctp_dev *mdev)
void mctp_dev_put(struct mctp_dev *mdev)
{
- if (refcount_dec_and_test(&mdev->refs)) {
+ if (mdev && refcount_dec_and_test(&mdev->refs)) {
dev_put(mdev->dev);
kfree_rcu(mdev, rcu);
}
@@ -370,6 +384,7 @@ static size_t mctp_get_link_af_size(const struct net_device *dev,
if (!mdev)
return 0;
ret = nla_total_size(4); /* IFLA_MCTP_NET */
+ mctp_dev_put(mdev);
return ret;
}
diff --git a/net/mctp/route.c b/net/mctp/route.c
index fe6c8bf1ec2c..6f277e56b168 100644
--- a/net/mctp/route.c
+++ b/net/mctp/route.c
@@ -836,7 +836,7 @@ int mctp_local_output(struct sock *sk, struct mctp_route *rt,
{
struct mctp_sock *msk = container_of(sk, struct mctp_sock, sk);
struct mctp_skb_cb *cb = mctp_cb(skb);
- struct mctp_route tmp_rt;
+ struct mctp_route tmp_rt = {0};
struct mctp_sk_key *key;
struct net_device *dev;
struct mctp_hdr *hdr;
@@ -948,6 +948,7 @@ int mctp_local_output(struct sock *sk, struct mctp_route *rt,
mctp_route_release(rt);
dev_put(dev);
+ mctp_dev_put(tmp_rt.dev);
return rc;
@@ -1124,11 +1125,13 @@ static int mctp_pkttype_receive(struct sk_buff *skb, struct net_device *dev,
rt->output(rt, skb);
mctp_route_release(rt);
+ mctp_dev_put(mdev);
return NET_RX_SUCCESS;
err_drop:
kfree_skb(skb);
+ mctp_dev_put(mdev);
return NET_RX_DROP;
}
diff --git a/net/mctp/test/utils.c b/net/mctp/test/utils.c
index 7b7918702592..e03ba66bbe18 100644
--- a/net/mctp/test/utils.c
+++ b/net/mctp/test/utils.c
@@ -54,7 +54,6 @@ struct mctp_test_dev *mctp_test_create_dev(void)
rcu_read_lock();
dev->mdev = __mctp_dev_get(ndev);
- mctp_dev_hold(dev->mdev);
rcu_read_unlock();
return dev;
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net-next v3 2/2] mctp: Fix incorrect netdev unref for extended addr
2022-02-22 4:17 [PATCH net-next v3 0/2] mctp: Fix incorrect refs for extended addr Matt Johnston
2022-02-22 4:17 ` [PATCH net-next v3 1/2] mctp: make __mctp_dev_get() take a refcount hold Matt Johnston
@ 2022-02-22 4:17 ` Matt Johnston
2022-02-23 12:30 ` [PATCH net-next v3 0/2] mctp: Fix incorrect refs " patchwork-bot+netdevbpf
2 siblings, 0 replies; 6+ messages in thread
From: Matt Johnston @ 2022-02-22 4:17 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Jakub Kicinski, Jeremy Kerr
In the extended addressing local route output codepath
dev_get_by_index_rcu() doesn't take a dev_hold() so we shouldn't
dev_put().
Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
---
net/mctp/route.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/net/mctp/route.c b/net/mctp/route.c
index 6f277e56b168..5078ce3315cf 100644
--- a/net/mctp/route.c
+++ b/net/mctp/route.c
@@ -838,7 +838,6 @@ int mctp_local_output(struct sock *sk, struct mctp_route *rt,
struct mctp_skb_cb *cb = mctp_cb(skb);
struct mctp_route tmp_rt = {0};
struct mctp_sk_key *key;
- struct net_device *dev;
struct mctp_hdr *hdr;
unsigned long flags;
unsigned int mtu;
@@ -851,12 +850,12 @@ int mctp_local_output(struct sock *sk, struct mctp_route *rt,
if (rt) {
ext_rt = false;
- dev = NULL;
-
if (WARN_ON(!rt->dev))
goto out_release;
} else if (cb->ifindex) {
+ struct net_device *dev;
+
ext_rt = true;
rt = &tmp_rt;
@@ -866,7 +865,6 @@ int mctp_local_output(struct sock *sk, struct mctp_route *rt,
rcu_read_unlock();
return rc;
}
-
rt->dev = __mctp_dev_get(dev);
rcu_read_unlock();
@@ -947,11 +945,9 @@ int mctp_local_output(struct sock *sk, struct mctp_route *rt,
if (!ext_rt)
mctp_route_release(rt);
- dev_put(dev);
mctp_dev_put(tmp_rt.dev);
return rc;
-
}
/* route management */
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v3 1/2] mctp: make __mctp_dev_get() take a refcount hold
2022-02-22 4:17 ` [PATCH net-next v3 1/2] mctp: make __mctp_dev_get() take a refcount hold Matt Johnston
@ 2022-02-23 3:59 ` Jakub Kicinski
2022-02-23 4:22 ` Jeremy Kerr
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2022-02-23 3:59 UTC (permalink / raw)
To: Jeremy Kerr; +Cc: Matt Johnston, netdev, David S. Miller
On Tue, 22 Feb 2022 12:17:38 +0800 Matt Johnston wrote:
> Previously there was a race that could allow the mctp_dev refcount
> to hit zero:
>
> rcu_read_lock();
> mdev = __mctp_dev_get(dev);
> // mctp_unregister() happens here, mdev->refs hits zero
> mctp_dev_hold(dev);
> rcu_read_unlock();
>
> Now we make __mctp_dev_get() take the hold itself. It is safe to test
> against the zero refcount because __mctp_dev_get() is called holding
> rcu_read_lock and mctp_dev uses kfree_rcu().
Jeremy, did you have any specific semantics or naming scheme in mind
here? PTAL. Is it better to make __mctp_dev_get() "safe" or create
mctp_dev_get()? etc
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v3 1/2] mctp: make __mctp_dev_get() take a refcount hold
2022-02-23 3:59 ` Jakub Kicinski
@ 2022-02-23 4:22 ` Jeremy Kerr
0 siblings, 0 replies; 6+ messages in thread
From: Jeremy Kerr @ 2022-02-23 4:22 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Matt Johnston, netdev, David S. Miller
Hi Jakub,
> Jeremy, did you have any specific semantics or naming scheme in mind
> here? PTAL. Is it better to make __mctp_dev_get() "safe" or create
> mctp_dev_get()? etc
The __ prefix is (was?) more about the requirement for the RCU read lock
there. That's still the case, so the __ may still be applicable.
We only have one non-test usage of a contender for a RCU-locked
mctp_dev_get(), ie, currently:
rcu_read_lock();
dev = __mctp_dev_get();
rcu_read_unlock();
- so I'm not sure it's worthwhile adding a separate function for that
at present, and I'm OK with this patch retaining the __.
I guess the question is really: as per existing conventions, does __
more imply an unlocked accessor, or a non-reference-counting accessor?
Cheers,
Jeremy
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v3 0/2] mctp: Fix incorrect refs for extended addr
2022-02-22 4:17 [PATCH net-next v3 0/2] mctp: Fix incorrect refs for extended addr Matt Johnston
2022-02-22 4:17 ` [PATCH net-next v3 1/2] mctp: make __mctp_dev_get() take a refcount hold Matt Johnston
2022-02-22 4:17 ` [PATCH net-next v3 2/2] mctp: Fix incorrect netdev unref for extended addr Matt Johnston
@ 2022-02-23 12:30 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-02-23 12:30 UTC (permalink / raw)
To: Matt Johnston; +Cc: netdev, davem, kuba, jk
Hello:
This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:
On Tue, 22 Feb 2022 12:17:37 +0800 you wrote:
> This fixes an incorrect netdev unref and also addresses the race
> condition identified by Jakub in v2. Thanks for the review.
>
> Cheers,
> Matt
>
> Matt Johnston (2):
> mctp: make __mctp_dev_get() take a refcount hold
> mctp: Fix incorrect netdev unref for extended addr
>
> [...]
Here is the summary with links:
- [net-next,v3,1/2] mctp: make __mctp_dev_get() take a refcount hold
https://git.kernel.org/netdev/net-next/c/dc121c008491
- [net-next,v3,2/2] mctp: Fix incorrect netdev unref for extended addr
https://git.kernel.org/netdev/net-next/c/e297db3eadd7
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-23 12:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-02-22 4:17 [PATCH net-next v3 0/2] mctp: Fix incorrect refs for extended addr Matt Johnston
2022-02-22 4:17 ` [PATCH net-next v3 1/2] mctp: make __mctp_dev_get() take a refcount hold Matt Johnston
2022-02-23 3:59 ` Jakub Kicinski
2022-02-23 4:22 ` Jeremy Kerr
2022-02-22 4:17 ` [PATCH net-next v3 2/2] mctp: Fix incorrect netdev unref for extended addr Matt Johnston
2022-02-23 12:30 ` [PATCH net-next v3 0/2] mctp: Fix incorrect refs " patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).