Hi Johannes,
Thanks for the review comments.
>Maybe it'd be better to split set/del now? The code kind of looks
>awkward now, don't you think?
>Or split this part of the parsing depending on set or del?
As suggested, set and del pmksa handling is split.
>The indentation here is also really awful ... I'd rather go over 80
>columns than break like that. But you could just have local variables
>for all the feature checks too.
>And if you don't split set/del, I'd recommend a variable for that too,
>set at the beginning, perhaps moving the "rdev_ops" thing up? But I'm
>thinking it's probably nicer to split it. See how that looks like?
Addressed comments by splitting set and del pmksa and using local
variables for feature checks.
>Then again, that isn't even relevant for DEL, is it? Should we even
>parse it? Does anyone want to "delete only if it's exactly this PMK"?
Removed this handling for DEL pmksa.
>Also seems like this should come with some nl80211.h updates though?
Added additional description for the DEL pmksa documentation.
Regards,
Vinayak
On Sat, Nov 25, 2023 at 12:28 AM Johannes Berg
<[email protected]> wrote:
>
> On Thu, 2023-11-09 at 18:00 +0530, Vinayak Yadawad wrote:
> >
> > +++ b/net/wireless/nl80211.c
> > @@ -12183,24 +12183,37 @@ static int nl80211_setdel_pmksa(struct sk_buff *skb, struct genl_info *info)
> >
> > memset(&pmksa, 0, sizeof(struct cfg80211_pmksa));
> >
> > - if (!info->attrs[NL80211_ATTR_PMKID])
> > + if ((info->genlhdr->cmd == NL80211_CMD_SET_PMKSA) &&
> > + (!info->attrs[NL80211_ATTR_PMKID]))
> > return -EINVAL;
>
> Maybe it'd be better to split set/del now? The code kind of looks
> awkward now, don't you think?
>
> Or split this part of the parsing depending on set or del?
>
> > - pmksa.pmkid = nla_data(info->attrs[NL80211_ATTR_PMKID]);
> > + if (info->attrs[NL80211_ATTR_PMKID])
> > + pmksa.pmkid = nla_data(info->attrs[NL80211_ATTR_PMKID]);
> >
> > if (info->attrs[NL80211_ATTR_MAC]) {
> > pmksa.bssid = nla_data(info->attrs[NL80211_ATTR_MAC]);
> > - } else if (info->attrs[NL80211_ATTR_SSID] &&
> > - info->attrs[NL80211_ATTR_FILS_CACHE_ID] &&
> > - (info->genlhdr->cmd == NL80211_CMD_DEL_PMKSA ||
> > + } else if (info->attrs[NL80211_ATTR_SSID]) {
> > + /* SSID based pmksa flush suppported only for FILS,
> > + * OWE/SAE OFFLOAD cases
> > + */
> > + if (info->attrs[NL80211_ATTR_FILS_CACHE_ID] &&
> > + (info->genlhdr->cmd == NL80211_CMD_DEL_PMKSA ||
> > info->attrs[NL80211_ATTR_PMK])) {
> > + pmksa.cache_id =
> > + nla_data(info->attrs[NL80211_ATTR_FILS_CACHE_ID]);
> > + } else if ((info->genlhdr->cmd == NL80211_CMD_DEL_PMKSA) &&
> > + (!wiphy_ext_feature_isset(
> > + &rdev->wiphy, NL80211_EXT_FEATURE_SAE_OFFLOAD) &&
> > + (!wiphy_ext_feature_isset(
> > + &rdev->wiphy,NL80211_EXT_FEATURE_OWE_OFFLOAD)))){
>
>
> The indentation here is also really awful ... I'd rather go over 80
> columns than break like that. But you could just have local variables
> for all the feature checks too.
>
> And if you don't split set/del, I'd recommend a variable for that too,
> set at the beginning, perhaps moving the "rdev_ops" thing up? But I'm
> thinking it's probably nicer to split it. See how that looks like?
>
> > + return -EINVAL;
> > + }
> > pmksa.ssid = nla_data(info->attrs[NL80211_ATTR_SSID]);
> > pmksa.ssid_len = nla_len(info->attrs[NL80211_ATTR_SSID]);
> > - pmksa.cache_id =
> > - nla_data(info->attrs[NL80211_ATTR_FILS_CACHE_ID]);
> > } else {
> > return -EINVAL;
> > }
> > +
> > if (info->attrs[NL80211_ATTR_PMK]) {
>
> Then again, that isn't even relevant for DEL, is it? Should we even
> parse it? Does anyone want to "delete only if it's exactly this PMK"?
>
>
> Also seems like this should come with some nl80211.h updates though?
>
> johannes