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

[Jeremy Kerr <jk@codeconstruct.com.au>]

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