Message-ID: <0541dec472c83b173a486a924983a450ede7e923.camel@sipsolutions.net>
Subject: Re: [PATCH 2/2] mac80211: probe unexercised mesh links
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Rajkumar Manoharan <rmanohar@codeaurora.org>
Cc:     linux-wireless@vger.kernel.org, kevinhayes@google.com,
        julanhsu@google.com
Date:   Fri, 15 Feb 2019 11:44:21 +0100
In-Reply-To: <1550152570-13051-3-git-send-email-rmanohar@codeaurora.org>
References: <1550152570-13051-1-git-send-email-rmanohar@codeaurora.org>
         <1550152570-13051-3-git-send-email-rmanohar@codeaurora.org>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk


>  void __ieee80211_subif_start_xmit(struct sk_buff *skb,
>  				  struct net_device *dev,
> -				  u32 info_flags);
> +				  u32 info_flags,
> +				  u32 ctrl_flags);

I'd feel better if we could avoid all this, but if you really can't then
I guess we should split this out to a separate patch.

>  
> +	/* Allow injected packets to bypass mesh routing */
> +	if (info->control.flags & IEEE80211_TX_CTRL_SKIP_MPATH_LOOKUP)

unlikely?

> +int ieee80211_probe_mesh_link(struct wiphy *wiphy, struct net_device *dev,
> +			      const u8 *dest, const u8 *buf, size_t len)
> +{
> +	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> +	struct ieee80211_local *local = sdata->local;
> +	struct sta_info *sta;
> +	struct sk_buff *skb;
> +	struct ethhdr *ehdr;
> +
> +	if (len < sizeof(*ehdr))
> +		return -EINVAL;
> +
> +	mutex_lock(&local->sta_mtx);
> +	sta = sta_info_get_bss(sdata, dest);
> +	mutex_unlock(&local->sta_mtx);
> +
> +	if (!sta)
> +		return -ENOENT;

better add a comment here that the locking is fine because you only
check *existence* and don't use the sta pointer for anything else

> +	ehdr = (struct ethhdr *)buf;
> +	if (!ether_addr_equal(ehdr->h_dest, dest) ||

that check could be in cfg80211, but then why even bother passing the
"dest" separately?

> +	    !ether_addr_equal(ehdr->h_source, sdata->vif.addr) ||

probably this one too

> +	    is_multicast_ether_addr(ehdr->h_dest))

this one too

But also, ehdr isn't packed I think, you might have alignment issues
here as you don't know how the netlink message looks like? I think?

> +	if (ehdr->h_proto != htons(ETH_P_802_3))
> +		return -EINVAL;

same here

> +	skb = dev_alloc_skb(local->hw.extra_tx_headroom + len);

you should make it a bit bigger so header conversion will fit, I guess?

johannes