[PATCH] brcm80211: smac: eliminate a null pointer dereference in dma.c

[PATCH] brcm80211: smac: eliminate a null pointer dereference in dma.c

[Arend van Spriel]

Though it's unlikely, di may be null, so we can't dereference
di->dma.dmactrlflags until we've checked it.

Move this de-reference after the check, and adjust the error
message to not require de-referencing di.

This is based upon Julia's original patch:
<1319846297-2985-2-git-send-email-julia@diku.dk>

Reported-by: Julia Lawall <julia@diku.dk>
Acked-by: Arend van Spriel <arend@broadcom.com>
Signed-off-by: Julian Calaby <julian.calaby@gmail.com>
---
 drivers/net/wireless/brcm80211/brcmsmac/dma.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmsmac/dma.c b/drivers/net/wireless/brcm80211/brcmsmac/dma.c
index ae541fb..e286fb4 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/dma.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/dma.c
@@ -358,13 +358,14 @@ static uint nrxdactive(struct dma_info *di, uint h, uint t)
 
 static uint _dma_ctrlflags(struct dma_info *di, uint mask, uint flags)
 {
-	uint dmactrlflags = di->dma.dmactrlflags;
+	uint dmactrlflags;
 
 	if (di == NULL) {
-		DMA_ERROR(("%s: _dma_ctrlflags: NULL dma handle\n", di->name));
+		DMA_ERROR(("_dma_ctrlflags: NULL dma handle\n"));
 		return 0;
 	}
 
+	dmactrlflags = di->dma.dmactrlflags;
 	dmactrlflags &= ~mask;
 	dmactrlflags |= flags;
 
-- 
1.7.4.1

Re: [PATCH] brcm80211: smac: eliminate a null pointer dereference in dma.c

[Dominique Martinet]

Hi,


Arend van Spriel wrote on Sat, Oct 29, 2011:
> Though it's unlikely, di may be null, so we can't dereference
> di->dma.dmactrlflags until we've checked it.

I've got no experience whatsoever in kernel coding so feel free to tell
me that's stupid, but if you say it's unlikely why not use the
unlikely() macro as well for the check?
(also, that's not the point of this patch, but can't hurt to point that
out)


>  	if (di == NULL) {
would change to
if (unlikely(di == NULL)) {


Regards,
-- 
Asmadeus | Dominique Martinet

Re: [PATCH] brcm80211: smac: eliminate a null pointer dereference in dma.c

[Julian Calaby]

Hi,

On Sat, Oct 29, 2011 at 21:25, Dominique Martinet
<asmadeus@codewreck.org> wrote:
> Hi,
>
>
> Arend van Spriel wrote on Sat, Oct 29, 2011:
>> Though it's unlikely, di may be null, so we can't dereference
>> di->dma.dmactrlflags until we've checked it.
>
> I've got no experience whatsoever in kernel coding so feel free to tell
> me that's stupid, but if you say it's unlikely why not use the
> unlikely() macro as well for the check?

I'm not sure the check for whether di is NULL is necessary at all, as
my very quick check of where this function is called from turned up
only one place that it's called from, where di is most definitely not
null.

As for using unlikely() it produces an instruction for the compiler to
mangle the if statement into assembler in a certain way and, as I
understand it, is mostly used where we *know* that:
1. The test is unlikely to be true and
2. Telling the compiler to write the assembly code like this will
produce a performance improvement.

I.e. a good place to stick an unlikely is in a "fast-path", e.g. the
packet receive code - if we don't process packets fast enough, the
hardware will start dropping them, so a performance improvement of a
couple of cycles - which is what unlikely() will give us - may visibly
improve performance.

That said, there are a couple of reasons why we shouldn't:
- Compilers are getting smarter all the time, and can probably figure
out that we're unlikely to hit the test often
- Premature optimisation is considered harmful.
- If, as I suspect, this function is only used in one location, the
compiler will probably inline the function, then optimise it from
there - the only caller I could find calls the function with a proper
value for di, so the compiler will probably drop the entire if
statement.

I'm sure that there are many other things that could be worked on in
this driver before we start worrying about whether we should stick
unlikely()s in the code.

> (also, that's not the point of this patch, but can't hurt to point that
> out)

This is what I spend my life doing - this patch was just another
instance of this =)

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/