On Mon, Jun 07, 2021 at 11:35:42AM +0300, Dan Carpenter wrote:

On Sun, Jun 06, 2021 at 11:46:38AM -0700, Wenli Looi wrote:

quoted

Uninitialized struct with invalid pointer causes BUG and prevents access
point from working. Access point works once I apply this patch.

This problem seems to have been present from the time the driver was
added to staging. Most users probably do not use access point so they
will not encounter this bug.

https://forum.armbian.com/topic/14727-wifi-ap-kernel-bug-in-kernel-5444/
has more details.

kzalloc() seems to be what other drivers are doing in the same situation
of creating struct station_info and calling cfg80211_new_sta.  In
particular, other drivers like ath6kl and mwifiex will silently return
when kzalloc fails, so this seems like the right behavior. (mwifiex
returns -ENOMEM from the place kzalloc is called, but if you follow the
chain of calls, the return value is ultimately ignored)

Links to same situation in other drivers:
https://github.com/torvalds/linux/blob/f5b6eb1e018203913dfefcf6fa988649ad11ad6e/drivers/net/wireless/ath/ath6kl/main.c#L488
https://github.com/torvalds/linux/blob/f5b6eb1e018203913dfefcf6fa988649ad11ad6e/drivers/net/wireless/marvell/mwifiex/uap_event.c#L120

Signed-off-by: Wenli Looi <redacted>
---

v1 -> v2: Switched from large stack variable to kzalloc

Nah, v1 was better, it just needs an updated commit message.  See my
other email for more details.

I read this again and I thought I should provide some more information.

This sinfo struct used to be huge and that's why people used to allocate
it if kzalloc() but now it's only 224 bytes so it's okay to put it on
the stack.

And the problem was never whether the variable was on the stack vs on
the heap so changing that wasn't part of the "a patch should do one
thing."  If you want to change it to kzalloc you have to do that in a
separate patch (don't do that).

And you were reading Greg's questions as saying the patch was wrong, but
actually they were just questions.

regards,
dan carpenter