Re: [PATCH v2] staging: rtl8723bs: Fix uninitialized variable

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Wenli Looi <wlooi@ucalgary.ca>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
	"Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Subject: Re: [PATCH v2] staging: rtl8723bs: Fix uninitialized variable
Date: Mon, 7 Jun 2021 11:46:42 +0300	[thread overview]
Message-ID: <20210607084642.GQ1955@kadam> (raw)
In-Reply-To: <20210607083542.GP1955@kadam>

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@ucalgary.ca>
> > ---
> > 
> > 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