[PATCH wireless-next 3/5] wifi: cfg80211: Refactor nl80211_dump_station() to prepare for per-link stats

[Praneesh P]

On 6/8/2026 1:00 PM, Johannes Berg wrote:
> On Sun, 2026-06-07 at 23:29 +0530, P Praneesh wrote:
>> +++ b/net/wireless/nl80211.c
>> @@ -39,6 +39,39 @@ static int nl80211_crypto_settings(struct cfg80211_registered_device *rdev,
>>   
>>   /* the netlink family */
>>   static struct genl_family nl80211_fam;
>> +/**
>> + * enum nl80211_dump_station_phase - station dump fragmentation phases
>> + * @NL80211_DUMP_STA_PHASE_AGGREGATED: send aggregated (MLO-combined) station
>> + *	statistics for the station entry
>> + * @NL80211_DUMP_STA_PHASE_PER_LINK: send per-link statistics for each active
>> + *	MLO link of the station; only used when dump_link_stats is set
>> + */
>> +enum nl80211_dump_station_phase {
>> +	NL80211_DUMP_STA_PHASE_AGGREGATED = 0,
>> +	NL80211_DUMP_STA_PHASE_PER_LINK   = 1,
>> +};
> [snip]
>
> probably nicer to move this stuff to closer to the dump code that needs
> it; at the very least it's missing a blank line.
>
> Also I think the whole 'phase' introduction etc. seems to more belong to
> patch 4 instead of this one? We can still do the structure allocation
> etc. here.
Will add the missing blank line and move both the enum and struct
definitions to sit just above nl80211_dump_station(). Patch 3 will
only introduce the context struct (sta_idx, mac_addr, sinfo) and
the stateful allocation/free mechanism. The phase field and its
enum will move to patch 4 where they are actually used.
>> +struct nl80211_dump_station_ctx {
>> +	int sta_idx;
>> +	enum nl80211_dump_station_phase phase;
>> +	u8 mac_addr[ETH_ALEN];
>> +	struct station_info sinfo;
>> +};
>> +
>> +/**
>> + * struct nl80211_dump_station_cb - state stored in netlink_callback::ctx
>> + * @wiphy_idx: args[0] - wiphy index from nl80211_prepare_wdev_dump
>> + * @wdev_id: args[1] - wdev identifier from nl80211_prepare_wdev_dump
>> + * @ctx: args[2] - dump station context pointer
>> + *
>> + * args[0] and args[1] are reserved by nl80211_prepare_wdev_dump().
>> + * The ctx pointer must live at args[2] to avoid corrupting those fields.
>> + */
>> +struct nl80211_dump_station_cb {
>> +	long wiphy_idx;
>> +	long wdev_id;
>> +	struct nl80211_dump_station_ctx *ctx;
>> +};
> I'm not sure I'm happy with this - better to just use args[2] directly
> for a pointer to struct nl80211_dump_station_ctx?
>
>>   static int nl80211_dump_station(struct sk_buff *skb,
>>   				struct netlink_callback *cb)
>>   {
>> -	struct station_info sinfo;
>>   	struct cfg80211_registered_device *rdev;
>>   	struct wireless_dev *wdev;
>> -	u8 mac_addr[ETH_ALEN];
>> -	int sta_idx = cb->args[2];
>> -	bool sinfo_alloc = false;
>> -	int err, i;
>> +	struct nl80211_dump_station_cb *cb_data = (void *)cb->ctx;
>> +	struct nl80211_dump_station_ctx *ctx = cb_data->ctx;
> This doesn't really seem better than just doing
>
> 	struct nl80211_dump_station_ctx *ctx = cb->args[2];
>
> given the overlay with generic code.
>
> Alternatively we could change the _whole_ nl80211 code (as a separate
> commit, perhaps even converting other things later) to have
>
> struct nl80211_dump_cb {
> 	long wiphy_idx;
> 	long wdev_id;
> 	union {
> 		long wiphy_filter;
> 		int mpath_idx;
> 		int reg_idx;
> 		int bss_idx;
> 		int survey_idx;
> 		//...
>
> 		// and later add
> 		struct nl80211_dump_station_ctx *sta_ctx;
> 	};
> };
>
> or so, and then we don't have the args[0]/... problem at all.
>
> But I'm not convinced mixing the structs with "must have this type at
> this offset" etc. is a good idea if it's going to stay this way.
Ok, I'll drop nl80211_dump_station_cb entirely and use:

struct nl80211_dump_station_ctx *ctx = (void *)cb->args[2];

directly, mirroring nl80211_dump_wiphy's pattern of casting
cb->args[0] to its state pointer. The cb->args[0]/[1] contract
is already established by nl80211_prepare_wdev_dump(), no
overlay struct is needed to document it.
>> +	/* Phase 0: dump aggregated station info */
>> +	if (ctx->phase == NL80211_DUMP_STA_PHASE_AGGREGATED) {
>> +		while (true) {
>> +			memset(&ctx->sinfo, 0, sizeof(ctx->sinfo));
>>
> if you have this memset, why do you need this one:
>
>> +out_err_release:
>> +	cfg80211_sinfo_release_content(&ctx->sinfo);
>> +	memset(&ctx->sinfo, 0, sizeof(ctx->sinfo));
> Seems a bit odd?
>
> johannes
The two memsets serve different purposes.

The first one (Phase 0 loop top) is a clean-slate reset before allocating
the per-station sinfo.links[] entries for each iteration.

The second one at out_err_release is a double-free guard. The ctx is
persistent across netlink dump callbacks (stored in cb->ctx), so when the
function returns early via out_err_release, nl80211_dump_station_done()
will eventually be called and it unconditionally invokes
cfg80211_sinfo_release_content() again:

if (ctx) {
   cfg80211_sinfo_release_content(&ctx->sinfo);
   kfree(ctx);
}

Without the memset at out_err_release, sinfo.links[] would still hold the
already-freed pointers when _done() runs, resulting in a double-free.
The memset nullifies those pointers so the second
cfg80211_sinfo_release_content() call in _done() hits
kfree(NULL), which is harmless.