* [PATCH next] wifi: nl80211: emit CMD_START_AP on multicast group when an AP is started @ 2022-12-09 15:28 Alvin Šipraga 2023-01-18 16:26 ` Johannes Berg 0 siblings, 1 reply; 4+ messages in thread From: Alvin Šipraga @ 2022-12-09 15:28 UTC (permalink / raw) To: Johannes Berg, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Alvin Šipraga, linux-wireless, netdev, linux-kernel From: Alvin Šipraga <alsi@bang-olufsen.dk> Userspace processes such as network daemons may wish to be informed when any AP interface is brought up on the system, for example to initiate a (re)configuration of IP settings or to start a DHCP server. Currently nl80211 does not broadcast any such event on its multicast groups, leaving userspace only two options: 1. the process must be the one that actually issued the NL80211_CMD_START_AP request, so that it can react on the response to that request; 2. the process must react to RTM_NEWLINK events indicating a change in carrier state, and may query for further information about the AP and react accordingly. Option (1) is robust, but it does not cover all scenarios. It is easy to imagine a situation where this is not the case (e.g. hostapd + systemd-networkd). Option (2) is not robust, because RTM_NEWLINK events may be silently discarded by the linkwatch logic (cf. linkwatch_fire_event()). Concretely, consider a scenario in which the carrier state flip-flops in the following way: ^ carrier state (high/low = carrier/no carrier) | | _______ _______ ... | | | | | ______| "foo" |____| "bar" (SSID in "quotes") | +-------A-------B----C---------> time If the time interval between (A) and (C) is less than 1 second, then linkwatch may emit only a single RTM_NEWLINK event indicating carrier gain. This is problematic because it is possible that the network configuration that should be applied is a function of the AP's properties such as SSID (cf. SSID= in systemd.network(5)). As illustrated in the above diagram, it may be that the AP with SSID "bar" ends up being configured as though it had SSID "foo". Address the above issue by having nl80211 emit an NL80211_CMD_START_AP message on the MLME nl80211 multicast group. This allows for arbitrary processes to be reliably informed. Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk> --- net/wireless/nl80211.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 33a82ecab9d5..323b7e40d855 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -5770,6 +5770,39 @@ static bool nl80211_valid_auth_type(struct cfg80211_registered_device *rdev, } } +static void nl80211_send_ap_started(struct wireless_dev *wdev) +{ + struct wiphy *wiphy = wdev->wiphy; + struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy); + struct sk_buff *msg; + void *hdr; + + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); + if (!msg) + return; + + hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_START_AP); + if (!hdr) + goto out; + + if (nla_put_u32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx) || + nla_put_u32(msg, NL80211_ATTR_IFINDEX, wdev->netdev->ifindex) || + nla_put_u64_64bit(msg, NL80211_ATTR_WDEV, wdev_id(wdev), + NL80211_ATTR_PAD) || + (wdev->u.ap.ssid_len && + nla_put(msg, NL80211_ATTR_SSID, wdev->u.ap.ssid_len, + wdev->u.ap.ssid))) + goto out; + + genlmsg_end(msg, hdr); + + genlmsg_multicast_netns(&nl80211_fam, wiphy_net(wiphy), msg, 0, + NL80211_MCGRP_MLME, GFP_KERNEL); + return; +out: + nlmsg_free(msg); +} + static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info) { struct cfg80211_registered_device *rdev = info->user_ptr[0]; @@ -6050,6 +6083,8 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info) if (info->attrs[NL80211_ATTR_SOCKET_OWNER]) wdev->conn_owner_nlportid = info->snd_portid; + + nl80211_send_ap_started(wdev); } out_unlock: wdev_unlock(wdev); -- 2.37.3 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH next] wifi: nl80211: emit CMD_START_AP on multicast group when an AP is started 2022-12-09 15:28 [PATCH next] wifi: nl80211: emit CMD_START_AP on multicast group when an AP is started Alvin Šipraga @ 2023-01-18 16:26 ` Johannes Berg 2023-01-21 13:07 ` Alvin Šipraga 0 siblings, 1 reply; 4+ messages in thread From: Johannes Berg @ 2023-01-18 16:26 UTC (permalink / raw) To: Alvin Šipraga, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Alvin Šipraga, linux-wireless, netdev, linux-kernel On Fri, 2022-12-09 at 16:28 +0100, Alvin Šipraga wrote: > > This is problematic because it is possible that the network > configuration that should be applied is a function of the AP's > properties such as SSID (cf. SSID= in systemd.network(5)). As > illustrated in the above diagram, it may be that the AP with SSID "bar" > ends up being configured as though it had SSID "foo". > You might not care if you want the SSID, but it still seems wrong: > +static void nl80211_send_ap_started(struct wireless_dev *wdev) > +{ > + struct wiphy *wiphy = wdev->wiphy; > + struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy); > + struct sk_buff *msg; > + void *hdr; > + > + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); > + if (!msg) > + return; > + > + hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_START_AP); > + if (!hdr) > + goto out; > + > + if (nla_put_u32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx) || > + nla_put_u32(msg, NL80211_ATTR_IFINDEX, wdev->netdev->ifindex) || > + nla_put_u64_64bit(msg, NL80211_ATTR_WDEV, wdev_id(wdev), > + NL80211_ATTR_PAD) || > + (wdev->u.ap.ssid_len && > + nla_put(msg, NL80211_ATTR_SSID, wdev->u.ap.ssid_len, > + wdev->u.ap.ssid))) > + goto out; > + > + genlmsg_end(msg, hdr); > + > + genlmsg_multicast_netns(&nl80211_fam, wiphy_net(wiphy), msg, 0, > + NL80211_MCGRP_MLME, GFP_KERNEL); > + return; > +out: > + nlmsg_free(msg); > +} This has no indication of the link, but with multi-link you could actually be sending this event multiple times to userspace on the same netdev. > static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info) > { > struct cfg80211_registered_device *rdev = info->user_ptr[0]; > @@ -6050,6 +6083,8 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info) > > if (info->attrs[NL80211_ATTR_SOCKET_OWNER]) > wdev->conn_owner_nlportid = info->snd_portid; > + > + nl80211_send_ap_started(wdev); > } because this can be called multiple times, once for each link. Seems like you should include the link ID or something? johannes ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH next] wifi: nl80211: emit CMD_START_AP on multicast group when an AP is started 2023-01-18 16:26 ` Johannes Berg @ 2023-01-21 13:07 ` Alvin Šipraga 2023-01-24 9:26 ` Johannes Berg 0 siblings, 1 reply; 4+ messages in thread From: Alvin Šipraga @ 2023-01-21 13:07 UTC (permalink / raw) To: Johannes Berg Cc: Alvin Šipraga, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-wireless, netdev, linux-kernel Hi Johannes, On Wed, Jan 18, 2023 at 05:26:19PM +0100, Johannes Berg wrote: > On Fri, 2022-12-09 at 16:28 +0100, Alvin Šipraga wrote: > > > > This is problematic because it is possible that the network > > configuration that should be applied is a function of the AP's > > properties such as SSID (cf. SSID= in systemd.network(5)). As > > illustrated in the above diagram, it may be that the AP with SSID "bar" > > ends up being configured as though it had SSID "foo". > > > > You might not care if you want the SSID, but it still seems wrong: > > > +static void nl80211_send_ap_started(struct wireless_dev *wdev) > > +{ > > + struct wiphy *wiphy = wdev->wiphy; > > + struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy); > > + struct sk_buff *msg; > > + void *hdr; > > + > > + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); > > + if (!msg) > > + return; > > + > > + hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_START_AP); > > + if (!hdr) > > + goto out; > > + > > + if (nla_put_u32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx) || > > + nla_put_u32(msg, NL80211_ATTR_IFINDEX, wdev->netdev->ifindex) || > > + nla_put_u64_64bit(msg, NL80211_ATTR_WDEV, wdev_id(wdev), > > + NL80211_ATTR_PAD) || > > + (wdev->u.ap.ssid_len && > > + nla_put(msg, NL80211_ATTR_SSID, wdev->u.ap.ssid_len, > > + wdev->u.ap.ssid))) > > + goto out; > > + > > + genlmsg_end(msg, hdr); > > + > > + genlmsg_multicast_netns(&nl80211_fam, wiphy_net(wiphy), msg, 0, > > + NL80211_MCGRP_MLME, GFP_KERNEL); > > + return; > > +out: > > + nlmsg_free(msg); > > +} > > This has no indication of the link, but with multi-link you could > actually be sending this event multiple times to userspace on the same > netdev. > > > static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info) > > { > > struct cfg80211_registered_device *rdev = info->user_ptr[0]; > > @@ -6050,6 +6083,8 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info) > > > > if (info->attrs[NL80211_ATTR_SOCKET_OWNER]) > > wdev->conn_owner_nlportid = info->snd_portid; > > + > > + nl80211_send_ap_started(wdev); > > } > > because this can be called multiple times, once for each link. > > Seems like you should include the link ID or something? Thanks for your review, you are quite right. I didn't give much thought to MLO as I am not too familiar with it. Is something like the below what you are looking for? Speaking of which: I drew inspiration from nl80211_send_ap_stopped() which see also doesn't include the link ID. Would you like me to include a second patch in v2 which adds the link ID to that function along the same lines? Kind regards, Alvin -------------->8---------------------------------- diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index ba4965f035b2..5720ffcbecc4 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -5770,7 +5770,7 @@ static bool nl80211_valid_auth_type(struct cfg80211_registered_device *rdev, } } -static void nl80211_send_ap_started(struct wireless_dev *wdev) +static void nl80211_send_ap_started(struct wireless_dev *wdev, unsigned int link_id) { struct wiphy *wiphy = wdev->wiphy; struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy); @@ -5791,7 +5791,9 @@ static void nl80211_send_ap_started(struct wireless_dev *wdev) NL80211_ATTR_PAD) || (wdev->u.ap.ssid_len && nla_put(msg, NL80211_ATTR_SSID, wdev->u.ap.ssid_len, - wdev->u.ap.ssid))) + wdev->u.ap.ssid)) || + (wdev->valid_links && + nla_put_u8(msg, NL80211_ATTR_MLO_LINK_ID, link_id))) goto out; genlmsg_end(msg, hdr); @@ -6084,7 +6086,7 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info) if (info->attrs[NL80211_ATTR_SOCKET_OWNER]) wdev->conn_owner_nlportid = info->snd_portid; - nl80211_send_ap_started(wdev); + nl80211_send_ap_started(wdev, link_id); } out_unlock: wdev_unlock(wdev); ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH next] wifi: nl80211: emit CMD_START_AP on multicast group when an AP is started 2023-01-21 13:07 ` Alvin Šipraga @ 2023-01-24 9:26 ` Johannes Berg 0 siblings, 0 replies; 4+ messages in thread From: Johannes Berg @ 2023-01-24 9:26 UTC (permalink / raw) To: Alvin Šipraga Cc: Alvin Šipraga, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-wireless, netdev, linux-kernel Hi, > > Seems like you should include the link ID or something? > > Thanks for your review, you are quite right. I didn't give much thought > to MLO as I am not too familiar with it. Is something like the below > what you are looking for? Yes, that looks good. > Speaking of which: I drew inspiration from nl80211_send_ap_stopped() > which see also doesn't include the link ID. Would you like me to include > a second patch in v2 which adds the link ID to that function along the > same lines? Maybe have that as a separate patch, but yeah, good idea - thanks for looking! johannes ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-24 9:27 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-12-09 15:28 [PATCH next] wifi: nl80211: emit CMD_START_AP on multicast group when an AP is started Alvin Šipraga 2023-01-18 16:26 ` Johannes Berg 2023-01-21 13:07 ` Alvin Šipraga 2023-01-24 9:26 ` Johannes Berg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).