Re: [PATCH 2/2] mac80211: probe unexercised mesh links

[Rajkumar Manoharan <rmanohar@codeaurora.org>]

On 2019-02-15 02:44, Johannes Berg wrote:
>>  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.
> 
Hmm.. I don't see any options other than this. no free slot in 
info_flags.
Will split this change.

>> 
>> +	/* 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
> 
Will move all the condition checks to cfg80211.

> 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?
> 
Hmm. I thought ieee80211_skb_resize in build_hdr will take care of it.

-Rajkumar