[PATCH wireless-next 4/5] wifi: cfg80211: Fragment per-link station stats in nl80211_dump_station()

[Praneesh P]

On 6/8/2026 1:12 PM, Johannes Berg wrote:
> On Sun, 2026-06-07 at 23:29 +0530, P Praneesh wrote:
>>   
>> +static int nl80211_fill_link_station(struct sk_buff *msg,
>> +				     struct cfg80211_registered_device *rdev,
>> +				     struct link_station_info *link_sinfo)
>> +{
>> +	struct nlattr *bss_param, *link_sinfoattr;
>> +
>> +#define PUT_LINK_SINFO(attr, memb, type) do {				\
>> +	BUILD_BUG_ON(sizeof(type) == sizeof(u64));			\
>> +	if (link_sinfo->filled & BIT_ULL(NL80211_STA_INFO_ ## attr) &&	\
>> +	    nla_put_ ## type(msg, NL80211_STA_INFO_ ## attr,		\
>> +			     link_sinfo->memb))				\
>> +		goto nla_put_failure;					\
>> +	} while (0)
>> +#define PUT_LINK_SINFO_U64(attr, memb) do {				\
>> +	if (link_sinfo->filled & BIT_ULL(NL80211_STA_INFO_ ## attr) &&	\
>> +	    nla_put_u64_64bit(msg, NL80211_STA_INFO_ ## attr,		\
>> +			      link_sinfo->memb, NL80211_STA_INFO_PAD))	\
>> +		goto nla_put_failure;					\
>> +	} while (0)
>> +
>> +	link_sinfoattr = nla_nest_start_noflag(msg, NL80211_ATTR_STA_INFO);
> See previous note about _noflag() in all of this code - that shouldn't
> be there.
Noted, I will address it in next version.
>> +static int nl80211_send_link_station(struct sk_buff *msg,
>> +				     struct netlink_callback *cb,
>> +				     struct cfg80211_registered_device *rdev,
>> +				     struct wireless_dev *wdev,
>> +				     const u8 *mac_addr,
>> +				     struct station_info *sinfo,
>> +				     int link_idx)
>> +{
>> +	void *hdr;
>> +	struct nlattr *links, *link;
>> +	struct link_station_info *link_sinfo;
>> +	struct nlattr *sinfoattr;
>> +	int err;
>> +
>> +	hdr = nl80211hdr_put(msg, NETLINK_CB(cb->skb).portid,
>> +			     cb->nlh->nlmsg_seq, NLM_F_MULTI,
>> +			     NL80211_CMD_NEW_STATION);
>> +	if (!hdr)
>> +		return -EMSGSIZE;
>> +
>> +	if ((wdev->netdev &&
>> +	     nla_put_u32(msg, NL80211_ATTR_IFINDEX, wdev->netdev->ifindex)) ||
>> +	    nla_put_u64_64bit(msg, NL80211_ATTR_WDEV, wdev_id(wdev),
>> +			      NL80211_ATTR_PAD) ||
>> +	    nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, mac_addr) ||
>> +	    nla_put_u32(msg, NL80211_ATTR_GENERATION, sinfo->generation)) {
>> +		err = -EMSGSIZE;
>> +		goto err_cancel;
>> +	}
>> +
>> +	sinfoattr = nla_nest_start_noflag(msg, NL80211_ATTR_STA_INFO);
>> +	if (!sinfoattr) {
>> +		err = -EMSGSIZE;
>> +		goto err_cancel;
>> +	}
>> +
>> +	link_sinfo = sinfo->links[link_idx];
>> +	if (!link_sinfo) {
>> +		err = -ENOENT;
>> +		goto err_cancel;
>> +	}
>> +
>> +	nla_nest_end(msg, sinfoattr);
>> +	if (!is_valid_ether_addr(link_sinfo->addr)) {
>> +		err = -EADDRNOTAVAIL;
>> +		goto err_cancel;
> That seems really odd? why even bother going into the whole thing if
> it's invalid? Also, doesn't that ENOENT get propagated all the way and
> it aborts? I guess it does but it should never happen because of
> valid_links? Still seems a bit odd.
Both the link_sinfo null check and is_valid_ether_addr() check
will be moved to the top of the function, before any header or
nest construction. The null check for sinfo->links[link_idx] is
already guarded upstream by the valid_links bitmask so it should
never be NULL in practice, I'll add a WARN_ON_ONCE and skip rather
than returning -ENOENT, which would otherwise abort the entire dump.
Will it be okay ?
>> @@ -8354,13 +8616,22 @@ static int nl80211_dump_station(struct sk_buff *skb,
>>   	struct wireless_dev *wdev;
>>   	struct nl80211_dump_station_cb *cb_data = (void *)cb->ctx;
>>   	struct nl80211_dump_station_ctx *ctx = cb_data->ctx;
>> +	struct nlattr **attrbuf = NULL;
>>   	int err, ret;
>>   
>>   	NL_ASSERT_CTX_FITS(struct nl80211_dump_station_cb);
>>   
>> -	err = nl80211_prepare_wdev_dump(cb, &rdev, &wdev, NULL);
>> -	if (err)
>> +	if (!ctx) {
>> +		attrbuf = kzalloc_objs(*attrbuf, NUM_NL80211_ATTR);
>> +		if (!attrbuf)
>> +			return -ENOMEM;
>> +	}
>> +
>> +	err = nl80211_prepare_wdev_dump(cb, &rdev, &wdev, attrbuf);
>> +	if (err) {
>> +		kfree(attrbuf);
>>   		return err;
>> +	}
>>   
>>   	/* nl80211_prepare_wdev_dump acquired it in the successful case */
>>   	__acquire(&rdev->wiphy.mtx);
>> @@ -8369,15 +8640,22 @@ static int nl80211_dump_station(struct sk_buff *skb,
>>   	if (!ctx) {
>>   		ctx = kzalloc_obj(*ctx);
>>   		if (!ctx) {
>> +			kfree(attrbuf);
> perhaps better to __free(kfree) instead of doing all these kfree()
> calls?
Yes, will use __free(kfree) for attrbuf.
>> -	/* Phase 0: dump aggregated station info */
>> -	if (ctx->phase == NL80211_DUMP_STA_PHASE_AGGREGATED) {
>> -		while (true) {
>> +	while (true) {
>> +		/* Phase 0: dump aggregated station info */
>> +		if (ctx->phase == NL80211_DUMP_STA_PHASE_AGGREGATED) {
> changing it now also kind of argues for not having it in the first patch
> in the first place ;-)
Noted, the loop restructuring will be moved entirely into
patch 4, not split across patches 3 and 4.
>>   			memset(&ctx->sinfo, 0, sizeof(ctx->sinfo));
>>   			for (int i = 0; i < IEEE80211_MLD_MAX_NUM_LINKS; i++) {
>>   				ctx->sinfo.links[i] =
>> @@ -8428,15 +8706,45 @@ static int nl80211_dump_station(struct sk_buff *skb,
>>   				goto out_err_release;
>>   			}
>>   
>> -			/* Reset ctx for next station */
>> -			cfg80211_sinfo_release_content(&ctx->sinfo);
>> -			ctx->sta_idx++;
>> +			ctx->phase = NL80211_DUMP_STA_PHASE_PER_LINK;
>>   		}
>> -	}
>>   
>> -	ctx->phase = NL80211_DUMP_STA_PHASE_AGGREGATED;
>> -	err = skb->len;
>> -	goto out_err;
>> +		/* Phase 1: dump per-link station info */
>> +		if (ctx->phase == NL80211_DUMP_STA_PHASE_PER_LINK &&
>> +		    ctx->dump_link_stats && ctx->sinfo.valid_links) {
>> +			while (ctx->link_idx < IEEE80211_MLD_MAX_NUM_LINKS) {
>> +				if (!(ctx->sinfo.valid_links &
>> +				      BIT(ctx->link_idx))) {
>> +					ctx->link_idx++;
>> +					continue;
>> +				}
>> +
>> +				ret = nl80211_send_link_station(skb, cb, rdev,
>> +								wdev,
>> +								ctx->mac_addr,
>> +								&ctx->sinfo,
>> +								ctx->link_idx);
>>
> I think conceptually this code is structured a bit strangely. It would
> seem a lot simpler to me to do something like
>
>
> 	// not literally such a macro, just the while (true) loop
> 	for_each_station_starting_from_idx(...) {
> 		if (ctx->phase == 0) {
> 			// fill sinfo etc.
> 		}
>
> 		// common stuff
> 		nl80211hdr_put()
> 		nla_put_u32(msg, NL80211_ATTR_IFINDEX...)
> 		nla_put_u64_64bit(msg, NL80211_ATTR_WDEV..)
> 		nla_put(msg, NL80211_ATTR_MAC...)
> 		nla_put_u32(msg, NL80211_ATTR_GENERATION...)
>
> 		switch (phase) {
> 		case 0:
> 			nl80211_put_sta_info_common(...);
> 			phase++;
> 			ctx->link_id = 0;
> 			break;
> 		case 1:
> 			if (!multi-phase-requested)
> 				break;
> 			nl80211_put_link_station(..., ctx->link_id);
> 			ctx->link_id++;
> 			if (link_id == NUM_LINKS)
> 				ctx->phase = 0; // next sta
> 			break;
> 		}
> 	}
>
>
> or something like that, more like we handle wiphy dump with all the
> messages, for example?
>
> I think it'd be better to have just one place that creates the header
> and all the common info.
>
> But maybe it's just my brain pathways being trained for that scheme.
>
> johannes
ok, I will try to align with this suggested model. I'll refactor so the 
outer loop
produces exactly one netlink message per iteration, with header
construction in one place:

while (true) {
     if (ctx->phase == PHASE_AGGREGATED) {
         /* fetch sinfo from driver once per station */
         memset(&ctx->sinfo, ...);
         /* allocate links[], call rdev_dump_station() */
         /* apply cfg80211_sta_set_mld_sinfo() if needed */
     }

     hdr = nl80211hdr_put(msg, ...);
     /* common attrs: ifindex, wdev, mac, generation */
     nla_put_u32(msg, NL80211_ATTR_IFINDEX, ...);
     nla_put_u64_64bit(msg, NL80211_ATTR_WDEV, ...);
     nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, ctx->mac_addr);
     nla_put_u32(msg, NL80211_ATTR_GENERATION, ...);

     switch (ctx->phase) {
     case PHASE_AGGREGATED:
         nl80211_put_sta_info_common(msg, rdev, &ctx->sinfo);
         ctx->phase = PHASE_PER_LINK;
         ctx->link_idx = first_valid_link();
         break;
     case PHASE_PER_LINK:
         nl80211_put_link_station(msg, rdev, &ctx->sinfo, ctx->link_idx);
         advance ctx->link_idx to next valid link;
         if (no more links) {
             cfg80211_sinfo_release_content(&ctx->sinfo);
             ctx->sta_idx++;
             ctx->phase = PHASE_AGGREGATED;
         }
         break;
     }

     genlmsg_end(msg, hdr);   /* or genlmsg_cancel() on EMSGSIZE 
break-out */
}


This eliminates the duplicated nla_put_u32/u64/MAC header blocks
that currently exist in both nl80211_send_accumulated_station()
and nl80211_send_link_station(), and makes the EMSGSIZE bail-out
uniform since we return before committing the message.

should the rdev_dump_station() / sinfo fill stay outside the
per-message loop (as a separate if (phase == AGGREGATED) guard
at the top), or is it preferable to fold it into the switch
case? I've sketched the former above since fetching sinfo is
logically per-station, not per-message. I'm also ok to adjust
if you'd prefer it inside the switch ?

- Praneesh