Messages in this thread | | | Subject | Re: [PATCH net-next v2] wireless-drivers: rtnetlink wifi simulation device | From | Johannes Berg <> | Date | Thu, 27 Sep 2018 12:10:34 +0200 |
| |
On Wed, 2018-09-26 at 12:43 -0700, Cody Schuffelen wrote:
> ip link set eth0 down > ip link set eth0 name buried_eth0 > ip link set buried_eth0 up > ip link add link buried_eth0 name wlan0 type virt_wifi > > eth0 is renamed to buried_eth0 to avoid a network manager trying to > manage it,
I feel you should remove this "buried" thing at least from the plain instructions - first of all, I'm not convinced it actually *works* in general (network managers are able to find it anyway, right? perhaps android's has some rules?), and it's not really necessary from a kernel POV.
Perhaps add a note like "you may have to rename or otherwise hide the eth0 from your connection manager" (also "NetworkManager" is a specific software, so "network manager" is confusing - I actually first thought you meant NM).
Anyway, just a thought.
> Thanks for the detailed review! I believe I've addressed all comments.
Thanks!
> The problem we've experienced with routing the hwsim virtual medium from the > inside the VM to outside the VM is this required running hwsim on the host > kernel as well. This is too intrusive for our use-case, whereas wifi that > exists completely inside the VM kernel is much more palatable.
Fair enough. I still think it'd be worthwhile in the long run because then you could start multiple APs to simulate roaming etc. without having to implement all of this here.
IOW - I don't think we should extend this much. As is, I think it's fine, but I wouldn't want to see extensions like "have two scan result", "let userspace control the RSSI of the scan results to force 'roaming'" etc.
> +static struct ieee80211_channel channel_2ghz = { > + .band = NL80211_BAND_2GHZ, > + .center_freq = 5500, > + .hw_value = 5500,
That doesn't seem right - a 2 GHz channel that's really 5.5 GHz?
> + .max_power = 5500,
That seems more like a copy/paste bug rather than intentional?
> +static struct ieee80211_rate bitrates_2ghz[] = { > + { > + .bitrate = 10, > + }, { > + .bitrate = 20, > + }, { > + .bitrate = 55, > + }, { > + .bitrate = 60, > + }, { > + .bitrate = 110, > + }, { > + .bitrate = 120, > + }, { > + .bitrate = 240, > + }, > +};
That's ... strangely formatted? I guess we'd usually write
{ .bitrate = 10 }, { .bitrate = 20 }, ...
> +static struct ieee80211_supported_band band_2ghz = { > + .channels = &channel_2ghz, > + .bitrates = bitrates_2ghz, > + .band = NL80211_BAND_2GHZ, > + .n_channels = 1, > + .n_bitrates = 7,
Please use ARRAY_SIZE()
> + .ht_cap = { > + .ht_supported = true, > + }, > + .vht_cap = { > + .vht_supported = true, > + },
That looks _really_ bare - you may want to copy something sane to here.
> +}; > + > +static struct ieee80211_channel channel_5ghz = {
> + .max_power = 5500,
same here - max_power 5500?
> + .max_reg_power = 9999, > +}; > + > +static struct ieee80211_rate bitrates_5ghz[] = { > + { > + .bitrate = 60, > + }, { > + .bitrate = 120, > + }, { > + .bitrate = 240, > + }, > +};
Same comment here regarding formatting.
> +static struct ieee80211_supported_band band_5ghz = { > + .channels = &channel_5ghz, > + .bitrates = bitrates_5ghz, > + .band = NL80211_BAND_5GHZ, > + .n_channels = 1, > + .n_bitrates = 3,
and ARRAY_SIZE
> + .ht_cap = { > + .ht_supported = true, > + }, > + .vht_cap = { > + .vht_supported = true, > + },
and HT/VHT
> +/** Assigned at module init. Guaranteed locally-administered and unicast. */ > +static u8 fake_router_bssid[ETH_ALEN] = {};
Maybe make that __ro_after_init?
It doesn't matter that much, but is a good signal.
> +static void virt_wifi_scan_result(struct work_struct *work) > +{ > + char 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(), > + }; > + > + ssid[0] = WLAN_EID_SSID; > + /* size of the array minus null terminator, length byte, tag byte */ > + ssid[1] = sizeof(ssid) - 3;
Seems like you could make that const? struct { u8 tag; u8 len; u8 ssid[8]; } ssid = { .tag = 0, .len = 8, .ssid = "VirtWifi" };
or something like that?
Hmm, maybe that doesn't work.. whatever, not really important.
> + 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, > + /* Truncate before the null. */ > + sizeof(ssid) - 1, GFP_KERNEL); > + cfg80211_put_bss(wiphy, informed_bss); > + > + informed_bss = cfg80211_inform_bss_data(wiphy, &mock_inform_bss, > + CFG80211_BSS_FTYPE_BEACON, > + fake_router_bssid, > + mock_inform_bss.boottime_ns, > + WLAN_CAPABILITY_ESS, 0, ssid, > + /* Truncate before the null. */ > + sizeof(ssid) - 1, GFP_KERNEL); > + cfg80211_put_bss(wiphy, informed_bss);
Hmm, what's the point of doing it twice? You don't really need to receive both PRESP/BEACON, just one is sufficient.
> + schedule_delayed_work(&priv->scan_complete, HZ * 2); > +}
I don't think you need to make that async again.
> +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; > + > + if (sme->bssid && !ether_addr_equal(sme->bssid, fake_router_bssid)) > + return -EINVAL;
If you wanted to be more "real", you'd accept this and then check it in the connect worker, rejecting it there if mismatched.
> +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"); > + 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; > +}
You should check that mac is fake_router_bssid, and otherwise return not found (-ENOENT), IIRC.
> +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 NETDEV_TX_BUSY;
I think you should just drop the frame.
> +/* Called under rcu_read_lock() from netif_receive_skb */ > +static rx_handler_result_t virt_wifi_rx_handler(struct sk_buff **pskb)
FWIW, the stuff beyond this point, especially the netlink, I'm less familiar with.
> +/** Called with rtnl lock held. */
/** is usually used only for kernel-doc
johannes
| |