[PATCH] b43: Fix smatch warning

[PATCH] b43: Fix smatch warning

[Larry Finger]

  CHECK   drivers/net/wireless/b43/main.c
drivers/net/wireless/b43/main.c +4115 b43_wireless_core_stop(7) warn: variable dereferenced before check 'dev'

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---

John,

This is next material.

Larry
---

 drivers/net/wireless/b43/main.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
index 0628901..2680684 100644
--- a/drivers/net/wireless/b43/main.c
+++ b/drivers/net/wireless/b43/main.c
@@ -4131,13 +4131,14 @@ out_unlock:
  * because the core might be gone away while we unlocked the mutex. */
 static struct b43_wldev * b43_wireless_core_stop(struct b43_wldev *dev)
 {
-	struct b43_wl *wl = dev->wl;
+	struct b43_wl *wl;
 	struct b43_wldev *orig_dev;
 	u32 mask;
 
 redo:
 	if (!dev || b43_status(dev) < B43_STAT_STARTED)
 		return dev;
+	wl = dev->wl;
 
 	/* Cancel work. Unlock to avoid deadlocks. */
 	mutex_unlock(&wl->mutex);
-- 
1.7.3.4

Re: [PATCH] b43: Fix smatch warning

[Larry Finger]

On 08/27/2011 01:54 PM, Michael Büsch wrote:
> On Sat, 27 Aug 2011 13:26:00 -0500
> Larry Finger<Larry.Finger@lwfinger.net>  wrote:
>
>>    CHECK   drivers/net/wireless/b43/main.c
>> drivers/net/wireless/b43/main.c +4115 b43_wireless_core_stop(7) warn: variable dereferenced before check 'dev'
>
>> This is next material.
>
> -next, just because the bug is not in the current kernel?
> Or because you think this is harmless?
>
> I'm not sure whether this is harmless. It effectively is that kind
> of bug that triggers dangerous compiler optimizations.
> I think the compiler has some freedom to assume dev can not be NULL when
> the function is entered and thus optimize out the !dev check.

No, the check has to be left in due to the changing of dev in the routine 
followed by a 'goto redo'.

According to 'git blame', the commit that added the part that swatch does not 
like was

36dbd954 (Michael Buesch 2009-09-04 22:51:29 +0200 4115)

That was when the branch back to redo was added. As your comment says

  * Returns the current dev. This might be different from the passed in dev,
  * because the core might be gone away while we unlocked the mutex. */

In fact, a bug in the original code is unlikely, but my patch did add one. I 
think the code should be

	if (!dev)
		return NULL;
	wl = dev->wl;
redo:

That should work correctly and satisfy swatch. Would that do too much damage to 
the compiler's optimization?

Larry

Re: [PATCH] b43: Fix smatch warning

[Michael Büsch]

On Sat, 27 Aug 2011 14:15:59 -0500
Larry Finger <Larry.Finger@lwfinger.net> wrote:

> On 08/27/2011 01:54 PM, Michael Büsch wrote:
> > On Sat, 27 Aug 2011 13:26:00 -0500
> > Larry Finger<Larry.Finger@lwfinger.net>  wrote:
> >
> >>    CHECK   drivers/net/wireless/b43/main.c
> >> drivers/net/wireless/b43/main.c +4115 b43_wireless_core_stop(7) warn: variable dereferenced before check 'dev'
> >
> >> This is next material.
> >
> > -next, just because the bug is not in the current kernel?
> > Or because you think this is harmless?
> >
> > I'm not sure whether this is harmless. It effectively is that kind
> > of bug that triggers dangerous compiler optimizations.
> > I think the compiler has some freedom to assume dev can not be NULL when
> > the function is entered and thus optimize out the !dev check.
> 
> No, the check has to be left in due to the changing of dev in the routine 
> followed by a 'goto redo'.

I think the compiler is still free to emit the !dev check twice.
Or the other way around: Emit it correctly for the "goto" case and
don't emit it for the function-enter case (and directly check dev->status).

> In fact, a bug in the original code is unlikely, but my patch did add one. I 
> think the code should be
> 
> 	if (!dev)
> 		return NULL;
> 	wl = dev->wl;
> redo:

yes


-- 
Greetings, Michael.