Re: [PATCH 2/2] mac80211: let unused MPP table entries timeout

[Johannes Berg <johannes@sipsolutions.net>]

On Tue, 2016-01-19 at 09:04 +0100, Henning Rogge wrote:

> +++ b/net/mac80211/mesh_pathtbl.c
> @@ -929,12 +929,55 @@ int mesh_path_del(struct ieee80211_sub_if_data
> *sdata, const u8 *addr)
>  		if (mpath->sdata == sdata &&
>  		    ether_addr_equal(addr, mpath->dst)) {
>  			__mesh_path_del(tbl, node);
> -			goto enddel;
> +			goto enddelpath;

C labels are scoped, why rename it?

>  		}
>  	}
>  
>  	err = -ENXIO;
> -enddel:
> +enddelpath:
> +	mesh_paths_generation++;
> +	spin_unlock(&tbl->hashwlock[hash_idx]);
> +	read_unlock_bh(&pathtbl_resize_lock);
> +	return err;
> +}
> +
> +/**
> + * mpp_path_del - delete a mesh proxy path from the table
> + *
> + * @addr: addr address (ETH_ALEN length)
> + * @sdata: local subif
> + *
> + * Returns: 0 if successful
> + */
> +static int mpp_path_del(struct ieee80211_sub_if_data *sdata, const
> u8 *addr)
> +{
> +	struct mesh_table *tbl;
> +	struct mesh_path *mpath;
> +	struct mpath_node *node;
> +	struct hlist_head *bucket;
> +	int hash_idx;
> +	int err = 0;
> +
> +	/* flush relevant mpp entries first */
> +	mpp_flush_by_proxy(sdata, addr);
> +
> +	read_lock_bh(&pathtbl_resize_lock);
> +	tbl = resize_dereference_mpp_paths();
> +	hash_idx = mesh_table_hash(addr, sdata, tbl);
> +	bucket = &tbl->hash_buckets[hash_idx];
> +
> +	spin_lock(&tbl->hashwlock[hash_idx]);
> +	hlist_for_each_entry(node, bucket, list) {
> +		mpath = node->mpath;
> +		if (mpath->sdata == sdata &&
> +		    ether_addr_equal(addr, mpath->dst)) {
> +			__mesh_path_del(tbl, node);
> +			goto enddelmpp;

This has the same use-after-free, I'd say?
And another instance later in this file.


> +					spin_lock_bh(&mppath-
> >state_lock);
> +					mppath->exp_time = jiffies;

That locking seems fairly pointless? You only write to this value here,
and it's an unsigned long, so in itself it's atomic and there's no
synchronisation against anything else needed.

johannes