| Message ID | 20230926160950.d698c25528e3.If118a835a25c59de20e1728ab71949fdb4172fb2@changeid (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: nl80211: remove set_fils_aad support | expand |
On 9/26/2023 7:09 AM, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > There's no user for this, so remove the support. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> if you are going to remove it, why not just revert e306784a8de0 ("cfg80211: AP mode driver offload for FILS association crypto") to make sure you get all of the artifacts? for example, I believe your patch will leave behind an unused struct cfg80211_fils_aad and unused FILS_AAD_ASSIGN() trace helper macro. the caveat to reverting is that it should only be a partial revert; the UAPI definitions would need to be retained (and should be documented as obsolete). however, let me check to make sure there is no plan to actually utilize this interface upstream. as i've indicated earlier, we are in the process of trying to transition to an "upstream first" mentality, but this is not going to happen overnight, but instead will take years. that said, i'd hate to rip out an interface now just to need to add it back in the future. /jeff
On Tue, 2023-09-26 at 13:35 -0700, Jeff Johnson wrote: > On 9/26/2023 7:09 AM, Johannes Berg wrote: > > From: Johannes Berg <johannes.berg@intel.com> > > > > There's no user for this, so remove the support. > > > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > > if you are going to remove it, why not just revert e306784a8de0 > ("cfg80211: AP mode driver offload for FILS association crypto") to make > sure you get all of the artifacts? for example, I believe your patch > will leave behind an unused struct cfg80211_fils_aad and unused > FILS_AAD_ASSIGN() trace helper macro. Hah, good point, I didn't do this well. > the caveat to reverting is that it should only be a partial revert; the > UAPI definitions would need to be retained (and should be documented as > obsolete). Yeah, that's why I didn't do it as a revert. > however, let me check to make sure there is no plan to actually utilize > this interface upstream. as i've indicated earlier, we are in the > process of trying to transition to an "upstream first" mentality, but > this is not going to happen overnight, but instead will take years. that > said, i'd hate to rip out an interface now just to need to add it back > in the future. Sure. I don't mind keeping something around that really _has_ a future, but it's been two years and nobody showed up ... but yeah, I also think that "has a future" means upstream. And clearly the old "other people can use it" argument doesn't work any more either, the only other vendors who are doing something in AP mode are Mediatek and maybe to some extent Realtek, and they all work on top of mac80211 with thinner firmware. Broadcom has disappeared as far as I can tell, with the occasional patch like recently that I'm also suspecting serves pure out-of-tree driver purposes... johannes
On 9/27/2023 12:00 AM, Johannes Berg wrote: > On Tue, 2023-09-26 at 13:35 -0700, Jeff Johnson wrote: >> On 9/26/2023 7:09 AM, Johannes Berg wrote: >>> From: Johannes Berg <johannes.berg@intel.com> >>> >>> There's no user for this, so remove the support. >>> >>> Signed-off-by: Johannes Berg <johannes.berg@intel.com> >> >> if you are going to remove it, why not just revert e306784a8de0 >> ("cfg80211: AP mode driver offload for FILS association crypto") to make >> sure you get all of the artifacts? for example, I believe your patch >> will leave behind an unused struct cfg80211_fils_aad and unused >> FILS_AAD_ASSIGN() trace helper macro. > > Hah, good point, I didn't do this well. > >> the caveat to reverting is that it should only be a partial revert; the >> UAPI definitions would need to be retained (and should be documented as >> obsolete). > > Yeah, that's why I didn't do it as a revert. > >> however, let me check to make sure there is no plan to actually utilize >> this interface upstream. as i've indicated earlier, we are in the >> process of trying to transition to an "upstream first" mentality, but >> this is not going to happen overnight, but instead will take years. that >> said, i'd hate to rip out an interface now just to need to add it back >> in the future. > > Sure. I don't mind keeping something around that really _has_ a future, > but it's been two years and nobody showed up ... but yeah, I also think > that "has a future" means upstream. > > And clearly the old "other people can use it" argument doesn't work any > more either, the only other vendors who are doing something in AP mode > are Mediatek and maybe to some extent Realtek, and they all work on top > of mac80211 with thinner firmware. Broadcom has disappeared as far as I > can tell, with the occasional patch like recently that I'm also > suspecting serves pure out-of-tree driver purposes... Based on internal discussion I don't see a path forward to using this interface upstream, so please continue with the extraction, incorporating my prior comments. Thanks! /jeff
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 3a4b684f89bf..9f930750db2e 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -4384,10 +4384,6 @@ struct mgmt_frame_regs { * * @color_change: Initiate a color change. * - * @set_fils_aad: Set FILS AAD data to the AP driver so that the driver can use - * those to decrypt (Re)Association Request and encrypt (Re)Association - * Response frame. - * * @set_radar_background: Configure dedicated offchannel chain available for * radar/CAC detection on some hw. This chain can't be used to transmit * or receive frames and it is bounded to a running wdev. @@ -4748,8 +4744,6 @@ struct cfg80211_ops { int (*color_change)(struct wiphy *wiphy, struct net_device *dev, struct cfg80211_color_change_settings *params); - int (*set_fils_aad)(struct wiphy *wiphy, struct net_device *dev, - struct cfg80211_fils_aad *fils_aad); int (*set_radar_background)(struct wiphy *wiphy, struct cfg80211_chan_def *chandef); int (*add_link_station)(struct wiphy *wiphy, struct net_device *dev, diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index de47838aca4f..45efc79bfa3c 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -16079,29 +16079,6 @@ static int nl80211_color_change(struct sk_buff *skb, struct genl_info *info) return err; } -static int nl80211_set_fils_aad(struct sk_buff *skb, - struct genl_info *info) -{ - struct cfg80211_registered_device *rdev = info->user_ptr[0]; - struct net_device *dev = info->user_ptr[1]; - struct cfg80211_fils_aad fils_aad = {}; - u8 *nonces; - - if (!info->attrs[NL80211_ATTR_MAC] || - !info->attrs[NL80211_ATTR_FILS_KEK] || - !info->attrs[NL80211_ATTR_FILS_NONCES]) - return -EINVAL; - - fils_aad.macaddr = nla_data(info->attrs[NL80211_ATTR_MAC]); - fils_aad.kek_len = nla_len(info->attrs[NL80211_ATTR_FILS_KEK]); - fils_aad.kek = nla_data(info->attrs[NL80211_ATTR_FILS_KEK]); - nonces = nla_data(info->attrs[NL80211_ATTR_FILS_NONCES]); - fils_aad.snonce = nonces; - fils_aad.anonce = nonces + FILS_NONCE_LEN; - - return rdev_set_fils_aad(rdev, dev, &fils_aad); -} - static int nl80211_add_link(struct sk_buff *skb, struct genl_info *info) { struct cfg80211_registered_device *rdev = info->user_ptr[0]; @@ -17452,13 +17429,6 @@ static const struct genl_small_ops nl80211_small_ops[] = { .flags = GENL_UNS_ADMIN_PERM, .internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV_UP), }, - { - .cmd = NL80211_CMD_SET_FILS_AAD, - .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, - .doit = nl80211_set_fils_aad, - .flags = GENL_UNS_ADMIN_PERM, - .internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV_UP), - }, { .cmd = NL80211_CMD_ADD_LINK, .doit = nl80211_add_link, diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h index 90bb7ac4b930..9dbad6ecbc6d 100644 --- a/net/wireless/rdev-ops.h +++ b/net/wireless/rdev-ops.h @@ -1402,20 +1402,6 @@ static inline int rdev_color_change(struct cfg80211_registered_device *rdev, return ret; } -static inline int -rdev_set_fils_aad(struct cfg80211_registered_device *rdev, - struct net_device *dev, struct cfg80211_fils_aad *fils_aad) -{ - int ret = -EOPNOTSUPP; - - trace_rdev_set_fils_aad(&rdev->wiphy, dev, fils_aad); - if (rdev->ops->set_fils_aad) - ret = rdev->ops->set_fils_aad(&rdev->wiphy, dev, fils_aad); - trace_rdev_return_int(&rdev->wiphy, ret); - - return ret; -} - static inline int rdev_set_radar_background(struct cfg80211_registered_device *rdev, struct cfg80211_chan_def *chandef) diff --git a/net/wireless/trace.h b/net/wireless/trace.h index 617c0d0dfa96..c6870c311cdf 100644 --- a/net/wireless/trace.h +++ b/net/wireless/trace.h @@ -2706,24 +2706,6 @@ DEFINE_EVENT(wiphy_wdev_cookie_evt, rdev_abort_pmsr, TP_ARGS(wiphy, wdev, cookie) ); -TRACE_EVENT(rdev_set_fils_aad, - TP_PROTO(struct wiphy *wiphy, struct net_device *netdev, - struct cfg80211_fils_aad *fils_aad), - TP_ARGS(wiphy, netdev, fils_aad), - TP_STRUCT__entry(WIPHY_ENTRY - NETDEV_ENTRY - __array(u8, macaddr, ETH_ALEN) - __field(u8, kek_len) - ), - TP_fast_assign(WIPHY_ASSIGN; - NETDEV_ASSIGN; - FILS_AAD_ASSIGN(fils_aad); - ), - TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", " FILS_AAD_PR_FMT, - WIPHY_PR_ARG, NETDEV_PR_ARG, __entry->macaddr, - __entry->kek_len) -); - TRACE_EVENT(rdev_update_owe_info, TP_PROTO(struct wiphy *wiphy, struct net_device *netdev, struct cfg80211_update_owe_info *owe_info),