Re: [RFC] mac80211: fix rx monitor filter refcounters

[Johannes Berg <johannes@sipsolutions.net>]

On Tue, 2010-09-28 at 18:36 +0200, Christian Lamparter wrote:
> This patch fixes a refcounter & commit bug when monitor
> rx flags are changed by:
> 	iw dev moni set monitor [new flags]
> 
> while interface is up.
> 
> ---
> Is there a sane way to do that?

Is this not sane enough? Looks OK to me, even if it adds a bit of code.

> -	if (sdata->vif.type == NL80211_IFTYPE_MONITOR && flags)
> +	if (sdata->vif.type == NL80211_IFTYPE_MONITOR && flags) {
> +		struct ieee80211_local *local = sdata->local;
> +		u32 changed_flags = sdata->u.mntr_flags ^ *flags;
> +
>  		sdata->u.mntr_flags = *flags;
> +		if (changed_flags & MONITOR_FLAG_FCSFAIL) {
> +			if (*flags & MONITOR_FLAG_FCSFAIL)
> +				local->fif_fcsfail++;
> +			else
> +				local->fif_fcsfail--;
> +		}
> +		if (changed_flags & MONITOR_FLAG_PLCPFAIL) {
> +			if (*flags & MONITOR_FLAG_PLCPFAIL)
> +				local->fif_plcpfail++;
> +			else
> +				local->fif_plcpfail--;
> +		}
> +		if (changed_flags & MONITOR_FLAG_COOK_FRAMES) {
> +			if (*flags & MONITOR_FLAG_COOK_FRAMES)
> +				local->cooked_mntrs++;
> +			else
> +				local->cooked_mntrs--;
> +		}
> +		if (changed_flags & MONITOR_FLAG_OTHER_BSS) {
> +			if (*flags & MONITOR_FLAG_OTHER_BSS)
> +				local->fif_other_bss++;
> +			else
> +				local->fif_other_bss--;
> +		}
> +		if (changed_flags & MONITOR_FLAG_CONTROL) {
> +			if (*flags & MONITOR_FLAG_CONTROL) {
> +				local->fif_pspoll++;
> +				local->fif_control++;
> +			} else {
> +				local->fif_pspoll--;
> +				local->fif_control--;
> +			}
> +		}

Although, come to think of it, one could do something like this:

static void adjust_flags(local, flags, offset)
{
#define ADJUST(_flg, _fif)	do { 		\
	if (flags & MONITOR_FLAG_#_flg)		\
		local->fif_#_fif += offset;	\
	} while (0)

	ADJUST(FCSFAIL, fcsfail);
	ADJUST(PLCPFAIL, plcpfail);
	ADJUST(CONTROL, control);
	ADJUST(CONTROL, pspoll);
	ADJUST(OTHER_BSS, other_bss);
#undef ADJUST
}

and then we can have four callers of this function.

Two here:
adjust_flags(local, sdata->u.mntr_flags, -1);
adjust_flags(local, *flags, 1)
sdata->u.mntr_flags = *flags;

and the same two in ieee80211_do_open / ieee80211_do_stop.

johannes