This device takes over an existing network device and produces a
new one that appears like a wireless connection, returning enough canned
responses to nl80211 to satisfy a standard connection manager. If
necessary, it can also be set up one step removed from an existing
network device, such as through a vlan/80211Q or macvlan connection to
not disrupt the existing network interface.
To use it to wrap a bare ethernet connection:
ip link add link eth0 name wlan0 type virt_wifi
You may have to rename or otherwise hide the eth0 from your connection
manager, as the original network link will become unusuable and only
the wireless wrapper will be functional. This can also be combined with
vlan or macvlan links on top of eth0 to share the network between
distinct links, but that requires support outside the machine for
accepting vlan-tagged packets or packets from multiple MAC addresses.
This is being used for Google's Remote Android Virtual Device project,
which runs Android devices in virtual machines. The standard network
interfaces provided inside the virtual machines are all ethernet.
However, Android is not interested in ethernet devices and would rather
connect to a wireless interface. This patch allows the virtual machine
guest to treat one of its network connections as wireless rather than
ethernet, satisfying Android's network connection requirements.
We believe this is a generally useful driver for simulating wireless
network connections in other environments where a wireless connection is
desired by some userspace process but is not available.
This is distinct from other testing efforts such as mac80211_hwsim by
being a cfg80211 device instead of mac80211 device, allowing straight
pass-through on the data plane instead of forcing packaging of ethernet
data into mac80211 frames.
Signed-off-by: A. Cody Schuffelen <[email protected]>
Acked-by: Alistair Strachan <[email protected]>
Acked-by: Greg Hartman <[email protected]>
---
First version: https://lkml.org/lkml/2018/7/27/947
First review: https://lore.kernel.org/lkml/[email protected]/
Second version: https://lore.kernel.org/lkml/[email protected]/
Second review: https://www.lkml.org/lkml/2018/9/27/228
Second review: https://www.lkml.org/lkml/2018/9/27/229
Second review: https://www.lkml.org/lkml/2018/9/27/669
Thanks for all the comments on v2! I believe I've addressed all of them
here. I've also added changes to react better to the netdev going down,
canceling ongoing scans and rejecting wifi network connection requests.
I wasn't completely clear on whether I should change the title (net-next
to mac80211-next) so I left it as is for v3 to try to keep the patchwork
series together.
The driver is also now a bool instead of a tristate to use __ro_after_init.
drivers/net/wireless/Kconfig | 7 +
drivers/net/wireless/Makefile | 2 +
drivers/net/wireless/virt_wifi.c | 618 +++++++++++++++++++++++++++++++
3 files changed, 627 insertions(+)
create mode 100644 drivers/net/wireless/virt_wifi.c
diff --git a/drivers/net/wireless/Kconfig b/drivers/net/wireless/Kconfig
index 166920ae23f8..f4e808411634 100644
--- a/drivers/net/wireless/Kconfig
+++ b/drivers/net/wireless/Kconfig
@@ -114,4 +114,11 @@ config USB_NET_RNDIS_WLAN
If you choose to build a module, it'll be called rndis_wlan.
+config VIRT_WIFI
+ bool "Wifi wrapper for ethernet drivers"
+ depends on CFG80211
+ ---help---
+ This option adds support for ethernet connections to appear as if they
+ are wifi connections through a special rtnetlink device.
+
endif # WLAN
diff --git a/drivers/net/wireless/Makefile b/drivers/net/wireless/Makefile
index 7fc96306712a..6cfe74515c95 100644
--- a/drivers/net/wireless/Makefile
+++ b/drivers/net/wireless/Makefile
@@ -27,3 +27,5 @@ obj-$(CONFIG_PCMCIA_WL3501) += wl3501_cs.o
obj-$(CONFIG_USB_NET_RNDIS_WLAN) += rndis_wlan.o
obj-$(CONFIG_MAC80211_HWSIM) += mac80211_hwsim.o
+
+obj-$(CONFIG_VIRT_WIFI) += virt_wifi.o
diff --git a/drivers/net/wireless/virt_wifi.c b/drivers/net/wireless/virt_wifi.c
new file mode 100644
index 000000000000..b25c1313d0e7
--- /dev/null
+++ b/drivers/net/wireless/virt_wifi.c
@@ -0,0 +1,618 @@
+// SPDX-License-Identifier: GPL-2.0
+/* drivers/net/wireless/virt_wifi.c
+ *
+ * A fake implementation of cfg80211_ops that can be tacked on to an ethernet
+ * net_device to make it appear as a wireless connection.
+ *
+ * Copyright (C) 2018 Google, Inc.
+ *
+ * Author: [email protected]
+ */
+
+#include <net/cfg80211.h>
+#include <net/rtnetlink.h>
+#include <linux/etherdevice.h>
+#include <linux/module.h>
+
+struct virt_wifi_priv {
+ bool being_deleted;
+ bool is_connected;
+ bool netdev_is_up;
+ struct net_device *netdev;
+ struct cfg80211_scan_request *scan_request;
+ u8 connect_requested_bss[ETH_ALEN];
+ struct delayed_work scan_result;
+ struct delayed_work connect;
+ struct delayed_work disconnect;
+ u16 disconnect_reason;
+};
+
+static struct ieee80211_channel channel_2ghz = {
+ .band = NL80211_BAND_2GHZ,
+ .center_freq = 2432,
+ .hw_value = 2432,
+ .max_power = 20,
+};
+
+static struct ieee80211_rate bitrates_2ghz[] = {
+ { .bitrate = 10 },
+ { .bitrate = 20 },
+ { .bitrate = 55 },
+ { .bitrate = 60 },
+ { .bitrate = 110 },
+ { .bitrate = 120 },
+ { .bitrate = 240 },
+};
+
+static struct ieee80211_supported_band band_2ghz = {
+ .channels = &channel_2ghz,
+ .bitrates = bitrates_2ghz,
+ .band = NL80211_BAND_2GHZ,
+ .n_channels = 1,
+ .n_bitrates = ARRAY_SIZE(bitrates_2ghz),
+ .ht_cap = {
+ .ht_supported = true,
+ .cap = IEEE80211_HT_CAP_SUP_WIDTH_20_40 |
+ IEEE80211_HT_CAP_GRN_FLD |
+ IEEE80211_HT_CAP_SGI_20 |
+ IEEE80211_HT_CAP_SGI_40 |
+ IEEE80211_HT_CAP_DSSSCCK40,
+ .ampdu_factor = 0x3,
+ .ampdu_density = 0x6,
+ .mcs = {
+ .rx_mask = {0xff, 0xff},
+ .tx_params = IEEE80211_HT_MCS_TX_DEFINED,
+ },
+ },
+};
+
+static struct ieee80211_channel channel_5ghz = {
+ .band = NL80211_BAND_5GHZ,
+ .center_freq = 5240,
+ .hw_value = 5240,
+ .max_power = 20,
+};
+
+static struct ieee80211_rate bitrates_5ghz[] = {
+ { .bitrate = 60 },
+ { .bitrate = 120 },
+ { .bitrate = 240 },
+};
+
+#define RX_MCS_MAP \
+ cpu_to_le16(IEEE80211_VHT_MCS_SUPPORT_0_9 << 0 | \
+ IEEE80211_VHT_MCS_SUPPORT_0_9 << 2 | \
+ IEEE80211_VHT_MCS_SUPPORT_0_9 << 4 | \
+ IEEE80211_VHT_MCS_SUPPORT_0_9 << 6 | \
+ IEEE80211_VHT_MCS_SUPPORT_0_9 << 8 | \
+ IEEE80211_VHT_MCS_SUPPORT_0_9 << 10 | \
+ IEEE80211_VHT_MCS_SUPPORT_0_9 << 12 | \
+ IEEE80211_VHT_MCS_SUPPORT_0_9 << 14)
+
+#define TX_MCS_MAP \
+ cpu_to_le16(IEEE80211_VHT_MCS_SUPPORT_0_9 << 0 | \
+ IEEE80211_VHT_MCS_SUPPORT_0_9 << 2 | \
+ IEEE80211_VHT_MCS_SUPPORT_0_9 << 4 | \
+ IEEE80211_VHT_MCS_SUPPORT_0_9 << 6 | \
+ IEEE80211_VHT_MCS_SUPPORT_0_9 << 8 | \
+ IEEE80211_VHT_MCS_SUPPORT_0_9 << 10 | \
+ IEEE80211_VHT_MCS_SUPPORT_0_9 << 12 | \
+ IEEE80211_VHT_MCS_SUPPORT_0_9 << 14) \
+
+static struct ieee80211_supported_band band_5ghz = {
+ .channels = &channel_5ghz,
+ .bitrates = bitrates_5ghz,
+ .band = NL80211_BAND_5GHZ,
+ .n_channels = 1,
+ .n_bitrates = ARRAY_SIZE(bitrates_5ghz),
+ .ht_cap = {
+ .ht_supported = true,
+ .cap = IEEE80211_HT_CAP_SUP_WIDTH_20_40 |
+ IEEE80211_HT_CAP_GRN_FLD |
+ IEEE80211_HT_CAP_SGI_20 |
+ IEEE80211_HT_CAP_SGI_40 |
+ IEEE80211_HT_CAP_DSSSCCK40,
+ .ampdu_factor = 0x3,
+ .ampdu_density = 0x6,
+ .mcs = {
+ .rx_mask = {0xff, 0xff},
+ .tx_params = IEEE80211_HT_MCS_TX_DEFINED,
+ },
+ },
+ .vht_cap = {
+ .vht_supported = true,
+ .cap = IEEE80211_VHT_CAP_MAX_MPDU_LENGTH_11454 |
+ IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160_80PLUS80MHZ |
+ IEEE80211_VHT_CAP_RXLDPC |
+ IEEE80211_VHT_CAP_SHORT_GI_80 |
+ IEEE80211_VHT_CAP_SHORT_GI_160 |
+ IEEE80211_VHT_CAP_TXSTBC |
+ IEEE80211_VHT_CAP_RXSTBC_1 |
+ IEEE80211_VHT_CAP_RXSTBC_2 |
+ IEEE80211_VHT_CAP_RXSTBC_3 |
+ IEEE80211_VHT_CAP_RXSTBC_4 |
+ IEEE80211_VHT_CAP_MAX_A_MPDU_LENGTH_EXPONENT_MASK,
+ .vht_mcs = {
+ .rx_mcs_map = RX_MCS_MAP,
+ .tx_mcs_map = TX_MCS_MAP,
+ }
+ },
+};
+
+/** Assigned at module init. Guaranteed locally-administered and unicast. */
+static u8 fake_router_bssid[ETH_ALEN] __ro_after_init = {};
+
+static int virt_wifi_scan(struct wiphy *wiphy,
+ struct cfg80211_scan_request *request)
+{
+ struct virt_wifi_priv *priv = wiphy_priv(wiphy);
+
+ wiphy_debug(wiphy, "scan\n");
+
+ if (priv->scan_request || priv->being_deleted)
+ return -EBUSY;
+
+ priv->scan_request = request;
+ schedule_delayed_work(&priv->scan_result, HZ * 2);
+
+ return 0;
+}
+
+static void virt_wifi_scan_result(struct work_struct *work)
+{
+ const union {
+ struct {
+ u8 tag;
+ u8 len;
+ u8 ssid[8];
+ } __packed parts;
+ u8 data[10];
+ } ssid = { .parts = {
+ .tag = WLAN_EID_SSID, .len = 8, .ssid = "VirtWifi" }
+ };
+ struct cfg80211_bss *informed_bss;
+ struct virt_wifi_priv *priv =
+ container_of(work, struct virt_wifi_priv,
+ scan_result.work);
+ struct wiphy *wiphy = priv_to_wiphy(priv);
+ struct cfg80211_inform_bss mock_inform_bss = {
+ .chan = &channel_5ghz,
+ .scan_width = NL80211_BSS_CHAN_WIDTH_20,
+ .signal = -60,
+ .boottime_ns = ktime_get_boot_ns(),
+ };
+ struct cfg80211_scan_info scan_info = {};
+
+ informed_bss =
+ cfg80211_inform_bss_data(wiphy, &mock_inform_bss,
+ CFG80211_BSS_FTYPE_PRESP,
+ fake_router_bssid,
+ mock_inform_bss.boottime_ns,
+ WLAN_CAPABILITY_ESS, 0, ssid.data,
+ sizeof(ssid), GFP_KERNEL);
+ cfg80211_put_bss(wiphy, informed_bss);
+
+ rtnl_lock();
+ if (priv->scan_request) {
+ cfg80211_scan_done(priv->scan_request, &scan_info);
+ priv->scan_request = NULL;
+ }
+ rtnl_unlock();
+}
+
+static int virt_wifi_connect(struct wiphy *wiphy,
+ struct net_device *netdev,
+ struct cfg80211_connect_params *sme)
+{
+ struct virt_wifi_priv *priv = wiphy_priv(wiphy);
+ bool could_schedule;
+
+ if (priv->being_deleted)
+ return -EBUSY;
+
+ wiphy_debug(wiphy, "connect\n");
+ could_schedule = schedule_delayed_work(&priv->connect, HZ * 2);
+ if (!could_schedule)
+ return -EBUSY;
+
+ if (sme->bssid)
+ ether_addr_copy(priv->connect_requested_bss, sme->bssid);
+ else
+ eth_zero_addr(priv->connect_requested_bss);
+ return 0;
+}
+
+static void virt_wifi_connect_complete(struct work_struct *work)
+{
+ struct virt_wifi_priv *priv =
+ container_of(work, struct virt_wifi_priv, connect.work);
+ u8 *requested_bss = priv->connect_requested_bss;
+ bool has_addr = !is_zero_ether_addr(requested_bss);
+ bool right_addr = ether_addr_equal(requested_bss, fake_router_bssid);
+ u16 status = WLAN_STATUS_SUCCESS;
+
+ rtnl_lock();
+ if (!priv->netdev_is_up || (has_addr && !right_addr))
+ status = WLAN_STATUS_UNSPECIFIED_FAILURE;
+ else
+ priv->is_connected = true;
+
+ cfg80211_connect_result(priv->netdev, requested_bss, NULL, 0, NULL, 0,
+ status, GFP_KERNEL);
+ rtnl_unlock();
+}
+
+static int virt_wifi_disconnect(struct wiphy *wiphy, struct net_device *netdev,
+ u16 reason_code)
+{
+ struct virt_wifi_priv *priv = wiphy_priv(wiphy);
+ bool could_schedule;
+
+ if (priv->being_deleted)
+ return -EBUSY;
+
+ wiphy_debug(wiphy, "disconnect\n");
+ could_schedule = schedule_delayed_work(&priv->disconnect, HZ * 2);
+ if (!could_schedule)
+ return -EBUSY;
+ priv->disconnect_reason = reason_code;
+ return 0;
+}
+
+static void virt_wifi_disconnect_complete(struct work_struct *work)
+{
+ struct virt_wifi_priv *priv =
+ container_of(work, struct virt_wifi_priv, disconnect.work);
+
+ cfg80211_disconnected(priv->netdev, priv->disconnect_reason, NULL, 0,
+ true, GFP_KERNEL);
+ priv->is_connected = false;
+}
+
+static int virt_wifi_get_station(struct wiphy *wiphy,
+ struct net_device *dev,
+ const u8 *mac,
+ struct station_info *sinfo)
+{
+ wiphy_debug(wiphy, "get_station\n");
+
+ if (!ether_addr_equal(mac, fake_router_bssid))
+ return -ENOENT;
+
+ sinfo->filled = BIT(NL80211_STA_INFO_TX_PACKETS) |
+ BIT(NL80211_STA_INFO_TX_FAILED) | BIT(NL80211_STA_INFO_SIGNAL) |
+ BIT(NL80211_STA_INFO_TX_BITRATE);
+ sinfo->tx_packets = 1;
+ sinfo->tx_failed = 0;
+ sinfo->signal = -60;
+ sinfo->txrate = (struct rate_info) {
+ .legacy = 10, /* units are 100kbit/s */
+ };
+ return 0;
+}
+
+static int virt_wifi_dump_station(struct wiphy *wiphy,
+ struct net_device *dev,
+ int idx,
+ u8 *mac,
+ struct station_info *sinfo)
+{
+ wiphy_debug(wiphy, "dump_station\n");
+
+ if (idx != 0)
+ return -ENOENT;
+
+ ether_addr_copy(mac, fake_router_bssid);
+ return virt_wifi_get_station(wiphy, dev, fake_router_bssid, sinfo);
+}
+
+static const struct cfg80211_ops virt_wifi_cfg80211_ops = {
+ .scan = virt_wifi_scan,
+
+ .connect = virt_wifi_connect,
+ .disconnect = virt_wifi_disconnect,
+
+ .get_station = virt_wifi_get_station,
+ .dump_station = virt_wifi_dump_station,
+};
+
+static struct wireless_dev *virt_wireless_dev(struct device *device,
+ struct net_device *netdev)
+{
+ struct wireless_dev *wdev;
+ struct wiphy *wiphy;
+ struct virt_wifi_priv *priv;
+
+ wdev = kzalloc(sizeof(*wdev), GFP_KERNEL);
+
+ if (!wdev)
+ return ERR_PTR(-ENOMEM);
+
+ wdev->iftype = NL80211_IFTYPE_STATION;
+ wiphy = wiphy_new(&virt_wifi_cfg80211_ops, sizeof(*priv));
+
+ if (!wiphy) {
+ kfree(wdev);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ wdev->wiphy = wiphy;
+
+ wiphy->max_scan_ssids = 4;
+ wiphy->max_scan_ie_len = 1000;
+ wiphy->signal_type = CFG80211_SIGNAL_TYPE_MBM;
+
+ wiphy->bands[NL80211_BAND_2GHZ] = &band_2ghz;
+ wiphy->bands[NL80211_BAND_5GHZ] = &band_5ghz;
+ wiphy->bands[NL80211_BAND_60GHZ] = NULL;
+
+ /* Don't worry about frequency regulations. */
+ wiphy->regulatory_flags = REGULATORY_WIPHY_SELF_MANAGED;
+ wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION);
+ set_wiphy_dev(wiphy, device);
+
+ priv = wiphy_priv(wiphy);
+ priv->netdev_is_up = false;
+ priv->being_deleted = false;
+ priv->is_connected = false;
+ priv->scan_request = NULL;
+ priv->netdev = netdev;
+ INIT_DELAYED_WORK(&priv->scan_result, virt_wifi_scan_result);
+ INIT_DELAYED_WORK(&priv->connect, virt_wifi_connect_complete);
+ INIT_DELAYED_WORK(&priv->disconnect, virt_wifi_disconnect_complete);
+ return wdev;
+}
+
+struct virt_wifi_netdev_priv {
+ struct net_device *lowerdev;
+ struct net_device *upperdev;
+ struct work_struct register_wiphy_work;
+};
+
+static netdev_tx_t virt_wifi_start_xmit(struct sk_buff *skb,
+ struct net_device *dev)
+{
+ struct virt_wifi_netdev_priv *priv = netdev_priv(dev);
+ struct virt_wifi_priv *w_priv = wiphy_priv(dev->ieee80211_ptr->wiphy);
+
+ if (!w_priv->is_connected)
+ return NET_XMIT_DROP;
+
+ skb->dev = priv->lowerdev;
+ return dev_queue_xmit(skb);
+}
+
+/* Called with rtnl lock held. */
+static int virt_wifi_net_device_open(struct net_device *dev)
+{
+ struct virt_wifi_priv *w_priv;
+
+ if (!dev->ieee80211_ptr)
+ return 0;
+ w_priv = wiphy_priv(dev->ieee80211_ptr->wiphy);
+
+ w_priv->netdev_is_up = true;
+ return 0;
+}
+
+/* Called with rtnl lock held. */
+static int virt_wifi_net_device_stop(struct net_device *dev)
+{
+ struct virt_wifi_priv *w_priv;
+ struct cfg80211_scan_info scan_info = { .aborted = true };
+
+ if (!dev->ieee80211_ptr)
+ return 0;
+ w_priv = wiphy_priv(dev->ieee80211_ptr->wiphy);
+ w_priv->netdev_is_up = false;
+
+ if (w_priv->scan_request) {
+ cfg80211_scan_done(w_priv->scan_request, &scan_info);
+ w_priv->scan_request = NULL;
+ }
+
+ return 0;
+}
+
+static const struct net_device_ops virt_wifi_ops = {
+ .ndo_start_xmit = virt_wifi_start_xmit,
+ .ndo_open = virt_wifi_net_device_open,
+ .ndo_stop = virt_wifi_net_device_stop,
+};
+
+static void free_netdev_and_wiphy(struct net_device *dev)
+{
+ struct virt_wifi_netdev_priv *priv = netdev_priv(dev);
+ struct virt_wifi_priv *w_priv;
+
+ flush_work(&priv->register_wiphy_work);
+ if (dev->ieee80211_ptr && !IS_ERR(dev->ieee80211_ptr)) {
+ w_priv = wiphy_priv(dev->ieee80211_ptr->wiphy);
+ w_priv->being_deleted = true;
+ flush_delayed_work(&w_priv->scan_result);
+ flush_delayed_work(&w_priv->connect);
+ flush_delayed_work(&w_priv->disconnect);
+
+ if (dev->ieee80211_ptr->wiphy->registered)
+ wiphy_unregister(dev->ieee80211_ptr->wiphy);
+ wiphy_free(dev->ieee80211_ptr->wiphy);
+ kfree(dev->ieee80211_ptr);
+ }
+ free_netdev(dev);
+}
+
+static void virt_wifi_setup(struct net_device *dev)
+{
+ ether_setup(dev);
+ dev->netdev_ops = &virt_wifi_ops;
+ dev->priv_destructor = free_netdev_and_wiphy;
+}
+
+/* Called under rcu_read_lock() from netif_receive_skb */
+static rx_handler_result_t virt_wifi_rx_handler(struct sk_buff **pskb)
+{
+ struct sk_buff *skb = *pskb;
+ struct virt_wifi_netdev_priv *priv =
+ rcu_dereference(skb->dev->rx_handler_data);
+ struct virt_wifi_priv *w_priv =
+ wiphy_priv(priv->upperdev->ieee80211_ptr->wiphy);
+
+ if (!w_priv->is_connected)
+ return RX_HANDLER_PASS;
+
+ /* GFP_ATOMIC because this is a packet interrupt handler. */
+ skb = skb_share_check(skb, GFP_ATOMIC);
+ if (!skb) {
+ dev_err(&priv->upperdev->dev, "can't skb_share_check\n");
+ return RX_HANDLER_CONSUMED;
+ }
+
+ *pskb = skb;
+ skb->dev = priv->upperdev;
+ skb->pkt_type = PACKET_HOST;
+ return RX_HANDLER_ANOTHER;
+}
+
+static void virt_wifi_register_wiphy(struct work_struct *work)
+{
+ struct virt_wifi_netdev_priv *priv =
+ container_of(work, struct virt_wifi_netdev_priv,
+ register_wiphy_work);
+ struct wireless_dev *wdev = priv->upperdev->ieee80211_ptr;
+ int err;
+
+ err = wiphy_register(wdev->wiphy);
+ if (err < 0) {
+ dev_err(&priv->upperdev->dev, "can't wiphy_register (%d)\n",
+ err);
+
+ /* Roll back the net_device, it's not going to do wifi. */
+ rtnl_lock();
+ err = rtnl_delete_link(priv->upperdev);
+ rtnl_unlock();
+
+ /* rtnl_delete_link should only throw errors if it's not a
+ * netlink device, but we know here it is already a virt_wifi
+ * device.
+ */
+ WARN_ONCE(err, "rtnl_delete_link failed on a virt_wifi device");
+ }
+}
+
+/* Called with rtnl lock held. */
+static int virt_wifi_newlink(struct net *src_net, struct net_device *dev,
+ struct nlattr *tb[], struct nlattr *data[],
+ struct netlink_ext_ack *extack)
+{
+ struct virt_wifi_netdev_priv *priv = netdev_priv(dev);
+ int err;
+
+ if (!tb[IFLA_LINK])
+ return -EINVAL;
+
+ INIT_WORK(&priv->register_wiphy_work, virt_wifi_register_wiphy);
+ priv->upperdev = dev;
+ priv->lowerdev = __dev_get_by_index(src_net,
+ nla_get_u32(tb[IFLA_LINK]));
+
+ if (!priv->lowerdev)
+ return -ENODEV;
+ if (!tb[IFLA_MTU])
+ dev->mtu = priv->lowerdev->mtu;
+ else if (dev->mtu > priv->lowerdev->mtu)
+ return -EINVAL;
+
+ err = netdev_rx_handler_register(priv->lowerdev, virt_wifi_rx_handler,
+ priv);
+ if (err) {
+ dev_err(&priv->lowerdev->dev,
+ "can't netdev_rx_handler_register: %d\n", err);
+ return err;
+ }
+
+ eth_hw_addr_inherit(dev, priv->lowerdev);
+ netif_stacked_transfer_operstate(priv->lowerdev, dev);
+
+ SET_NETDEV_DEV(dev, &priv->lowerdev->dev);
+ dev->ieee80211_ptr = virt_wireless_dev(&priv->lowerdev->dev, dev);
+
+ if (IS_ERR(dev->ieee80211_ptr)) {
+ dev_err(&priv->lowerdev->dev, "can't init wireless: %ld\n",
+ PTR_ERR(dev->ieee80211_ptr));
+ err = PTR_ERR(dev->ieee80211_ptr);
+ goto remove_handler;
+ }
+
+ err = register_netdevice(dev);
+ if (err) {
+ dev_err(&priv->lowerdev->dev, "can't register_netdevice: %d\n",
+ err);
+ goto free_wiphy;
+ }
+
+ err = netdev_upper_dev_link(priv->lowerdev, dev, extack);
+ if (err) {
+ dev_err(&priv->lowerdev->dev, "can't netdev_upper_dev_link: %d\n",
+ err);
+ goto unregister_netdev;
+ }
+
+ /* The newlink callback is invoked while holding the rtnl lock, but
+ * register_wiphy wants to claim the rtnl lock itself.
+ */
+ schedule_work(&priv->register_wiphy_work);
+
+ return 0;
+unregister_netdev:
+ unregister_netdevice(dev);
+free_wiphy: /* Safe whether or not netdev destructor runs. */
+ wiphy_free(dev->ieee80211_ptr->wiphy);
+ kfree(dev->ieee80211_ptr);
+ dev->ieee80211_ptr = NULL;
+remove_handler:
+ netdev_rx_handler_unregister(priv->lowerdev);
+
+ return err;
+}
+
+/* Called with rtnl lock held. */
+static void virt_wifi_dellink(struct net_device *dev,
+ struct list_head *head)
+{
+ struct virt_wifi_netdev_priv *priv = netdev_priv(dev);
+
+ netdev_rx_handler_unregister(priv->lowerdev);
+ netdev_upper_dev_unlink(priv->lowerdev, dev);
+
+ unregister_netdevice_queue(dev, head);
+
+ /* Deleting the wiphy is handled in the netdev destructor. */
+}
+
+static struct rtnl_link_ops virt_wifi_link_ops = {
+ .kind = "virt_wifi",
+ .setup = virt_wifi_setup,
+ .newlink = virt_wifi_newlink,
+ .dellink = virt_wifi_dellink,
+ .priv_size = sizeof(struct virt_wifi_netdev_priv),
+};
+
+static int __init virt_wifi_init_module(void)
+{
+ /* Guaranteed to be locallly-administered and not multicast. */
+ eth_random_addr(fake_router_bssid);
+ return rtnl_link_register(&virt_wifi_link_ops);
+}
+
+static void __exit virt_wifi_cleanup_module(void)
+{
+ rtnl_link_unregister(&virt_wifi_link_ops);
+}
+
+module_init(virt_wifi_init_module);
+module_exit(virt_wifi_cleanup_module);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Cody Schuffelen <[email protected]>");
+MODULE_DESCRIPTION("Driver for a wireless wrapper of ethernet devices");
+MODULE_ALIAS_RTNL_LINK("virt_wifi");
--
2.19.0.605.g01d371f741-goog
Hi Cody,
> drivers/net/wireless/Kconfig | 7 +
> drivers/net/wireless/Makefile | 2 +
> drivers/net/wireless/virt_wifi.c | 618 +++++++++++++++++++++++++++++++
> 3 files changed, 627 insertions(+)
> create mode 100644 drivers/net/wireless/virt_wifi.c
I did a quick check of your patch using checkpatch kernel tool,
here is a summary of its output:
$ ./scripts/checkpatch.pl --strict test.patch
...
total: 165 errors, 428 warnings, 9 checks, 634 lines checked
Most part of those complaints is about either whitespaces or code
idents. I am not sure whether this is a patch itself or email client.
So could you please take a look and run checkpatch on your side.
> +static void virt_wifi_scan_result(struct work_struct *work)
> +{
> + const union {
> + struct {
> + u8 tag;
> + u8 len;
> + u8 ssid[8];
> + } __packed parts;
> + u8 data[10];
> + } ssid = { .parts = {
> + .tag = WLAN_EID_SSID, .len = 8, .ssid = "VirtWifi" }
> + };
> + struct cfg80211_bss *informed_bss;
> + struct virt_wifi_priv *priv =
> + container_of(work, struct virt_wifi_priv,
> + scan_result.work);
> + struct wiphy *wiphy = priv_to_wiphy(priv);
> + struct cfg80211_inform_bss mock_inform_bss = {
> + .chan = &channel_5ghz,
> + .scan_width = NL80211_BSS_CHAN_WIDTH_20,
> + .signal = -60,
> + .boottime_ns = ktime_get_boot_ns(),
> + };
> + struct cfg80211_scan_info scan_info = {};
> +
> + informed_bss =
> + cfg80211_inform_bss_data(wiphy, &mock_inform_bss,
> + CFG80211_BSS_FTYPE_PRESP,
> + fake_router_bssid,
> + mock_inform_bss.boottime_ns,
> + WLAN_CAPABILITY_ESS, 0, ssid.data,
> + sizeof(ssid), GFP_KERNEL);
It is possible to simplify this part switching to cfg80211_inform_bss
function: this function wraps your scan data in into cfg80211_inform_bss
structure internally using some reasonable defaults, e.g. channel width.
Besides, signal strength for scan entries should be passed in mBm units,
so use DBM_TO_MBM macro. For instance, with your current code 'iw' tool
produces the following output:
$ sudo iw dev wlan0 scan
...
signal: 0.-60 dBm
...
> +static void virt_wifi_connect_complete(struct work_struct *work)
> +{
> + struct virt_wifi_priv *priv =
> + container_of(work, struct virt_wifi_priv, connect.work);
> + u8 *requested_bss = priv->connect_requested_bss;
> + bool has_addr = !is_zero_ether_addr(requested_bss);
> + bool right_addr = ether_addr_equal(requested_bss, fake_router_bssid);
> + u16 status = WLAN_STATUS_SUCCESS;
> +
> + rtnl_lock();
> + if (!priv->netdev_is_up || (has_addr && !right_addr))
> + status = WLAN_STATUS_UNSPECIFIED_FAILURE;
> + else
> + priv->is_connected = true;
> +
> + cfg80211_connect_result(priv->netdev, requested_bss, NULL, 0, NULL, 0,
> + status, GFP_KERNEL);
> + rtnl_unlock();
> +}
Carrier state for wireless device depends on its connection state.
E.g., carrier is set when wireless connection succeeds and cleared
when device disconnects. So use netif_carrier_on/netif_carrier_off
calls in connect/disconnect handlers to set correct carrier state.
IIUC the following locations look reasonable:
- netif_carrier_off on init
- netif_carrier_on in virt_wifi_connect_complete on success
- netif_carrier_off in virt_wifi_disconnect
> +static void virt_wifi_disconnect_complete(struct work_struct *work)
> +{
> + struct virt_wifi_priv *priv =
> + container_of(work, struct virt_wifi_priv, disconnect.work);
> +
> + cfg80211_disconnected(priv->netdev, priv->disconnect_reason, NULL, 0,
> + true, GFP_KERNEL);
> + priv->is_connected = false;
> +}
Why do you need delayed disconnect processing ? IIUC it can be dropped
and cfg80211_disconnected call can be moved to virt_wifi_disconnect.
> +
> +static int virt_wifi_get_station(struct wiphy *wiphy,
> + struct net_device *dev,
> + const u8 *mac,
> + struct station_info *sinfo)
> +{
> + wiphy_debug(wiphy, "get_station\n");
> +
> + if (!ether_addr_equal(mac, fake_router_bssid))
> + return -ENOENT;
> +
> + sinfo->filled = BIT(NL80211_STA_INFO_TX_PACKETS) |
> + BIT(NL80211_STA_INFO_TX_FAILED) | BIT(NL80211_STA_INFO_SIGNAL) |
> + BIT(NL80211_STA_INFO_TX_BITRATE);
Recently some of NL80211_STA_INFO_ attribute types has been modified to
use BIT_ULL macro. Could you please check commit 22d0d2fafca93ba1d92a
for details and modify your coded if needed.
> + sinfo->tx_packets = 1;
Only one packet, really ? Not sure if you plan to use the output of 'iw'
or any other tool. If yes, then it probably makes sense to use stats
from the original network link. Otherwise, your 'iw' output is
going to look like this:
$ iw dev wlan0 station dump
...
tx packets: 1
...
> + sinfo->tx_failed = 0;
...
> +static int virt_wifi_dump_station(struct wiphy *wiphy,
> + struct net_device *dev,
> + int idx,
> + u8 *mac,
> + struct station_info *sinfo)
> +{
> + wiphy_debug(wiphy, "dump_station\n");
> +
> + if (idx != 0)
> + return -ENOENT;
> +
> + ether_addr_copy(mac, fake_router_bssid);
> + return virt_wifi_get_station(wiphy, dev, fake_router_bssid, sinfo);
> +}
Callback dump_station should return AP data only when STA is connected.
Currently your driver returns fake AP data even when it is not
connected.
> +static const struct cfg80211_ops virt_wifi_cfg80211_ops = {
> + .scan = virt_wifi_scan,
> +
> + .connect = virt_wifi_connect,
> + .disconnect = virt_wifi_disconnect,
> +
> + .get_station = virt_wifi_get_station,
> + .dump_station = virt_wifi_dump_station,
> +};
Hey, this minimal cfg80211 implementation works fine with wpa_supplicant
and open AP config. By the way, if you plan to add more features, then
I would suggest to consider the following cfg80211 callbacks:
- change_station, get_channel
to provide more info in connected state, e.g. compare the output
of the following commands between your virtual interface and
actual wireless interface:
$ iw dev wlan0 link
$ iw dev wlan0 info
- stubs for add_key, del_key to enable encrypted AP simulation
Regards,
Sergey
On Fri, Oct 5, 2018 at 7:33 AM, Sergey Matyukevich
<[email protected]> wrote:
> Hi Cody,
>
>> drivers/net/wireless/Kconfig | 7 +
>> drivers/net/wireless/Makefile | 2 +
>> drivers/net/wireless/virt_wifi.c | 618 +++++++++++++++++++++++++++++++
>> 3 files changed, 627 insertions(+)
>> create mode 100644 drivers/net/wireless/virt_wifi.c
>
> I did a quick check of your patch using checkpatch kernel tool,
> here is a summary of its output:
>
> $ ./scripts/checkpatch.pl --strict test.patch
> ...
> total: 165 errors, 428 warnings, 9 checks, 634 lines checked
>
> Most part of those complaints is about either whitespaces or code
> idents. I am not sure whether this is a patch itself or email client.
> So could you please take a look and run checkpatch on your side.
>
Yeah, it could be his email client, weird though because if I pull the
patch from the kernel.org archive's mbox though, I don't get any
errors except the MAINTAINERS file thing:
wget https://lore.kernel.org/lkml/[email protected]/raw
-O /tmp/tmp.patch
./scripts/checkpatch.pl --strict /tmp/tmp.patch
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#167:
new file mode 100644
total: 0 errors, 1 warnings, 0 checks, 634 lines checked
- Joel
On Fri, Oct 5, 2018 at 2:05 PM, Joel Fernandes <[email protected]> wrote:
> On Fri, Oct 5, 2018 at 7:33 AM, Sergey Matyukevich
> <[email protected]> wrote:
>> Hi Cody,
>>
>>> drivers/net/wireless/Kconfig | 7 +
>>> drivers/net/wireless/Makefile | 2 +
>>> drivers/net/wireless/virt_wifi.c | 618 +++++++++++++++++++++++++++++++
>>> 3 files changed, 627 insertions(+)
>>> create mode 100644 drivers/net/wireless/virt_wifi.c
>>
>> I did a quick check of your patch using checkpatch kernel tool,
>> here is a summary of its output:
>>
>> $ ./scripts/checkpatch.pl --strict test.patch
>> ...
>> total: 165 errors, 428 warnings, 9 checks, 634 lines checked
>>
>> Most part of those complaints is about either whitespaces or code
>> idents. I am not sure whether this is a patch itself or email client.
>> So could you please take a look and run checkpatch on your side.
>>
>
> Yeah, it could be his email client, weird though because if I pull the
> patch from the kernel.org archive's mbox though, I don't get any
> errors except the MAINTAINERS file thing:
>
> wget https://lore.kernel.org/lkml/[email protected]/raw
> -O /tmp/tmp.patch
> ./scripts/checkpatch.pl --strict /tmp/tmp.patch
>
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #167:
> new file mode 100644
>
> total: 0 errors, 1 warnings, 0 checks, 634 lines checked
>
FWIW, the X-Mailer on the patch is: git-send-email 2.19.0.605.g01d371f741-goog
So I am guessing this is some issue with the way the patch was
generated, or something else. Cody, care to share the steps you used
to generate and send the patch?
- Joel
Cody Schuffelen <[email protected]> writes:
> This device takes over an existing network device and produces a
> new one that appears like a wireless connection, returning enough canned
> responses to nl80211 to satisfy a standard connection manager. If
> necessary, it can also be set up one step removed from an existing
> network device, such as through a vlan/80211Q or macvlan connection to
> not disrupt the existing network interface.
>
> To use it to wrap a bare ethernet connection:
>
> ip link add link eth0 name wlan0 type virt_wifi
>
> You may have to rename or otherwise hide the eth0 from your connection
> manager, as the original network link will become unusuable and only
> the wireless wrapper will be functional. This can also be combined with
> vlan or macvlan links on top of eth0 to share the network between
> distinct links, but that requires support outside the machine for
> accepting vlan-tagged packets or packets from multiple MAC addresses.
>
> This is being used for Google's Remote Android Virtual Device project,
> which runs Android devices in virtual machines. The standard network
> interfaces provided inside the virtual machines are all ethernet.
> However, Android is not interested in ethernet devices and would rather
> connect to a wireless interface. This patch allows the virtual machine
> guest to treat one of its network connections as wireless rather than
> ethernet, satisfying Android's network connection requirements.
>
> We believe this is a generally useful driver for simulating wireless
> network connections in other environments where a wireless connection is
> desired by some userspace process but is not available.
>
> This is distinct from other testing efforts such as mac80211_hwsim by
> being a cfg80211 device instead of mac80211 device, allowing straight
> pass-through on the data plane instead of forcing packaging of ethernet
> data into mac80211 frames.
>
> Signed-off-by: A. Cody Schuffelen <[email protected]>
> Acked-by: Alistair Strachan <[email protected]>
> Acked-by: Greg Hartman <[email protected]>
> ---
> First version: https://lkml.org/lkml/2018/7/27/947
> First review: https://lore.kernel.org/lkml/[email protected]/
> Second version: https://lore.kernel.org/lkml/[email protected]/
> Second review: https://www.lkml.org/lkml/2018/9/27/228
> Second review: https://www.lkml.org/lkml/2018/9/27/229
> Second review: https://www.lkml.org/lkml/2018/9/27/669
>
> Thanks for all the comments on v2! I believe I've addressed all of them
> here. I've also added changes to react better to the netdev going down,
> canceling ongoing scans and rejecting wifi network connection requests.
>
> I wasn't completely clear on whether I should change the title (net-next
> to mac80211-next) so I left it as is for v3 to try to keep the patchwork
> series together.
I already said in v2 that you should not mark this for net-next as this
goes to mac80211-next (if it gets accepted), now you are just
unnecessarily confusing people.
--
Kalle Valo
Cody Schuffelen <[email protected]> writes:
> Thanks for the comments! I will work on incorporating the suggested
> changes.
Few important remarks:
1. Do not EVER submit HTML mails, mailing lists will automatically
reject them. So your mail didn't reach the lists and I only got it
because I was in the Cc field.
2. Do not top post, it's really annoying:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#do_not_top_post_and_edit_your_quotes
3. Reply to the review comments you get, even if you agree with them. By
not replying you give an impression that you are ignoring the review
comments.
--
Kalle Valo
> > Hi Cody,
> >
> >> drivers/net/wireless/Kconfig | 7 +
> >> drivers/net/wireless/Makefile | 2 +
> >> drivers/net/wireless/virt_wifi.c | 618 +++++++++++++++++++++++++++++++
> >> 3 files changed, 627 insertions(+)
> >> create mode 100644 drivers/net/wireless/virt_wifi.c
> >
> > I did a quick check of your patch using checkpatch kernel tool,
> > here is a summary of its output:
> >
> > $ ./scripts/checkpatch.pl --strict test.patch
> > ...
> > total: 165 errors, 428 warnings, 9 checks, 634 lines checked
> >
> > Most part of those complaints is about either whitespaces or code
> > idents. I am not sure whether this is a patch itself or email client.
> > So could you please take a look and run checkpatch on your side.
> >
>
> Yeah, it could be his email client, weird though because if I pull the
> patch from the kernel.org archive's mbox though, I don't get any
> errors except the MAINTAINERS file thing:
>
> wget https://lore.kernel.org/lkml/[email protected]/raw
> -O /tmp/tmp.patch
> ./scripts/checkpatch.pl --strict /tmp/tmp.patch
>
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #167:
> new file mode 100644
>
> total: 0 errors, 1 warnings, 0 checks, 634 lines checked
Hi Cody and Joel,
Please ignore my comment regarding the whitespace issues in the patch.
I don't see those issues when downloading the patch via gmail server.
So the issue was on my side.
Regards,
Sergey
On Thu, 2018-10-04 at 12:59 -0700, Cody Schuffelen wrote:
>
> I wasn't completely clear on whether I should change the title (net-next
> to mac80211-next) so I left it as is for v3 to try to keep the patchwork
> series together.
You can/should change it - patchwork doesn't really track this at all
anyway.
> The driver is also now a bool instead of a tristate to use __ro_after_init.
Hmm? Why would that be required? __ro_after_init works fine in modules?
> +static struct ieee80211_rate bitrates_2ghz[] = {
> + { .bitrate = 10 },
> + { .bitrate = 20 },
> + { .bitrate = 55 },
> + { .bitrate = 60 },
> + { .bitrate = 110 },
> + { .bitrate = 120 },
> + { .bitrate = 240 },
> +};
Come to think of it, the typical order here would be 1,2,5.5,11,6,12,24
(6<->11), due to the ordering in the probe request frame I guess.
I'm not sure it matters though.
> +static struct ieee80211_supported_band band_2ghz = {
These can be const, I think?
> +/** Assigned at module init. Guaranteed locally-administered and unicast. */
I think you should avoid ** - it's the kernel-doc marker.
> +static u8 fake_router_bssid[ETH_ALEN] __ro_after_init = {};
If this is the reason for not allowing it to be a module then you don't
need to disallow the module case.
> +static int virt_wifi_scan(struct wiphy *wiphy,
> + struct cfg80211_scan_request *request)
> +{
> + struct virt_wifi_priv *priv = wiphy_priv(wiphy);
> +
> + wiphy_debug(wiphy, "scan\n");
> +
> + if (priv->scan_request || priv->being_deleted)
> + return -EBUSY;
> +
> + priv->scan_request = request;
> + schedule_delayed_work(&priv->scan_result, HZ * 2);
> +
> + return 0;
> +}
> +
> +static void virt_wifi_scan_result(struct work_struct *work)
> +{
> + const union {
> + struct {
> + u8 tag;
> + u8 len;
> + u8 ssid[8];
> + } __packed parts;
> + u8 data[10];
> + } ssid = { .parts = {
> + .tag = WLAN_EID_SSID, .len = 8, .ssid = "VirtWifi" }
> + };
Not sure I see much value in the union, but I don't think it matters
much.
(You could just cast below - (void *)&ssid, sizeof(ssid))
> + rtnl_lock();
> + if (priv->scan_request) {
> + cfg80211_scan_done(priv->scan_request, &scan_info);
> + priv->scan_request = NULL;
> + }
> + rtnl_unlock();
Do you need the rtnl for the priv->scan_request locking?
> +static int virt_wifi_get_station(struct wiphy *wiphy,
> + struct net_device *dev,
> + const u8 *mac,
> + struct station_info *sinfo)
> +{
> [...]
> + sinfo->tx_packets = 1;
> + sinfo->tx_failed = 0;
> + sinfo->signal = -60;
I think Sergey pointed out the -60 elsewhere - need to check here too,
and maybe set in some place that you actually report dBm/mBm.
Also, I think you should only report something here if actually
connected - Sergey pointed this out on the dump station but it applies
here too.
> +static void free_netdev_and_wiphy(struct net_device *dev)
> +{
> + struct virt_wifi_netdev_priv *priv = netdev_priv(dev);
> + struct virt_wifi_priv *w_priv;
> +
> + flush_work(&priv->register_wiphy_work);
> + if (dev->ieee80211_ptr && !IS_ERR(dev->ieee80211_ptr)) {
> + w_priv = wiphy_priv(dev->ieee80211_ptr->wiphy);
> + w_priv->being_deleted = true;
> + flush_delayed_work(&w_priv->scan_result);
> + flush_delayed_work(&w_priv->connect);
> + flush_delayed_work(&w_priv->disconnect);
this is called from
> +static void virt_wifi_setup(struct net_device *dev)
> +{
> + ether_setup(dev);
> + dev->netdev_ops = &virt_wifi_ops;
> + dev->priv_destructor = free_netdev_and_wiphy;
> +}
the destructor, but I believe the destructor is invoked with the RTNL
held. As such, this will deadlock (and lockdep should complain - at
least in current kernel versions) when it's actually running when you
flush it, since you flush something that's (potentially) waiting to
acquire the RTNL, but you are holding the RTNL here and so neither side
can make progress.
> + /* The newlink callback is invoked while holding the rtnl lock, but
> + * register_wiphy wants to claim the rtnl lock itself.
> + */
> + schedule_work(&priv->register_wiphy_work);
Maybe we should fix/change that?
johannes
> > + informed_bss =
> > + cfg80211_inform_bss_data(wiphy, &mock_inform_bss,
> > + CFG80211_BSS_FTYPE_PRESP,
> > + fake_router_bssid,
> > + mock_inform_bss.boottime_ns,
> > + WLAN_CAPABILITY_ESS, 0, ssid.data,
> > + sizeof(ssid), GFP_KERNEL);
>
> It is possible to simplify this part switching to cfg80211_inform_bss
> function: this function wraps your scan data in into cfg80211_inform_bss
> structure internally using some reasonable defaults, e.g. channel width.
>
> Besides, signal strength for scan entries should be passed in mBm units,
> so use DBM_TO_MBM macro. For instance, with your current code 'iw' tool
> produces the following output:
> $ sudo iw dev wlan0 scan
> ...
> signal: 0.-60 dBm
> ...
Good catch, fixed.
> > +static void virt_wifi_connect_complete(struct work_struct *work)
> > +{
> > + struct virt_wifi_priv *priv =
> > + container_of(work, struct virt_wifi_priv, connect.work);
> > + u8 *requested_bss = priv->connect_requested_bss;
> > + bool has_addr = !is_zero_ether_addr(requested_bss);
> > + bool right_addr = ether_addr_equal(requested_bss, fake_router_bssid);
> > + u16 status = WLAN_STATUS_SUCCESS;
> > +
> > + rtnl_lock();
> > + if (!priv->netdev_is_up || (has_addr && !right_addr))
> > + status = WLAN_STATUS_UNSPECIFIED_FAILURE;
> > + else
> > + priv->is_connected = true;
> > +
> > + cfg80211_connect_result(priv->netdev, requested_bss, NULL, 0, NULL, 0,
> > + status, GFP_KERNEL);
> > + rtnl_unlock();
> > +}
>
> Carrier state for wireless device depends on its connection state.
> E.g., carrier is set when wireless connection succeeds and cleared
> when device disconnects. So use netif_carrier_on/netif_carrier_off
> calls in connect/disconnect handlers to set correct carrier state.
> IIUC the following locations look reasonable:
> - netif_carrier_off on init
> - netif_carrier_on in virt_wifi_connect_complete on success
> - netif_carrier_off in virt_wifi_disconnect
Thanks, added.
> > +static void virt_wifi_disconnect_complete(struct work_struct *work)
> > +{
> > + struct virt_wifi_priv *priv =
> > + container_of(work, struct virt_wifi_priv, disconnect.work);
> > +
> > + cfg80211_disconnected(priv->netdev, priv->disconnect_reason, NULL, 0,
> > + true, GFP_KERNEL);
> > + priv->is_connected = false;
> > +}
>
> Why do you need delayed disconnect processing ? IIUC it can be dropped
> and cfg80211_disconnected call can be moved to virt_wifi_disconnect.
Done.
> > +
> > +static int virt_wifi_get_station(struct wiphy *wiphy,
> > + struct net_device *dev,
> > + const u8 *mac,
> > + struct station_info *sinfo)
> > +{
> > + wiphy_debug(wiphy, "get_station\n");
> > +
> > + if (!ether_addr_equal(mac, fake_router_bssid))
> > + return -ENOENT;
> > +
> > + sinfo->filled = BIT(NL80211_STA_INFO_TX_PACKETS) |
> > + BIT(NL80211_STA_INFO_TX_FAILED) | BIT(NL80211_STA_INFO_SIGNAL) |
> > + BIT(NL80211_STA_INFO_TX_BITRATE);
>
> Recently some of NL80211_STA_INFO_ attribute types has been modified to
> use BIT_ULL macro. Could you please check commit 22d0d2fafca93ba1d92a
> for details and modify your coded if needed.
Thanks for the the reference, updated to use BIT_ULL with the station commands.
> > + sinfo->tx_packets = 1;
>
> Only one packet, really ? Not sure if you plan to use the output of 'iw'
> or any other tool. If yes, then it probably makes sense to use stats
> from the original network link. Otherwise, your 'iw' output is
> going to look like this:
>
> $ iw dev wlan0 station dump
> ...
> tx packets: 1
> ...
>
> > + sinfo->tx_failed = 0;
>
> ...
Added bookkeeping to the net_device packet forwarded to track how many
packets were sent, and how many failed being sent due to no
connection.
> > +static int virt_wifi_dump_station(struct wiphy *wiphy,
> > + struct net_device *dev,
> > + int idx,
> > + u8 *mac,
> > + struct station_info *sinfo)
> > +{
> > + wiphy_debug(wiphy, "dump_station\n");
> > +
> > + if (idx != 0)
> > + return -ENOENT;
> > +
> > + ether_addr_copy(mac, fake_router_bssid);
> > + return virt_wifi_get_station(wiphy, dev, fake_router_bssid, sinfo);
> > +}
>
> Callback dump_station should return AP data only when STA is connected.
> Currently your driver returns fake AP data even when it is not
> connected.
Thanks, fixed.
> > +static const struct cfg80211_ops virt_wifi_cfg80211_ops = {
> > + .scan = virt_wifi_scan,
> > +
> > + .connect = virt_wifi_connect,
> > + .disconnect = virt_wifi_disconnect,
> > +
> > + .get_station = virt_wifi_get_station,
> > + .dump_station = virt_wifi_dump_station,
> > +};
>
> Hey, this minimal cfg80211 implementation works fine with wpa_supplicant
> and open AP config. By the way, if you plan to add more features, then
> I would suggest to consider the following cfg80211 callbacks:
> - change_station, get_channel
> to provide more info in connected state, e.g. compare the output
> of the following commands between your virtual interface and
> actual wireless interface:
> $ iw dev wlan0 link
> $ iw dev wlan0 info
>
> - stubs for add_key, del_key to enable encrypted AP simulation
Thanks for testing it out!
I've uploaded the new version here: https://lkml.org/lkml/2018/11/21/251
> 1. Do not EVER submit HTML mails, mailing lists will automatically
> reject them. So your mail didn't reach the lists and I only got it
> because I was in the Cc field.
I apologize, my mistake.
> 2. Do not top post, it's really annoying:
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#do_not_top_post_and_edit_your_quotes
Thanks for the reference, I will keep that in mind.
> 3. Reply to the review comments you get, even if you agree with them. By
> not replying you give an impression that you are ignoring the review
> comments.
Thanks for calling this out, will do.
On Tue, Oct 9, 2018 at 1:25 AM Johannes Berg <[email protected]> wrote:
>
> On Thu, 2018-10-04 at 12:59 -0700, Cody Schuffelen wrote:
> >
> > I wasn't completely clear on whether I should change the title (net-next
> > to mac80211-next) so I left it as is for v3 to try to keep the patchwork
> > series together.
>
> You can/should change it - patchwork doesn't really track this at all
> anyway.
Got it, thanks.
>
> > The driver is also now a bool instead of a tristate to use __ro_after_init.
>
> Hmm? Why would that be required? __ro_after_init works fine in modules?
My mistake, you're right. Tested and this does work with a module.
> > +static struct ieee80211_rate bitrates_2ghz[] = {
> > + { .bitrate = 10 },
> > + { .bitrate = 20 },
> > + { .bitrate = 55 },
> > + { .bitrate = 60 },
> > + { .bitrate = 110 },
> > + { .bitrate = 120 },
> > + { .bitrate = 240 },
> > +};
>
> Come to think of it, the typical order here would be 1,2,5.5,11,6,12,24
> (6<->11), due to the ordering in the probe request frame I guess.
>
> I'm not sure it matters though.
Thanks, changed.
>
> > +static struct ieee80211_supported_band band_2ghz = {
>
> These can be const, I think?
Unfortunately, the arrays they're assigned to in the wiphy is non-const:
https://github.com/torvalds/linux/blob/master/include/net/cfg80211.h#L4055
https://github.com/torvalds/linux/blob/master/include/net/cfg80211.h#L346
struct ieee80211_supported_band *bands[IEEE80211_NUM_BANDS];
> > +/** Assigned at module init. Guaranteed locally-administered and unicast. */
>
> I think you should avoid ** - it's the kernel-doc marker.
Good catch. Sorry about that, you pointed it out on an earlier version
as well on a different line.
>
> > +static u8 fake_router_bssid[ETH_ALEN] __ro_after_init = {};
>
> If this is the reason for not allowing it to be a module then you don't
> need to disallow the module case.
Good point, fixed.
> > +
> > +static void virt_wifi_scan_result(struct work_struct *work)
> > +{
> > + const union {
> > + struct {
> > + u8 tag;
> > + u8 len;
> > + u8 ssid[8];
> > + } __packed parts;
> > + u8 data[10];
> > + } ssid = { .parts = {
> > + .tag = WLAN_EID_SSID, .len = 8, .ssid = "VirtWifi" }
> > + };
>
> Not sure I see much value in the union, but I don't think it matters
> much.
> (You could just cast below - (void *)&ssid, sizeof(ssid))
Good point, done.
>
> > + rtnl_lock();
> > + if (priv->scan_request) {
> > + cfg80211_scan_done(priv->scan_request, &scan_info);
> > + priv->scan_request = NULL;
> > + }
> > + rtnl_unlock();
>
> Do you need the rtnl for the priv->scan_request locking?
I've redone the structure a bit to clean this up, and don't use the
lock here anymore.
> > +static int virt_wifi_get_station(struct wiphy *wiphy,
> > + struct net_device *dev,
> > + const u8 *mac,
> > + struct station_info *sinfo)
> > +{
> > [...]
> > + sinfo->tx_packets = 1;
> > + sinfo->tx_failed = 0;
> > + sinfo->signal = -60;
>
> I think Sergey pointed out the -60 elsewhere - need to check here too,
> and maybe set in some place that you actually report dBm/mBm.
Added a comment here, it looks like even with CFG80211_SIGNAL_TYPE_MBM
this should be in dBm.
https://github.com/torvalds/linux/blob/master/include/net/cfg80211.h#L1264
> Also, I think you should only report something here if actually
> connected - Sergey pointed this out on the dump station but it applies
> here too.
Thanks, get_station and dump_station now both check if the device is connected.
> > +static void free_netdev_and_wiphy(struct net_device *dev)
> > +{
> > + struct virt_wifi_netdev_priv *priv = netdev_priv(dev);
> > + struct virt_wifi_priv *w_priv;
> > +
> > + flush_work(&priv->register_wiphy_work);
> > + if (dev->ieee80211_ptr && !IS_ERR(dev->ieee80211_ptr)) {
> > + w_priv = wiphy_priv(dev->ieee80211_ptr->wiphy);
> > + w_priv->being_deleted = true;
> > + flush_delayed_work(&w_priv->scan_result);
> > + flush_delayed_work(&w_priv->connect);
> > + flush_delayed_work(&w_priv->disconnect);
>
> this is called from
>
> > +static void virt_wifi_setup(struct net_device *dev)
> > +{
> > + ether_setup(dev);
> > + dev->netdev_ops = &virt_wifi_ops;
> > + dev->priv_destructor = free_netdev_and_wiphy;
> > +}
>
> the destructor, but I believe the destructor is invoked with the RTNL
> held. As such, this will deadlock (and lockdep should complain - at
> least in current kernel versions) when it's actually running when you
> flush it, since you flush something that's (potentially) waiting to
> acquire the RTNL, but you are holding the RTNL here and so neither side
> can make progress.
Good catch, this was not a good strategy. I think it was stable for me
only because RTNL is technically not held when the destructor is
invoked by netdev_run_todo, though I see now that's only because
netdev_run_todo is a special case with respect to the RTNL mutex so
interacting with it there seems like a bad idea.
https://github.com/torvalds/linux/blob/master/net/core/dev.c#L8735
I've changed the asynchronous strategy: at a high level, it now sets
the device in a "down"/"being deleted" state to block any further
asynchronous requests, cancels pending delayed work operations, and
cleans up dangling callbacks the delayed work operations had committed
to doing. More functions are also now annotated with which locks
should already be held and which will be acquired and released. This
relies heavily on cancel_delayed_work_sync. My assumption here is that
cancel_delayed_work_sync puts the delayed work operations in a
definite "not running" state, and uses the return value to determine
if it needs to clean up any callbacks.
> > + /* The newlink callback is invoked while holding the rtnl lock, but
> > + * register_wiphy wants to claim the rtnl lock itself.
> > + */
> > + schedule_work(&priv->register_wiphy_work);
>
> Maybe we should fix/change that?
Worked around it for this implementation by sharing one wiphy across
all net_device instances. The module init and destroy now acquire the
rtnl mutex to init/destroy the wiphy as well as registering the rtnl
device type. The netdev setup is now more sane as the wiphy setup is
done ahead of time. The only downside is that multiple wifi devices
cannot scan simultaneously, so no great loss.
I've uploaded the new version here: https://lkml.org/lkml/2018/11/21/251
> I already said in v2 that you should not mark this for net-next as this
> goes to mac80211-next (if it gets accepted), now you are just
> unnecessarily confusing people.
My fault, I misunderstood. The next version will be named correctly.
The new version is uploaded here: https://lkml.org/lkml/2018/11/21/251