Re: [PATCH] net: mac80211: tx.c: be sure of 'sdata->vif.type' must be NL80211_IFTYPE_AP when be in NL80211_IFTYPE_AP case

Johannes Berg
2013-11-30T12:53:56Z

On Sat, 2013-11-30 at 19:59 +0800, Chen Gang wrote:
> On 11/29/2013 11:38 PM, Johannes Berg wrote:
> >
> >> +++ b/net/mac80211/tx.c
> >> @@ -1814,8 +1814,9 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
> >> break;
> >> /* fall through */
> >> case NL80211_IFTYPE_AP:
> >> - if (sdata->vif.type == NL80211_IFTYPE_AP)
> >> - chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> >> + if (sdata->vif.type != NL80211_IFTYPE_AP)
> >> + goto fail_rcu;
> >> + chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> >
> > This change is completely wrong.
> >
>
> Oh, it is.
>
> Hmm... for me, this work flow still can be implemented with a little
> clearer way (at least it will avoid related warning):
>
> -------------------------diff begin------------------------------
>
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index c558b24..7076128 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -1810,14 +1810,14 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
> if (!chanctx_conf)
> goto fail_rcu;
> band = chanctx_conf->def.chan->band;
> - if (sta)
> - break;
> - /* fall through */
> + if (!sta)
> + goto try_next;
> + break;
> case NL80211_IFTYPE_AP:
> - if (sdata->vif.type == NL80211_IFTYPE_AP)
> - chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> + chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> if (!chanctx_conf)
> goto fail_rcu;
> +try_next:

I don't think that's better than the (fairly obvious) fall-through, and
has a pretty odd goto. Also, depending on the compiler, it still knows
the previous case label and doesn't warn.

johannes