Re: [PATCH 2/3] mac80211: support active monitor interfaces

[Johannes Berg <johannes@sipsolutions.net>]

On Sun, 2013-05-26 at 16:05 +0200, Felix Fietkau wrote:

>  	if (WARN_ON(sdata->vif.type == NL80211_IFTYPE_AP_VLAN ||
>  		    (sdata->vif.type == NL80211_IFTYPE_MONITOR &&
> -		     !(local->hw.flags & IEEE80211_HW_WANT_MONITOR_VIF))))
> +		     !(local->hw.flags & IEEE80211_HW_WANT_MONITOR_VIF) &&
> +			 !(sdata->u.mntr_flags & MONITOR_FLAG_ACTIVE))))
>  		return -EINVAL;

bad indentation

> +++ b/net/mac80211/iface.c
> @@ -209,9 +209,11 @@ static int ieee80211_change_mac(struct net_device *dev, void *addr)
>  	if (ieee80211_sdata_running(sdata))
>  		return -EBUSY;
>  
> -	ret = ieee80211_verify_mac(sdata->local, sa->sa_data);
> -	if (ret)
> -		return ret;
> +	if (sdata->vif.type != NL80211_IFTYPE_MONITOR) {
> +		ret = ieee80211_verify_mac(sdata->local, sa->sa_data);
> +		if (ret)
> +			return ret;
> +	}

I'm not convinced this makes sense, why would you want to have a monitor
with an invalid address? I'd rather not allow that, having e.g. a
multicast address that is ACK'ing frames would be very very strange.
 
> @@ -479,9 +481,13 @@ int ieee80211_do_open(struct wireless_dev *wdev, bool coming_up)
>  	case NL80211_IFTYPE_AP:
>  		sdata->bss = &sdata->u.ap;
>  		break;
> +	case NL80211_IFTYPE_MONITOR:
> +		if ((sdata->u.mntr_flags & MONITOR_FLAG_ACTIVE) &&
> +		    !(local->hw.flags & IEEE80211_HW_SUPPORTS_ACTIVE_MONITOR))
> +			return -EOPNOTSUPP;

How can you even get here? Shouldn't you just disallow setting the flag
unless it's supported?

> -		if (local->monitors == 0 && local->open_count == 0) {
> +		if (sdata->u.mntr_flags & MONITOR_FLAG_ACTIVE) {
> +			res = drv_add_interface(local, sdata);
> +			if (res)
> +				goto err_stop;
> +
> +			local->active_mntrs++;

Did I miss something, or don't you use the counter?

johannes