Hi Aloka, Sorry for the long delay. > Reported-by: kernel test robot <lkp@xxxxxxxxx> > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> Please drop those, you can list them in the version annotations, but they didn't really report the feature or something :) > + * @mbssid.max_interfaces: maximum number of interfaces supported by the driver > + * in a multiple BSSID set that can ben configured using > + * %NL80211_MBSSID_CONFIG_ATTR_COUNT. I don't quite understand why you need this as well as NL80211_EXT_FEATURE_MBSSID_AP? Isn't it quite clear that if you have mbssid.max_interfaces then it must support MBSSID_AP, and vice versa? Also, I don't think I am happy with the code that lets you advertise NL80211_EXT_FEATURE_MBSSID_AP and not have a max_interfaces, and then the kernel just accepts an arbitrary number - if that's the case, why bother advertising max_interfaces at all? IOW, I think you should drop NL80211_EXT_FEATURE_MBSSID_AP and force having max_interfaces. > +/** > + * enum nl80211_mbssid_config_attributes - Attributes to configure/advertise > + * multiple BSSID and enhanced multi-BSSID advertisements in AP mode. > + * > + * @__NL80211_MBSSID_CONFIG_ATTR_INVALID: Invalid > + * > + * @NL80211_MBSSID_CONFIG_ATTR_PARENT: For a non-transmitted BSSID, this > + * attribute provides the interface index (u32) of the transmitted profile. > + * Must match one of the interface indices advertised by the kernel. > + * Mandatory parameter for non-transmitting interfaces, ignored for the > + * transmitting interface. > + * > + * @NL80211_MBSSID_CONFIG_ATTR_COUNT: Total number of BSSs (u8) in the > + * multiple BSSID set. Mandatory attribute with minimum value of 1. I have a feeling we've discussed this before and I just forgot - but why do we need the count at all? Shouldn't that be obvious by the various values of ..._CONFIG_ATTR_INDEX? What happens if you call it with * index=1, count=3 * index=2, count=3 * index=3, count=4 for three non-transmitting interfaces? That would cause ... issues? But that sort of means the drivers cannot rely on the 'count' anyway, unless it's somehow only set up on the *first* (transmitting) interface, or something like that, and then validated later? But that means cfg80211 cannot do the validation, and so what's the point? What is it needed for anyway? Couldn't you just take the maximum index plus 1 as the count? Somehow we've got to be keeping track of all of that anyhow. Perhaps we should actually do _more_ tracking in cfg80211 and link it to the parent, and then track that the index space is allocated contiguously? Also, regarding the count, what if you never reach it, because you only configure * index=1, count=4 * index=2, count=4 and that's it - seems like the count thing just causes confusion and tracking it internally makes more sense? mac80211 also never even uses the count, afaict. > + * @NL80211_MBSSID_CONFIG_ATTR_INDEX: The index of this BSS (u8) in the > + * multiple BSSID set. Mandatory parameter for non-transmitting interfaces, > + * ignored for the transmitting interface. > + * Range: 1 to %NL80211_MBSSID_CONFIG_ATTR_COUNT-1. 0 is for the transmitting interface then? > + * @NL80211_MBSSID_CONFIG_ATTR_EMA: Userspace and the drivers should use this > + * flag to indicate if enhanced multi-BSSID advertisements (EMA AP) feature > + * is supported/enabled. When set, beacons are generated as per EMA rules, > + * see %nl80211_mbssid_elems_attributes for details. Drivers must set > + * NL80211_EXT_FEATURE_EMA_AP for this flag to be used. I guess 'used' -> 'permitted' or something. > + * @NL80211_MBSSID_ELEMS_ATTR_COUNT: Number of multiple BSSID elements (u8). > + * This attribute is also used to advertise the maximum profile periodicity > + * supported by the driver if EMA is enabled. This one I see even less value in than the count above - isn't that simply counting the number of entries in ...MBSSID_ELEMS_ATTR_DATA? FWIW, your code is also quite happy to let userspace crash the kernel if it specifies _COUNT=1 and then has 100 entries in _DATA, as far as I can tell. Again, I think you should remove this and just count the entries in ..._ATTR_DATA first. > +static const struct nla_policy > +nl80211_mbssid_elems_policy[NL80211_MBSSID_ELEMS_ATTR_MAX + 1] = { > + [NL80211_MBSSID_ELEMS_ATTR_COUNT] = { .type = NLA_U8 }, > + [NL80211_MBSSID_ELEMS_ATTR_DATA] = { .type = NLA_NESTED }, Some of this could be more explicit, e.g. if we keep COUNT despite the discussion above, the policy should specify a min value of 1. Same is probably true elsewhere. > +static int nl80211_put_mbssid_support(struct wiphy *wiphy, > + struct sk_buff *msg) > +{ > + struct nlattr *config = NULL, *elems = NULL; You don't need that - you can move those into the ifs > +fail: > + if (elems) > + nla_nest_cancel(msg, elems); > + if (config) > + nla_nest_cancel(msg, config); because you don't need that > +static int nl80211_parse_mbssid_config(struct wiphy *wiphy, > + struct net_device *dev, > + struct nlattr *attrs, u8 num_elems, > + struct cfg80211_mbssid_config *config) > +{ > + struct nlattr *tb[NL80211_MBSSID_CONFIG_ATTR_MAX + 1]; > + struct net_device *parent; > + > + if (!wiphy_ext_feature_isset(wiphy, NL80211_EXT_FEATURE_MBSSID_AP)) > + return -EOPNOTSUPP; > + > + if (nla_parse_nested(tb, NL80211_MBSSID_CONFIG_ATTR_MAX, attrs, NULL, > + NULL) || !tb[NL80211_MBSSID_CONFIG_ATTR_COUNT]) please reformat > + return -EINVAL; > + > + config->ema = nla_get_flag(tb[NL80211_MBSSID_CONFIG_ATTR_EMA]); > + if (config->ema) { > + if (!wiphy_ext_feature_isset(wiphy, NL80211_EXT_FEATURE_EMA_AP)) > + return -EOPNOTSUPP; > + > + /* nl80211_parse_mbssid_elems() sets this value */ > + if (!num_elems || (wiphy->mbssid.max_periodicity && > + num_elems > wiphy->mbssid.max_periodicity)) the !num_elems check should be moved to the policy, and I think mbssid.max_periodicity should be required > + config->count = nla_get_u8(tb[NL80211_MBSSID_CONFIG_ATTR_COUNT]); > + if (!config->count || (wiphy->mbssid.max_interfaces && > + config->count > wiphy->mbssid.max_interfaces)) > + return -EINVAL; same with max_interfaces imho. > + > + if (tb[NL80211_MBSSID_CONFIG_ATTR_PARENT]) { > + config->parent = nla_get_u32(tb[NL80211_MBSSID_CONFIG_ATTR_PARENT]); > + parent = __dev_get_by_index(wiphy_net(wiphy), config->parent); > + if (!parent || !parent->ieee80211_ptr) > + return -EINVAL; > + > + if (parent == dev) > + config->parent = 0; > + } > + > + if (tb[NL80211_MBSSID_CONFIG_ATTR_INDEX] && config->parent) { > + config->index = nla_get_u8(tb[NL80211_MBSSID_CONFIG_ATTR_INDEX]); > + if (!config->index || config->index >= config->count) policy check for the index please > + return -EINVAL; > + } > + > + return 0; > +} > + > +static struct cfg80211_mbssid_elems * > +nl80211_parse_mbssid_elems(struct wiphy *wiphy, struct nlattr *attrs) > +{ > + struct nlattr *nl_elems, *tb[NL80211_MBSSID_ELEMS_ATTR_MAX + 1]; > + struct cfg80211_mbssid_elems *elems = NULL; > + int rem_elems; > + u8 i = 0, num_elems; > + > + if (!wiphy_ext_feature_isset(wiphy, NL80211_EXT_FEATURE_MBSSID_AP)) > + return NULL; > + > + if (nla_parse_nested(tb, NL80211_MBSSID_ELEMS_ATTR_MAX, attrs, NULL, > + NULL) || > + !tb[NL80211_MBSSID_ELEMS_ATTR_COUNT] || > + !tb[NL80211_MBSSID_ELEMS_ATTR_DATA]) > + return NULL; > + > + num_elems = nla_get_u8(tb[NL80211_MBSSID_ELEMS_ATTR_COUNT]); > + if (!num_elems) > + return NULL; > + > + elems = kzalloc(struct_size(elems, elem, num_elems), GFP_KERNEL); > + if (!elems) > + return NULL; > + > + elems->cnt = num_elems; > + nla_for_each_nested(nl_elems, tb[NL80211_MBSSID_ELEMS_ATTR_DATA], > + rem_elems) { This is the bug I was talking about - num_elems can be much less than the actual number of elements in here. johannes