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

[Johannes Berg]

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.

> +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.

> 
> @@ -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?

> -	/* 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 ;-)

>  			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