| Message ID | 20220324123906.276337-1-nico.escande@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ACS: always discard DFS channels when DFS isn't allowed | expand |
On Thu, Mar 24, 2022 at 01:39:06PM +0100, Nicolas Escande wrote: > When you shouldn't use DFS channels (either disabled in conf or the > driver doesn't support radar detection) we normaly mark all DFS cahnnels > as disabled to prevent them from being used. > However, there is a loophole when the driver exports > WPA_DRIVER_FLAGS_DFS_OFFLOAD, this step is bypassed. > Just because the driver handles DFS event without hostapd's help > shouldn't impact ACS in any way, it is different from ACS offload. > > This led me to intanses where an AP would be brought up on a DFS > channel, selected by ACS, while it shouldn't have (ieee80211h=0) > diff --git a/src/ap/hw_features.c b/src/ap/hw_features.c > @@ -119,9 +119,7 @@ int hostapd_get_hw_features(struct hostapd_iface *iface) > HOSTAPD_CHAN_RADAR) && dfs_enabled) { > dfs = 1; > } else if (((feature->channels[j].flag & > - HOSTAPD_CHAN_RADAR) && > - !(iface->drv_flags & > - WPA_DRIVER_FLAGS_DFS_OFFLOAD)) || > + HOSTAPD_CHAN_RADAR) && !dfs_enabled) || > (feature->channels[j].flag & > HOSTAPD_CHAN_NO_IR)) { > feature->channels[j].flag |= Why is this removing the use of WPA_DRIVER_FLAGS_DFS_OFFLOAD completely here? I'm not completely sure I understood what kind of a case could have allowed ACS to select a DFS channel incorrectly. Maybe that is something that should be addressed in the ACS implementation instead? Please note that the DFS offload cases might use different style for configuring the DFS operations, so it is not clear whether the ieee80211h parameter is really applicable in all such cases and this type of a change could result in breaking something that is already deployed.
On Sun Apr 17, 2022 at 11:18 AM CEST, Jouni Malinen wrote: > Why is this removing the use of WPA_DRIVER_FLAGS_DFS_OFFLOAD completely > here? I'm not completely sure I understood what kind of a case could > have allowed ACS to select a DFS channel incorrectly. Maybe that is > something that should be addressed in the ACS implementation instead? > > Please note that the DFS offload cases might use different style for > configuring the DFS operations, so it is not clear whether the > ieee80211h parameter is really applicable in all such cases and this > type of a change could result in breaking something that is already > deployed. So this code fetches the channels supported by the hardware and applies flags on it depending on whether or not we can use them and under what conditions. So for a regular driver (one that does not sets the WPA_DRIVER_FLAGS_DFS_OFFLOAD flag), when DFS is disabled by hostapd conf, any DFS channel will be flagged as disabled. This gets bypassed when the driver sets WPA_DRIVER_FLAGS_DFS_OFFLOAD. From what I understood, the DFS offload flag means that the driver itself should react to DFS events and handle things like CAC wait times. It shouldn't imply anything for the rest of the code, it shouldn't hide the fact that this channel shouldn't be used for example. This is problematic when doing ACS because we usually do not take into account any channel that is flagged as disabled. So this applies to DFS channels when the conf doesn't allow them. This lead me to cases where a conf that explicitely doesn't allow DFS channel (ieee80211h=0) to be used with ACS still enabled, still selected a DFS channel because the driver handles DFS itself with WPA_DRIVER_FLAGS_DFS_OFFLOAD flag, but doesn't bypass our ACS implementation with WPA_DRIVER_FLAGS_ACS_OFFLOAD. I understand that this could have implications but for me the "right" behaviour should be : - non usable channels should be flagged as such (so when conf doesn't allow them exclude them) - hostapd's ACS implem should select a "usable" channels from the list (unless ACS is offloaded with WPA_DRIVER_FLAGS_ACS_OFFLOAD in which case all bets are off) What do you think ? > -- > Jouni Malinen PGP id EFC895FA
On Tue Apr 19, 2022 at 11:18 AM CEST, Nicolas Escande wrote: > On Sun Apr 17, 2022 at 11:18 AM CEST, Jouni Malinen wrote: > > Why is this removing the use of WPA_DRIVER_FLAGS_DFS_OFFLOAD completely > > here? I'm not completely sure I understood what kind of a case could > > have allowed ACS to select a DFS channel incorrectly. Maybe that is > > something that should be addressed in the ACS implementation instead? > > > > Please note that the DFS offload cases might use different style for > > configuring the DFS operations, so it is not clear whether the > > ieee80211h parameter is really applicable in all such cases and this > > type of a change could result in breaking something that is already > > deployed. > > So this code fetches the channels supported by the hardware and applies > flags on it depending on whether or not we can use them and under what > conditions. So for a regular driver (one that does not sets the > WPA_DRIVER_FLAGS_DFS_OFFLOAD flag), when DFS is disabled by hostapd conf, > any DFS channel will be flagged as disabled. This gets bypassed when the > driver sets WPA_DRIVER_FLAGS_DFS_OFFLOAD. > > From what I understood, the DFS offload flag means that the driver > itself should react to DFS events and handle things like CAC wait times. > It shouldn't imply anything for the rest of the code, it shouldn't hide > the fact that this channel shouldn't be used for example. > > This is problematic when doing ACS because we usually do not take into > account any channel that is flagged as disabled. So this applies to DFS > channels when the conf doesn't allow them. > > This lead me to cases where a conf that explicitely doesn't allow DFS > channel (ieee80211h=0) to be used with ACS still enabled, still selected > a DFS channel because the driver handles DFS itself with > WPA_DRIVER_FLAGS_DFS_OFFLOAD flag, but doesn't bypass our ACS > implementation with WPA_DRIVER_FLAGS_ACS_OFFLOAD. > > I understand that this could have implications but for me the "right" > behaviour should be : > - non usable channels should be flagged as such (so when conf doesn't > allow them exclude them) > - hostapd's ACS implem should select a "usable" channels from the list > (unless ACS is offloaded with WPA_DRIVER_FLAGS_ACS_OFFLOAD in which > case all bets are off) > > What do you think ? > > > -- > > Jouni Malinen PGP id EFC895FA Hi Jouni, Any new input on this ?
On Sun May 8, 2022 at 12:42 PM CEST, Nicolas Escande wrote: > On Tue Apr 19, 2022 at 11:18 AM CEST, Nicolas Escande wrote: > > On Sun Apr 17, 2022 at 11:18 AM CEST, Jouni Malinen wrote: > > > Why is this removing the use of WPA_DRIVER_FLAGS_DFS_OFFLOAD completely > > > here? I'm not completely sure I understood what kind of a case could > > > have allowed ACS to select a DFS channel incorrectly. Maybe that is > > > something that should be addressed in the ACS implementation instead? > > > > > > Please note that the DFS offload cases might use different style for > > > configuring the DFS operations, so it is not clear whether the > > > ieee80211h parameter is really applicable in all such cases and this > > > type of a change could result in breaking something that is already > > > deployed. > > > > So this code fetches the channels supported by the hardware and applies > > flags on it depending on whether or not we can use them and under what > > conditions. So for a regular driver (one that does not sets the > > WPA_DRIVER_FLAGS_DFS_OFFLOAD flag), when DFS is disabled by hostapd conf, > > any DFS channel will be flagged as disabled. This gets bypassed when the > > driver sets WPA_DRIVER_FLAGS_DFS_OFFLOAD. > > > > From what I understood, the DFS offload flag means that the driver > > itself should react to DFS events and handle things like CAC wait times. > > It shouldn't imply anything for the rest of the code, it shouldn't hide > > the fact that this channel shouldn't be used for example. > > > > This is problematic when doing ACS because we usually do not take into > > account any channel that is flagged as disabled. So this applies to DFS > > channels when the conf doesn't allow them. > > > > This lead me to cases where a conf that explicitely doesn't allow DFS > > channel (ieee80211h=0) to be used with ACS still enabled, still selected > > a DFS channel because the driver handles DFS itself with > > WPA_DRIVER_FLAGS_DFS_OFFLOAD flag, but doesn't bypass our ACS > > implementation with WPA_DRIVER_FLAGS_ACS_OFFLOAD. > > > > I understand that this could have implications but for me the "right" > > behaviour should be : > > - non usable channels should be flagged as such (so when conf doesn't > > allow them exclude them) > > - hostapd's ACS implem should select a "usable" channels from the list > > (unless ACS is offloaded with WPA_DRIVER_FLAGS_ACS_OFFLOAD in which > > case all bets are off) > > > > What do you think ? > > > > > -- > > > Jouni Malinen PGP id EFC895FA > > Hi Jouni, > Any new input on this ? Hi there, As I see some older stuff gets reviewed, any chance this gets another look at ?
diff --git a/src/ap/hw_features.c b/src/ap/hw_features.c index 2b66ab563..406f2b38f 100644 --- a/src/ap/hw_features.c +++ b/src/ap/hw_features.c @@ -119,9 +119,7 @@ int hostapd_get_hw_features(struct hostapd_iface *iface) HOSTAPD_CHAN_RADAR) && dfs_enabled) { dfs = 1; } else if (((feature->channels[j].flag & - HOSTAPD_CHAN_RADAR) && - !(iface->drv_flags & - WPA_DRIVER_FLAGS_DFS_OFFLOAD)) || + HOSTAPD_CHAN_RADAR) && !dfs_enabled) || (feature->channels[j].flag & HOSTAPD_CHAN_NO_IR)) { feature->channels[j].flag |=
When you shouldn't use DFS channels (either disabled in conf or the driver doesn't support radar detection) we normaly mark all DFS cahnnels as disabled to prevent them from being used. However, there is a loophole when the driver exports WPA_DRIVER_FLAGS_DFS_OFFLOAD, this step is bypassed. Just because the driver handles DFS event without hostapd's help shouldn't impact ACS in any way, it is different from ACS offload. This led me to intanses where an AP would be brought up on a DFS channel, selected by ACS, while it shouldn't have (ieee80211h=0) Signed-off-by: Nicolas Escande <nico.escande@gmail.com> --- src/ap/hw_features.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)