Some hardware allow extended filtering of ARP frames not intended for
this host. To perform such filtering, the hardware needs to know the current
IP address(es) of the host, bound to its interface.
Add support for ARP filtering to mac80211 by adding a new op to the driver
interface, allowing to configure the current IP addresses. Call this op when
the interface is opened (as there may be an address already configured) and
whenever the IP addresses change.
This patch adds configuration of IPv4 addresses only, as IPv6 addresses don't
need ARP filtering. IPv6 support could be added later if someone comes up with
more frame types that can be filtered in hardware already.
Signed-off-by: Juuso Oikarinen <[email protected]>
---
include/net/mac80211.h | 9 ++++++++
net/mac80211/driver-ops.h | 12 ++++++++++
net/mac80211/ieee80211_i.h | 2 +
net/mac80211/iface.c | 1 +
net/mac80211/main.c | 49 +++++++++++++++++++++++++++++++++++++++++++-
5 files changed, 72 insertions(+), 1 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index de22cbf..2354956 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1468,6 +1468,8 @@ enum ieee80211_ampdu_mlme_action {
IEEE80211_AMPDU_TX_OPERATIONAL,
};
+struct in_ifaddr;
+
/**
* struct ieee80211_ops - callbacks from mac80211 to the driver
*
@@ -1535,6 +1537,11 @@ enum ieee80211_ampdu_mlme_action {
* of the bss parameters has changed when a call is made. The callback
* can sleep.
*
+ * @configure_ip_filter: Configuration function for IP address based filters,
+ * such as an ARP query filter. This function is called with all the IP
+ * addresses configured to the interface as argument - all frames targeted
+ * to any of these addresses should pass through.
+ *
* @prepare_multicast: Prepare for multicast filter configuration.
* This callback is optional, and its return value is passed
* to configure_filter(). This callback must be atomic.
@@ -1674,6 +1681,8 @@ struct ieee80211_ops {
struct ieee80211_vif *vif,
struct ieee80211_bss_conf *info,
u32 changed);
+ int (*configure_ip_filter)(struct ieee80211_hw *hw,
+ struct in_ifaddr *ifa_list);
u64 (*prepare_multicast)(struct ieee80211_hw *hw,
struct netdev_hw_addr_list *mc_list);
void (*configure_filter)(struct ieee80211_hw *hw,
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 4f22713..fff6aca 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -83,6 +83,18 @@ static inline void drv_bss_info_changed(struct ieee80211_local *local,
trace_drv_bss_info_changed(local, sdata, info, changed);
}
+struct in_ifaddr;
+static inline int drv_configure_ip_filter(struct ieee80211_hw *hw,
+ struct in_ifaddr *ifa_list)
+{
+ struct ieee80211_local *local = hw_to_local(hw);
+ int ret = 0;
+
+ if (local->ops->configure_ip_filter)
+ ret = local->ops->configure_ip_filter(hw, ifa_list);
+ return ret;
+}
+
static inline u64 drv_prepare_multicast(struct ieee80211_local *local,
struct netdev_hw_addr_list *mc_list)
{
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 1a9e2da..56e1331 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -851,6 +851,7 @@ struct ieee80211_local {
struct work_struct dynamic_ps_disable_work;
struct timer_list dynamic_ps_timer;
struct notifier_block network_latency_notifier;
+ struct notifier_block ifa_notifier;
int user_power_level; /* in dBm */
int power_constr_level; /* in dBm */
@@ -996,6 +997,7 @@ void ieee80211_send_pspoll(struct ieee80211_local *local,
void ieee80211_recalc_ps(struct ieee80211_local *local, s32 latency);
int ieee80211_max_network_latency(struct notifier_block *nb,
unsigned long data, void *dummy);
+int ieee80211_set_arp_filter(struct net_device *ndev);
void ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
struct ieee80211_channel_sw_ie *sw_elem,
struct ieee80211_bss *bss,
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 50deb01..5b488b8 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -330,6 +330,7 @@ static int ieee80211_open(struct net_device *dev)
if (sdata->vif.type == NL80211_IFTYPE_STATION)
ieee80211_queue_work(&local->hw, &sdata->u.mgd.work);
+ ieee80211_set_arp_filter(dev);
netif_tx_start_all_queues(dev);
return 0;
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 22a384d..f39e065 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -20,6 +20,7 @@
#include <linux/rtnetlink.h>
#include <linux/bitmap.h>
#include <linux/pm_qos_params.h>
+#include <linux/inetdevice.h>
#include <net/net_namespace.h>
#include <net/cfg80211.h>
@@ -329,6 +330,43 @@ static void ieee80211_recalc_smps_work(struct work_struct *work)
mutex_unlock(&local->iflist_mtx);
}
+int ieee80211_set_arp_filter(struct net_device *ndev)
+{
+ struct ieee80211_local *local;
+ struct in_device *idev;
+ int ret;
+
+ BUG_ON(!ndev);
+
+ idev = ndev->ip_ptr;
+ if (!idev)
+ return 0;
+
+ local = wdev_priv(ndev->ieee80211_ptr);
+ ret = drv_configure_ip_filter(&local->hw, idev->ifa_list);
+ return ret;
+}
+
+static int ieee80211_ifa_changed(struct notifier_block *nb,
+ unsigned long data, void *arg)
+{
+ struct in_ifaddr *ifa = arg;
+ struct ieee80211_local *local =
+ container_of(nb, struct ieee80211_local,
+ ifa_notifier);
+ struct net_device *ndev = ifa->ifa_dev->dev;
+ struct wireless_dev *wdev = ndev->ieee80211_ptr;
+
+ if (!wdev)
+ return NOTIFY_DONE;
+
+ if (wdev->wiphy != local->hw.wiphy)
+ return NOTIFY_DONE;
+
+ ieee80211_set_arp_filter(ndev);
+ return NOTIFY_DONE;
+}
+
struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
const struct ieee80211_ops *ops)
{
@@ -612,14 +650,22 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
ieee80211_max_network_latency;
result = pm_qos_add_notifier(PM_QOS_NETWORK_LATENCY,
&local->network_latency_notifier);
-
if (result) {
rtnl_lock();
goto fail_pm_qos;
}
+ local->ifa_notifier.notifier_call = ieee80211_ifa_changed;
+ result = register_inetaddr_notifier(&local->ifa_notifier);
+ if (result)
+ goto fail_ifa;
+
return 0;
+ fail_ifa:
+ pm_qos_remove_notifier(PM_QOS_NETWORK_LATENCY,
+ &local->network_latency_notifier);
+ rtnl_lock();
fail_pm_qos:
ieee80211_led_exit(local);
ieee80211_remove_interfaces(local);
@@ -647,6 +693,7 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
pm_qos_remove_notifier(PM_QOS_NETWORK_LATENCY,
&local->network_latency_notifier);
+ unregister_inetaddr_notifier(&local->ifa_notifier);
rtnl_lock();
--
1.6.3.3
On Tue, 2010-05-25 at 10:48 +0300, Juuso Oikarinen wrote:
> +struct in_ifaddr;
> +
I think you should include a header file for that, or was there a reason
you didn't?
> + * @configure_ip_filter: Configuration function for IP address based filters,
> + * such as an ARP query filter. This function is called with all the IP
> + * addresses configured to the interface as argument - all frames targeted
> + * to any of these addresses should pass through.
Huh ok I thought you wanted ARP filtering, not IP filtering. You need to
be more explicit about what this should do. IP filtering is not useful
since those frames will be unicast on the ethernet layer. I think this
should be more focused on ARP filtering.
Additionally, it needs to be very explicit that it must filter ONLY ARP
packets that do not match any of the given IP addresses. If, for
example, the hardware can only handle 2 addresses, but you have 3 in the
list, the filter must be turned off. Any packet that the device cannot
parse properly must also be passed through. All such things should be
documented.
> +struct in_ifaddr;
You should get the right include too.
> +static inline int drv_configure_ip_filter(struct ieee80211_hw *hw,
> + struct in_ifaddr *ifa_list)
> +{
> + struct ieee80211_local *local = hw_to_local(hw);
> + int ret = 0;
> +
> + if (local->ops->configure_ip_filter)
> + ret = local->ops->configure_ip_filter(hw, ifa_list);
> + return ret;
> +}
Tracing would be nice, you should even able able to trace all addresses
in a variable-length array.
> @@ -330,6 +330,7 @@ static int ieee80211_open(struct net_device *dev)
> if (sdata->vif.type == NL80211_IFTYPE_STATION)
> ieee80211_queue_work(&local->hw, &sdata->u.mgd.work);
>
> + ieee80211_set_arp_filter(dev);
That seems unnecessary if drivers assume that there are no addresses to
start with?
BTW, how will drivers deal with getting this while unassociated? If, for
example, I set an address before associating, you'll get it while
unassociated and not again when associated. Another thing to document --
driver needs to handle that, DHCP is not everything :)
> +static int ieee80211_ifa_changed(struct notifier_block *nb,
> + unsigned long data, void *arg)
> +{
> + struct in_ifaddr *ifa = arg;
> + struct ieee80211_local *local =
> + container_of(nb, struct ieee80211_local,
> + ifa_notifier);
> + struct net_device *ndev = ifa->ifa_dev->dev;
> + struct wireless_dev *wdev = ndev->ieee80211_ptr;
> +
> + if (!wdev)
> + return NOTIFY_DONE;
> +
> + if (wdev->wiphy != local->hw.wiphy)
> + return NOTIFY_DONE;
> +
> + ieee80211_set_arp_filter(ndev);
> + return NOTIFY_DONE;
> +}
This is obviously broken when you have multiple virtual interfaces, so
you either need to build a common list of IP addresses, or punt the
problem to the driver and give the callback an ieee80211_vif argument
and clearly document that the driver will have to keep track of it for
each interface.
johannes
On Tue, 2010-05-25 at 11:13 +0200, ext Johannes Berg wrote:
> On Tue, 2010-05-25 at 10:48 +0300, Juuso Oikarinen wrote:
>
> > +struct in_ifaddr;
> > +
>
> I think you should include a header file for that, or was there a reason
> you didn't?
Yeah, I'll add the header.
> > + * @configure_ip_filter: Configuration function for IP address based filters,
> > + * such as an ARP query filter. This function is called with all the IP
> > + * addresses configured to the interface as argument - all frames targeted
> > + * to any of these addresses should pass through.
>
> Huh ok I thought you wanted ARP filtering, not IP filtering. You need to
> be more explicit about what this should do. IP filtering is not useful
> since those frames will be unicast on the ethernet layer. I think this
> should be more focused on ARP filtering.
True, this is about ARP filtering. I was pondering naming it arp or IP,
then opted to go with IP just because I was thinking that there may be
some other frames we might want to filter based on IP in the future.
Maybe I'm thinking too far ahead ;)
I'll update the naming to be ARP all around.
> Additionally, it needs to be very explicit that it must filter ONLY ARP
> packets that do not match any of the given IP addresses. If, for
> example, the hardware can only handle 2 addresses, but you have 3 in the
> list, the filter must be turned off. Any packet that the device cannot
> parse properly must also be passed through. All such things should be
> documented.
This is a valid point. That's actually what the wl1271 driver does - as
the HW only supports one address for its filter. I'll add some text for
this.
> > +struct in_ifaddr;
>
> You should get the right include too.
>
> > +static inline int drv_configure_ip_filter(struct ieee80211_hw *hw,
> > + struct in_ifaddr *ifa_list)
> > +{
> > + struct ieee80211_local *local = hw_to_local(hw);
> > + int ret = 0;
> > +
> > + if (local->ops->configure_ip_filter)
> > + ret = local->ops->configure_ip_filter(hw, ifa_list);
> > + return ret;
> > +}
>
> Tracing would be nice, you should even able able to trace all addresses
> in a variable-length array.
Yeah, I was lazy. I will look into this :P
>
> > @@ -330,6 +330,7 @@ static int ieee80211_open(struct net_device *dev)
> > if (sdata->vif.type == NL80211_IFTYPE_STATION)
> > ieee80211_queue_work(&local->hw, &sdata->u.mgd.work);
> >
> > + ieee80211_set_arp_filter(dev);
>
> That seems unnecessary if drivers assume that there are no addresses to
> start with?
>
The drivers assume there is no address to start with - this is for the
scenario when there is, so that the driver enables the filtering from
scratch. You can do this, for instance:
ifconfig wlan0 up 192.168.1.1
ifconfig wlan0 down
ifconfig wlan0 up
The address 192.168.1.1 will be there already on ifconfig wlan0 up.
> BTW, how will drivers deal with getting this while unassociated? If, for
> example, I set an address before associating, you'll get it while
> unassociated and not again when associated. Another thing to document --
> driver needs to handle that, DHCP is not everything :)
This is actually a good point. The wl1271 driver configures the ARP
filter immediately - associated or not - once the chipset is booted up
(as above.) This is indeed not very generic.
I will move the set_arp_filter -thingy from _open() to when the we are
associated. That way we get the initial IP address (if there happen to
be any) from start when the address is actually needed.
>
> > +static int ieee80211_ifa_changed(struct notifier_block *nb,
> > + unsigned long data, void *arg)
> > +{
> > + struct in_ifaddr *ifa = arg;
> > + struct ieee80211_local *local =
> > + container_of(nb, struct ieee80211_local,
> > + ifa_notifier);
> > + struct net_device *ndev = ifa->ifa_dev->dev;
> > + struct wireless_dev *wdev = ndev->ieee80211_ptr;
> > +
> > + if (!wdev)
> > + return NOTIFY_DONE;
> > +
> > + if (wdev->wiphy != local->hw.wiphy)
> > + return NOTIFY_DONE;
> > +
> > + ieee80211_set_arp_filter(ndev);
> > + return NOTIFY_DONE;
> > +}
>
> This is obviously broken when you have multiple virtual interfaces, so
> you either need to build a common list of IP addresses, or punt the
> problem to the driver and give the callback an ieee80211_vif argument
> and clearly document that the driver will have to keep track of it for
> each interface.
Good point. I will "punt" (never heard this expression before!) the
problem to the driver with a vif pointer.
Thanks for your review. :)
-Juuso
> johannes
>
On Tue, 2010-05-25 at 13:25 +0200, ext Johannes Berg wrote:
> On Tue, 2010-05-25 at 12:31 +0300, Juuso Oikarinen wrote:
>
> > > BTW, how will drivers deal with getting this while unassociated? If, for
> > > example, I set an address before associating, you'll get it while
> > > unassociated and not again when associated. Another thing to document --
> > > driver needs to handle that, DHCP is not everything :)
> >
> > This is actually a good point. The wl1271 driver configures the ARP
> > filter immediately - associated or not - once the chipset is booted up
> > (as above.) This is indeed not very generic.
> >
> > I will move the set_arp_filter -thingy from _open() to when the we are
> > associated. That way we get the initial IP address (if there happen to
> > be any) from start when the address is actually needed.
>
> But then you would still get a callback way before the hardware is even
> started if you set the address before ifup ... mac80211 should probably
> catch that. Or does the networking stack already defer it then?
True again. The call-backs will not get deferred by the stack. That must
also be handled.
> Another thing that came to mind -- how about locking? Will the callback
> have to be atomic? Can you really access idev->ifa_list without any
> locking at all?
AFAIK the callback need not be atomic. I will need to check about the
locking, but we probably do need that.
-Juuso
> > > This is obviously broken when you have multiple virtual interfaces, so
> > > you either need to build a common list of IP addresses, or punt the
> > > problem to the driver and give the callback an ieee80211_vif argument
> > > and clearly document that the driver will have to keep track of it for
> > > each interface.
> >
> > Good point. I will "punt" (never heard this expression before!)
>
> Don't take my word for it ... I heard that once but can't find reference
> to it being a correct use of the word ;)
>
> johannes
>
On Tue, 2010-05-25 at 11:13 +0200, ext Johannes Berg wrote:
> On Tue, 2010-05-25 at 10:48 +0300, Juuso Oikarinen wrote:
>
> You should get the right include too.
>
> > +static inline int drv_configure_ip_filter(struct ieee80211_hw *hw,
> > + struct in_ifaddr *ifa_list)
> > +{
> > + struct ieee80211_local *local = hw_to_local(hw);
> > + int ret = 0;
> > +
> > + if (local->ops->configure_ip_filter)
> > + ret = local->ops->configure_ip_filter(hw, ifa_list);
> > + return ret;
> > +}
>
> Tracing would be nice, you should even able able to trace all addresses
> in a variable-length array.
>
I looked into the tracing. I still prefer using the ifa_list directly as
argument to the driver, and not copy the addresses in it to another
array.
The ifa_list is a linked list, and does AFAIK does not directly fit into
the tracing infrasturcture.
Hence I added a trace for this op, but omitted tracing the IP addresses.
I will sent an RFC patch with this shortly.
-Juuso
> johannes
>
On Wed, 2010-05-26 at 14:14 +0300, Juuso Oikarinen wrote:
> > > +static inline int drv_configure_ip_filter(struct ieee80211_hw *hw,
> > > + struct in_ifaddr *ifa_list)
> > > +{
> > > + struct ieee80211_local *local = hw_to_local(hw);
> > > + int ret = 0;
> > > +
> > > + if (local->ops->configure_ip_filter)
> > > + ret = local->ops->configure_ip_filter(hw, ifa_list);
> > > + return ret;
> > > +}
> >
> > Tracing would be nice, you should even able able to trace all addresses
> > in a variable-length array.
> >
>
> I looked into the tracing. I still prefer using the ifa_list directly as
> argument to the driver, and not copy the addresses in it to another
> array.
>
> The ifa_list is a linked list, and does AFAIK does not directly fit into
> the tracing infrasturcture.
>
> Hence I added a trace for this op, but omitted tracing the IP addresses.
Yeah, that's fine ... it later occurred to me that I also left that out
for the multicast list for exactly this reason.
johannes
On Tue, 2010-05-25 at 12:31 +0300, Juuso Oikarinen wrote:
> > BTW, how will drivers deal with getting this while unassociated? If, for
> > example, I set an address before associating, you'll get it while
> > unassociated and not again when associated. Another thing to document --
> > driver needs to handle that, DHCP is not everything :)
>
> This is actually a good point. The wl1271 driver configures the ARP
> filter immediately - associated or not - once the chipset is booted up
> (as above.) This is indeed not very generic.
>
> I will move the set_arp_filter -thingy from _open() to when the we are
> associated. That way we get the initial IP address (if there happen to
> be any) from start when the address is actually needed.
But then you would still get a callback way before the hardware is even
started if you set the address before ifup ... mac80211 should probably
catch that. Or does the networking stack already defer it then?
Another thing that came to mind -- how about locking? Will the callback
have to be atomic? Can you really access idev->ifa_list without any
locking at all?
> > This is obviously broken when you have multiple virtual interfaces, so
> > you either need to build a common list of IP addresses, or punt the
> > problem to the driver and give the callback an ieee80211_vif argument
> > and clearly document that the driver will have to keep track of it for
> > each interface.
>
> Good point. I will "punt" (never heard this expression before!)
Don't take my word for it ... I heard that once but can't find reference
to it being a correct use of the word ;)
johannes