I will resend the first patch. Thanks for your insightful comments. I was wondering why every other driver seemed to be allocating "struct station_info" using kzalloc() On Mon, Jun 7, 2021 at 1:46 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote: > > [△EXTERNAL] > > > > 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: > > > 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 <wlooi@xxxxxxxxxxx> > > > --- > > > > > > 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 >
