[PATCH] net: mctp: hold route device before source address lookup

[PATCH] net: mctp: hold route device before source address lookup

[Yiqi Sun]

mctp_route_lookup() walks the route table under RCU and may find a direct
route whose route-table reference to rt->dev is being removed concurrently
by NETDEV_UNREGISTER.

Since commit 22cb45afd221 ("net: mctp: perform source address lookups
when we populate our dst"), the direct-route lookup path reads the source
address with mctp_dev_saddr(rt->dev) before mctp_dst_from_route() takes the
dst's own mctp_dev reference. If device teardown wins that race,
mctp_route_remove_dev() can delete the route and mctp_route_release() can
drop the route's mctp_dev reference to zero. mctp_dev_put() frees
mdev->addrs synchronously, while only the mctp_dev body is deferred with
kfree_rcu().

That leaves the lookup side able to read freed mdev->addrs in
mctp_dev_saddr(), or later increment a zero refcount in mctp_dev_hold().

Add a try-hold helper for mctp_dev and use it before reading the source
address from a direct route. mctp_dst_from_route() now consumes that held
reference when it populates dst. If no dst is requested, drop the temporary
reference in mctp_route_lookup(). Do the same when the route cannot be used
for a gateway path without a source address.

Fixes: 22cb45afd221 ("net: mctp: perform source address lookups when we populate our dst")
Signed-off-by: Yiqi Sun <sunyiqixm@gmail.com>
---
 include/net/mctpdevice.h |  1 +
 net/mctp/device.c        |  5 +++++
 net/mctp/route.c         | 28 ++++++++++++++++++++--------
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/include/net/mctpdevice.h b/include/net/mctpdevice.h
index 957d9ef924c5..1d052f11127c 100644
--- a/include/net/mctpdevice.h
+++ b/include/net/mctpdevice.h
@@ -50,6 +50,7 @@ int mctp_register_netdev(struct net_device *dev,
 void mctp_unregister_netdev(struct net_device *dev);
 
 void mctp_dev_hold(struct mctp_dev *mdev);
+bool mctp_dev_try_hold(struct mctp_dev *mdev);
 void mctp_dev_put(struct mctp_dev *mdev);
 
 void mctp_dev_set_key(struct mctp_dev *dev, struct mctp_sk_key *key);
diff --git a/net/mctp/device.c b/net/mctp/device.c
index 2c84df674669..18b192bbfaa7 100644
--- a/net/mctp/device.c
+++ b/net/mctp/device.c
@@ -303,6 +303,11 @@ void mctp_dev_hold(struct mctp_dev *mdev)
 	refcount_inc(&mdev->refs);
 }
 
+bool mctp_dev_try_hold(struct mctp_dev *mdev)
+{
+	return mdev && refcount_inc_not_zero(&mdev->refs);
+}
+
 void mctp_dev_put(struct mctp_dev *mdev)
 {
 	if (mdev && refcount_dec_and_test(&mdev->refs)) {
diff --git a/net/mctp/route.c b/net/mctp/route.c
index 1f3dccbb7aed..3d0737030917 100644
--- a/net/mctp/route.c
+++ b/net/mctp/route.c
@@ -897,15 +897,16 @@ static mctp_eid_t mctp_dev_saddr(struct mctp_dev *dev)
 	return addr;
 }
 
-/* must only be called on a direct route, as the final output hop */
+/* must only be called on a direct route, as the final output hop, with a
+ * reference already held on route->dev.
+ */
 static void mctp_dst_from_route(struct mctp_dst *dst, mctp_eid_t eid,
 				mctp_eid_t saddr, unsigned int mtu,
-				struct mctp_route *route)
+				struct mctp_route *route, struct mctp_dev *dev)
 {
-	mctp_dev_hold(route->dev);
 	dst->nexthop = eid;
-	dst->dev = route->dev;
-	dst->mtu = READ_ONCE(dst->dev->dev->mtu);
+	dst->dev = dev;
+	dst->mtu = READ_ONCE(dev->dev->mtu);
 	if (mtu)
 		dst->mtu = min(dst->mtu, mtu);
 	dst->halen = 0;
@@ -998,14 +999,25 @@ int mctp_route_lookup(struct net *net, unsigned int dnet,
 			mtu = mtu ?: rt->mtu;
 
 		if (rt->dst_type == MCTP_ROUTE_DIRECT) {
-			mctp_eid_t saddr = mctp_dev_saddr(rt->dev);
+			struct mctp_dev *dev = rt->dev;
+			mctp_eid_t saddr;
+
+			if (!mctp_dev_try_hold(dev))
+				break;
+
+			saddr = mctp_dev_saddr(dev);
 
 			/* cannot do gateway-ed routes without a src  */
-			if (saddr == MCTP_ADDR_NULL && depth != 0)
+			if (saddr == MCTP_ADDR_NULL && depth != 0) {
+				mctp_dev_put(dev);
 				break;
+			}
 
 			if (dst)
-				mctp_dst_from_route(dst, daddr, saddr, mtu, rt);
+				mctp_dst_from_route(dst, daddr, saddr, mtu, rt,
+						    dev);
+			else
+				mctp_dev_put(dev);
 			rc = 0;
 			break;
 
-- 
2.34.1

Re: [PATCH] net: mctp: hold route device before source address lookup

[Jeremy Kerr]

Hi Yiqi Sun,

> mctp_route_lookup() walks the route table under RCU and may find a direct
> route whose route-table reference to rt->dev is being removed concurrently
> by NETDEV_UNREGISTER.
> 
> Since commit 22cb45afd221 ("net: mctp: perform source address lookups
> when we populate our dst"), the direct-route lookup path reads the source
> address with mctp_dev_saddr(rt->dev) before mctp_dst_from_route() takes the
> dst's own mctp_dev reference. If device teardown wins that race,
> mctp_route_remove_dev() can delete the route and mctp_route_release() can
> drop the route's mctp_dev reference to zero. mctp_dev_put() frees
> mdev->addrs synchronously, while only the mctp_dev body is deferred with
> kfree_rcu().
> 
> That leaves the lookup side able to read freed mdev->addrs in
> mctp_dev_saddr(), or later increment a zero refcount in mctp_dev_hold().
> 
> Add a try-hold helper for mctp_dev and use it before reading the source
> address from a direct route. mctp_dst_from_route() now consumes that held
> reference when it populates dst. If no dst is requested, drop the temporary
> reference in mctp_route_lookup(). Do the same when the route cannot be used
> for a gateway path without a source address.

Good find, thank you. Just a couple of comments on the fix though:

> index 1f3dccbb7aed..3d0737030917 100644
> --- a/net/mctp/route.c
> +++ b/net/mctp/route.c
> @@ -897,15 +897,16 @@ static mctp_eid_t mctp_dev_saddr(struct mctp_dev *dev)
>  	return addr;
>  }
>  
> -/* must only be called on a direct route, as the final output hop */
> +/* must only be called on a direct route, as the final output hop, with a
> + * reference already held on route->dev.
> + */
>  static void mctp_dst_from_route(struct mctp_dst *dst, mctp_eid_t eid,
>  				mctp_eid_t saddr, unsigned int mtu,
> -				struct mctp_route *route)
> +				struct mctp_route *route, struct mctp_dev *dev)
>  {
> -	mctp_dev_hold(route->dev);
>  	dst->nexthop = eid;
> -	dst->dev = route->dev;
> -	dst->mtu = READ_ONCE(dst->dev->dev->mtu);
> +	dst->dev = dev;
> +	dst->mtu = READ_ONCE(dev->dev->mtu);
>  	if (mtu)
>  		dst->mtu = min(dst->mtu, mtu);
>  	dst->halen = 0;

I don't see the need to pass the dev argument separately here, it's
duplicating route->dev; all we need is to drop the mctp_dev_hold(), and
update the comment, as you have done.

> @@ -998,14 +999,25 @@ int mctp_route_lookup(struct net *net, unsigned int dnet,
>  			mtu = mtu ?: rt->mtu;
>  
>  		if (rt->dst_type == MCTP_ROUTE_DIRECT) {
> -			mctp_eid_t saddr = mctp_dev_saddr(rt->dev);
> +			struct mctp_dev *dev = rt->dev;
> +			mctp_eid_t saddr;
> +
> +			if (!mctp_dev_try_hold(dev))
> +				break;

I would also be fine with using refcount_inc_not_zero() directly, but
adding the helper is good too.

> +
> +			saddr = mctp_dev_saddr(dev);
>  
>  			/* cannot do gateway-ed routes without a src  */
> -			if (saddr == MCTP_ADDR_NULL && depth != 0)
> +			if (saddr == MCTP_ADDR_NULL && depth != 0) {
> +				mctp_dev_put(dev);
>  				break;
> +			}
>  
>  			if (dst)
> -				mctp_dst_from_route(dst, daddr, saddr, mtu, rt);
> +				mctp_dst_from_route(dst, daddr, saddr, mtu, rt,
> +						    dev);
> +			else
> +				mctp_dev_put(dev);
>  			rc = 0;
>  			break;

If you end up doing a v2, don't forget the `[PATH net]` subject prefix,
to indicate the net (vs. net-next) tree.

Cheers,


Jeremy