netdev - Re: [PATCH v6 4/4] mac80211: multicast to unicast conversion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1477571393.4534.11.camel@sipsolutions.net>
Date:   Thu, 27 Oct 2016 14:29:53 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Michael Braun <michael-dev@...i-braun.de>
Cc:     linux-wireless@...r.kernel.org, projekt-wlan@....tu-ilmenau.de,
        netdev@...r.kernel.org
Subject: Re: [PATCH v6 4/4] mac80211: multicast to unicast conversion


> @@ -2242,6 +2242,20 @@ static int ieee80211_set_wds_peer(struct wiphy
> *wiphy, struct net_device *dev,
>  	return 0;
>  }
>  
> +static int ieee80211_set_multicast_to_unicast(struct wiphy *wiphy,
> +					      struct net_device
> *dev,
> +					      const bool enabled)

I don't understand why you put this with set_wds_peer, please add it
here also at the end like anywhere else - I've changed that for you in
the cfg80211 patch.

> +	struct ieee80211_sub_if_data *sdata =
> IEEE80211_DEV_TO_SUB_IF(dev);
> +
> +	if (sdata->vif.type != NL80211_IFTYPE_AP)
> +		return -1;

Not needed. Also, don't ever just blindly return -1 in the kernel, that
means -EPERM, i.e. permission denied.

> +/* rewrite destination mac address */

that's a pretty useless comment ...

> +/* Check if multicast to unicast conversion is needed and do it.
> + * Returns 1 if skb was freed and should not be send out.

Follow kernel convention of returning a negative error code, e.g.
-ENOTCONN in this case, and remove this comment.

> +	/* clone packets and update destination mac */
> +	list_for_each_entry_rcu(sta, &local->sta_list, list) {
> +		if (sdata != sta->sdata)
> +			continue;
> +		if (unlikely(!ether_addr_equal(eth->h_source, sta-
> >sta.addr)))
> +			/* do not send back to source */
> +			continue;
> +		if (!prev) {
> +			prev = sta;
> +			continue;
> +		}
> +		cloned_skb = skb_clone(skb, GFP_ATOMIC);
> +		if (unlikely(ieee80211_change_da(cloned_skb, prev)))
> {
> +			dev_kfree_skb(cloned_skb);
> +			continue;
> +		}
> +		__ieee80211_subif_start_xmit(cloned_skb, cloned_skb-
> >dev, 0);

I don't like the recursion here.

I think we can call this from ieee80211_subif_start_xmit(), and then do
something like

if (unlikely(multicast)) {
	struct skb_queue queue;

	__skb_queue_head_init(&queue);
	convert_frames(
	while ((skb = __skb_dequeue(&queue))
		__ieee80211_subif_start_xmit(...);
} else {
	__ieee80211_subif_start_xmit(...);
}

Yes, that's a little less efficient (RCU stuff, although that could
even move in theory), but it avoids the recursion which imho is better.

johannes