wireless-drivers: rtnetlink wifi simulation device
Subsystems (3)
PATCH v3 1/1 wireless-drivers: rtnetlink wifi simulation device
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.
---
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/20180926194324.71290-1-schuffelen-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org/
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
drivers/net/wireless/Kconfig +7 -0
@@ -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
drivers/net/wireless/Makefile +2 -0
@@ -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
drivers/net/wireless/virt_wifi.c new file +618 -0
618 lines not on this page · raw message
Signature · 1 line
2.19.0.605.g01d371f741-goog
Hi Cody,
Quoted 5 lines
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.
Quoted 22 lines
drivers/net/wireless/virt_wifi.c new file +22
-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 (),
drivers/net/wireless/virt_wifi.c line 192
Quoted 10 lines
drivers/net/wireless/virt_wifi.c new file +10
-0+ }; +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 ...
Quoted 9 lines
drivers/net/wireless/virt_wifi.c new file +9
-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 ; +
drivers/net/wireless/virt_wifi.c line 243
Quoted 10 lines
drivers/net/wireless/virt_wifi.c new file +10
-0+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
drivers/net/wireless/virt_wifi.c line 270
Quoted 9 lines
drivers/net/wireless/virt_wifi.c new file +9
-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; +}
Why do you need delayed disconnect processing ? IIUC it can be dropped and cfg80211_disconnected call can be moved to virt_wifi_disconnect.
Quoted 4 lines
drivers/net/wireless/virt_wifi.c new file +4
-0+ +static int virt_wifi_get_station(struct wiphy *wiphy, + struct net_device *dev, + const u8 *mac,
drivers/net/wireless/virt_wifi.c line 284
Quoted 10 lines
drivers/net/wireless/virt_wifi.c new file +10
-0+ 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.
Quoted 1 line
+
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 ...
Quoted 1 line
+
sinfo->tx_failed= 0;
...
Quoted 4 lines
drivers/net/wireless/virt_wifi.c new file +4
-0+static int virt_wifi_dump_station(struct wiphy *wiphy, + struct net_device *dev, + int idx, + u8 *mac,
drivers/net/wireless/virt_wifi.c line 307
Quoted 10 lines
drivers/net/wireless/virt_wifi.c new file +10
-0+ 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.
drivers/net/wireless/virt_wifi.c line 317
Quoted 9 lines
drivers/net/wireless/virt_wifi.c new file +9
-0+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
Quoted 11 lines · Sergey Matyukevich
Hi Cody,
Quoted 5 lines
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.cI did a quick check of your patch using checkpatch kernel tool, here is a summary of its output:
Quoted 8 lines
$ ./scripts/checkpatch.pl --strict test.patch ... total: 165 errors, 428 warnings, 9 checks, 634 lines checkedMost 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
Quoted 30 lines · Joel Fernandes
Quoted 11 lines · Sergey Matyukevich
Hi Cody,
Quoted 5 lines
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.cI did a quick check of your patch using checkpatch kernel tool, here is a summary of its output:
Quoted 8 lines
$ ./scripts/checkpatch.pl --strict test.patch ... total: 165 errors, 428 warnings, 9 checks, 634 lines checkedMost 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
MAINTAINERSfile thing:wget https://lore.kernel.org/lkml/20181004195906.201895-1-schuffelen-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org/raw
-O/tmp/tmp.patch./scripts/checkpatch.pl--strict/tmp/tmp.patch
Quoted 6 lines
WARNING: added, moved or deleted file(s), does
MAINTAINERSneed updating? #167:
new file mode 100644total: 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
A message that is not in the archive (sent off-list or not yet received): CAAELVAGAZ-UM77P=JqzHjUGja9oJcN5y9+YvxZWKBmOXAu=zNQ@mail.gmail.com
Quoted 2 lines · Cody Schuffelen
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:
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.
Signature · 1 line
Kalle Valo
Quoted 3 lines
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.
Quoted 3 lines
2. Do not top post, it's really annoying:
Thanks for the reference, I will keep that in mind.
Quoted 3 lines
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.
Quoted 28 lines
Quoted 11 lines
Hi Cody,
Quoted 5 lines
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.cI did a quick check of your patch using checkpatch kernel tool, here is a summary of its output:
Quoted 8 lines
$ ./scripts/checkpatch.pl --strict test.patch ... total: 165 errors, 428 warnings, 9 checks, 634 lines checkedMost 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
MAINTAINERSfile thing:wget https://lore.kernel.org/lkml/[email protected]/raw
-O/tmp/tmp.patch./scripts/checkpatch.pl--strict/tmp/tmp.patch
Quoted 5 lines
WARNING: added, moved or deleted file(s), does
MAINTAINERSneed updating? #167:
new file mode 100644total: 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
Quoted 12 lines
drivers/net/wireless/virt_wifi.c line 192
Quoted 7 lines
drivers/net/wireless/virt_wifi.c new file +7
-0+ 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 intocfg80211_inform_bssstructure internally using some reasonable defaults, e.g. channel width.
Quoted 7 lines
Besides, signal strength for scan entries should be passed in mBm units, so use
DBM_TO_MBMmacro. For instance, with your current code 'iw' tool produces the following output:$ sudo iw dev wlan0 scan ... signal: 0.-60 dBm ...
Good catch, fixed.
Quoted 20 lines
Quoted 9 lines
drivers/net/wireless/virt_wifi.c new file +9
-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 ; +drivers/net/wireless/virt_wifi.c line 243
Quoted 10 lines
drivers/net/wireless/virt_wifi.c new file +10
-0+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 (); +}
Quoted 8 lines
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_offon init
-netif_carrier_oninvirt_wifi_connect_completeon success
-netif_carrier_offinvirt_wifi_disconnect
Thanks, added.
Quoted 4 lines
Quoted 4 lines
drivers/net/wireless/virt_wifi.c new file +4
-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);
Quoted 8 lines
drivers/net/wireless/virt_wifi.c line 270
Quoted 5 lines
drivers/net/wireless/virt_wifi.c new file +5
-0+ +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_disconnectedcall can be moved tovirt_wifi_disconnect.
Done.
Quoted 10 lines
Quoted 10 lines
drivers/net/wireless/virt_wifi.c new file +10
-0+ +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 ;
Quoted 8 lines
drivers/net/wireless/virt_wifi.c line 284
Quoted 4 lines
drivers/net/wireless/virt_wifi.c new file +4
-0+ + 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 useBIT_ULLmacro. Could you please check commit22d0d2fafca93ba1d92afor details and modify your coded if needed.
Thanks for the the reference, updated to use BIT_ULL with the station commands.
Quoted 7 lines
Quoted 1 line
+
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:
Quoted 8 lines
$ iw dev wlan0 station dump ... tx packets: 1 ...Quoted 1 line
+
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.
Quoted 10 lines
Quoted 10 lines
drivers/net/wireless/virt_wifi.c new file +10
-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 ;
Quoted 8 lines
drivers/net/wireless/virt_wifi.c line 307
Quoted 4 lines
drivers/net/wireless/virt_wifi.c new file +4
-0+ +ether_addr_copy (mac, fake_router_bssid); +return virt_wifi_get_station (wiphy, dev, fake_router_bssid, sinfo); +}Callback
dump_stationshould return AP data only when STA is connected. Currently your driver returns fake AP data even when it is not connected.
Thanks, fixed.
Quoted 17 lines
drivers/net/wireless/virt_wifi.c line 317
Quoted 9 lines
drivers/net/wireless/virt_wifi.c new file +9
-0+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_supplicantand 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:
Quoted 4 lines
$ iw dev wlan0 link $ iw dev wlan0 info- stubs for
add_key,del_keyto enable encrypted AP simulation
Thanks for testing it out!
I've uploaded the new version here: https://lkml.org/lkml/2018/11/21/251
Quoted 46 lines · Cody Schuffelen
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_wifiYou 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_hwsimby 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 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
Quoted 7 lines
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.
Signature · 1 line
Kalle Valo
Quoted 3 lines
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
Johannes Berg
maintainerQuoted 4 lines · Cody Schuffelen
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.
Quoted 1 line
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?
drivers/net/wireless/virt_wifi.c line 45
Quoted 9 lines
drivers/net/wireless/virt_wifi.c new file +9
-0+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.
Quoted 1 line
+static struct
ieee80211_supported_bandband_2ghz= {
These can be const, I think?
Quoted 1 line
+/** Assigned at module init. Guaranteed locally-administered and unicast. */
I think you should avoid ** - it's the kernel-doc marker.
Quoted 1 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.
Quoted 18 lines
drivers/net/wireless/virt_wifi.c new file +18
-0+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) +{
drivers/net/wireless/virt_wifi.c line 172
Quoted 10 lines
drivers/net/wireless/virt_wifi.c new file +10
-0+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))
drivers/net/wireless/virt_wifi.c line 200
Quoted 6 lines
drivers/net/wireless/virt_wifi.c new file +6
-0+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?
drivers/net/wireless/virt_wifi.c line 287
Quoted 9 lines
drivers/net/wireless/virt_wifi.c new file +8
-0+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.
drivers/net/wireless/virt_wifi.c line 434
Quoted 12 lines
drivers/net/wireless/virt_wifi.c new file +12
-0+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
drivers/net/wireless/virt_wifi.c line 449
Quoted 6 lines
drivers/net/wireless/virt_wifi.c new file +6
-0+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.
drivers/net/wireless/virt_wifi.c line 563
Quoted 4 lines
drivers/net/wireless/virt_wifi.c new file +4
-0+/* 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
Quoted 9 lines · Johannes Berg
Quoted 4 lines · Cody Schuffelen
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.
Quoted 4 lines
Quoted 2 lines
The driver is also now a bool instead of a tristate to use
__ro_after_init.Hmm? Why would that be required?
__ro_after_initworks fine in modules?
My mistake, you're right. Tested and this does work with a module.
Quoted 13 lines
drivers/net/wireless/virt_wifi.c line 45
Quoted 9 lines
drivers/net/wireless/virt_wifi.c new file +9
-0+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.
Quoted 1 line
I'm not sure it matters though.
Thanks, changed.
Quoted 4 lines
Quoted 2 lines
+static struct
ieee80211_supported_bandband_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];
Quoted 3 lines
Quoted 1 line
+/** 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.
Quoted 5 lines
Quoted 2 lines
+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.
Quoted 9 lines
Quoted 9 lines
drivers/net/wireless/virt_wifi.c new file +9
-0+ +static void virt_wifi_scan_result (struct work_struct *work) +{ + const union { + struct { + u8 tag; + u8 len; + u8 ssid[8]; + } __packed parts;
Quoted 8 lines
drivers/net/wireless/virt_wifi.c line 172
Quoted 4 lines
drivers/net/wireless/virt_wifi.c new file +4
-0+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.
Quoted 9 lines
drivers/net/wireless/virt_wifi.c line 200
Quoted 7 lines
drivers/net/wireless/virt_wifi.c new file +6
-0+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_requestlocking?
I've redone the structure a bit to clean this up, and don't use the lock here anymore.
Quoted 4 lines
Quoted 4 lines
drivers/net/wireless/virt_wifi.c new file +4
-0+static int virt_wifi_get_station (struct wiphy *wiphy, +struct net_device *dev, +const u8 *mac, +struct station_info *sinfo)
Quoted 8 lines
drivers/net/wireless/virt_wifi.c line 287
Quoted 5 lines
drivers/net/wireless/virt_wifi.c new file +4
-0+{ + 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
Quoted 3 lines
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.
Quoted 15 lines
drivers/net/wireless/virt_wifi.c line 434
Quoted 12 lines
drivers/net/wireless/virt_wifi.c new file +12
-0+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
Quoted 13 lines
drivers/net/wireless/virt_wifi.c line 449
Quoted 6 lines
drivers/net/wireless/virt_wifi.c new file +6
-0+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.
Quoted 6 lines
drivers/net/wireless/virt_wifi.c line 563
Quoted 4 lines
drivers/net/wireless/virt_wifi.c new file +4
-0+/* 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