[PATCH] Staging: wlan-ng: p80211netdev.c cleanup

[PATCH] Staging: wlan-ng: p80211netdev.c cleanup

[Nils Radtke]

Effected preliminary cleanup lead by the idea of kerneljanitor.org .
As recommended I'm asking for approval of the location of cleanup
and the manner it happened to know I'm heading in the right
direction.

Compiles. Not tested.

Signed-off-by: Nils Radtke <lkml@Think-Future.com>
---
 drivers/staging/wlan-ng/p80211netdev.c |   50 +++++++++++++++++++-------------
 1 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/wlan-ng/p80211netdev.c b/drivers/staging/wlan-ng/p80211netdev.c
index aa1792c..8104e11 100644
--- a/drivers/staging/wlan-ng/p80211netdev.c
+++ b/drivers/staging/wlan-ng/p80211netdev.c
@@ -176,25 +176,30 @@ static struct net_device_stats *p80211knetdev_get_stats(netdevice_t * netdev)
 ----------------------------------------------------------------*/
 static int p80211knetdev_open(netdevice_t *netdev)
 {
-	int result = 0;		/* success */
+	int ret = 0;
 	wlandevice_t *wlandev = netdev->ml_priv;
 
 	/* Check to make sure the MSD is running */
-	if (wlandev->msdstate != WLAN_MSD_RUNNING)
-		return -ENODEV;
+	if (wlandev->msdstate != WLAN_MSD_RUNNING) {
+		ret = -ENODEV;
+		goto end;
+	}
 
 	/* Tell the MSD to open */
-	if (wlandev->open != NULL) {
-		result = wlandev->open(wlandev);
-		if (result = 0) {
-			netif_start_queue(wlandev->netdev);
-			wlandev->state = WLAN_DEVICE_OPEN;
-		}
-	} else {
-		result = -EAGAIN;
+	if (wlandev->open = NULL) {
+		printk(KERN_ERR "Sorry, got wlandev->open = NULL.\n");
+		ret = -EAGAIN;
+		goto end;
 	}
 
-	return result;
+	ret = wlandev->open(wlandev);
+	if (ret) {
+		netif_start_queue(wlandev->netdev);
+		wlandev->state = WLAN_DEVICE_OPEN;
+	}
+
+end:
+	return ret;
 }
 
 /*----------------------------------------------------------------
@@ -211,16 +216,23 @@ static int p80211knetdev_open(netdevice_t *netdev)
 ----------------------------------------------------------------*/
 static int p80211knetdev_stop(netdevice_t *netdev)
 {
-	int result = 0;
+	int ret = -EFAULT;
 	wlandevice_t *wlandev = netdev->ml_priv;
 
-	if (wlandev->close != NULL)
-		result = wlandev->close(wlandev);
+	if (wlandev->close = NULL) {
+		printk(KERN_ERR "Sorry, got wlandev->close = NULL.\n");
+		ret = -EFAULT; /* FIXME: nr: correct return code? */
+		goto end;
+	}
 
-	netif_stop_queue(wlandev->netdev);
-	wlandev->state = WLAN_DEVICE_CLOSED;
+	ret = wlandev->close(wlandev);
+	if (ret) {
+		netif_stop_queue(wlandev->netdev);
+		wlandev->state = WLAN_DEVICE_CLOSED;
+	}
 
-	return result;
+end:
+	return ret;
 }
 
 /*----------------------------------------------------------------
@@ -242,8 +254,6 @@ void p80211netdev_rx(wlandevice_t *wlandev, struct sk_buff *skb)
 	skb_queue_tail(&wlandev->nsd_rxq, skb);
 
 	tasklet_schedule(&wlandev->rx_bh);
-
-	return;
 }
 
 /*----------------------------------------------------------------
-- 
1.7.1

Re: [PATCH] Staging: wlan-ng: p80211netdev.c cleanup

[Dan Carpenter]

On Sat, Sep 18, 2010 at 08:30:52PM +0200, Nils Radtke wrote:
> diff --git a/drivers/staging/wlan-ng/p80211netdev.c b/drivers/staging/wlan-ng/p80211netdev.c
> index aa1792c..8104e11 100644
> --- a/drivers/staging/wlan-ng/p80211netdev.c
> +++ b/drivers/staging/wlan-ng/p80211netdev.c
> @@ -176,25 +176,30 @@ static struct net_device_stats *p80211knetdev_get_stats(netdevice_t * netdev)
>  ----------------------------------------------------------------*/
>  static int p80211knetdev_open(netdevice_t *netdev)
>  {
> -	int result = 0;		/* success */
> +	int ret = 0;

If it was new code then I also would have prefered "ret" I suppose, but
really it's not a good idea to rename things for no reason.

>  	wlandevice_t *wlandev = netdev->ml_priv;
>  
>  	/* Check to make sure the MSD is running */
> -	if (wlandev->msdstate != WLAN_MSD_RUNNING)
> -		return -ENODEV;
> +	if (wlandev->msdstate != WLAN_MSD_RUNNING) {
> +		ret = -ENODEV;
> +		goto end;
> +	}

It's better to just return directly.  This code makes me wonder what
happens after I goto end, but actually nothing happens.  So it's
a little annoying.

>  
>  	/* Tell the MSD to open */
> -	if (wlandev->open != NULL) {
> -		result = wlandev->open(wlandev);
> -		if (result = 0) {
> -			netif_start_queue(wlandev->netdev);
> -			wlandev->state = WLAN_DEVICE_OPEN;
> -		}
> -	} else {
> -		result = -EAGAIN;
> +	if (wlandev->open = NULL) {
> +		printk(KERN_ERR "Sorry, got wlandev->open = NULL.\n");

This printk is not needed.  I haven't looked at this code, but if the
user can trigger this error message then it can be used to flood
/var/log/messages and cause a denial of service attack.

>  
> -	return result;
> +	ret = wlandev->open(wlandev);
> +	if (ret) {

This test is reversed (we return zero on success).  It should be:
	if (ret)
		return ret;
And then the other stuff can be pulled in an indent level.

> +		netif_start_queue(wlandev->netdev);
> +		wlandev->state = WLAN_DEVICE_OPEN;
> +	}
> +
> +end:
> +	return ret;
>  }

The other function had many of the same issues.

The change log for this should have been something like:
	This is a cleanup of p80211knetdev_open().  I rearranged the
	to unify all the return paths so there was just one return
	statement.  I also changed the if statements so I could pull 
	the code in one indent level and simplify things a bit.  And I
	added a couple printk() to the let the user know about errors.

(Except that adding printk()s is a functional change so it goes into a
separate patch instead of hidden inside a "cleanup" patch.  But you get
the idea).

It's always dangerous writing patches when you can't test them. Most of
my patches are one liners because and when people ask me to rewrite them
in a more complicated way I tell them I don't feel comfortable doing
that because I can't test it.

Really though my advice is to start out by fixing bugs instead of
focusing on cleanup.  I've fixed many of the interesting buffer
overflows in main kernel but if you run smatch on staging then there are
a bunch remaining.  Even reading through the false positives is good
practise for reading code.

Here's one the wlan-ng driver that you were working on:

drivers/staging/wlan-ng/prism2fw.c +594 mkpdrlist(9)
	error: buffer overflow 'pda16' 512 <= 512
drivers/staging/wlan-ng/prism2fw.c +634 mkpdrlist(49)
	error: buffer overflow 'pda16' 512 <= 512

   592          curroff = 0;
   593          while (curroff < (HFA384x_PDA_LEN_MAX / 2) &&
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	We check that the array offset is within bounds here

   594                 le16_to_cpu(pda16[curroff + 1]) != HFA384x_PDR_END_OF_PDA) {
                                         ^^^^^^^^^^^
	and then we add one here which puts us out of bounds.

   595                  pda->rec[pda->nrec] = (hfa384x_pdrec_t *) &(pda16[curroff]);
   596  

Fixing overflows is way more interesting than cleanup patches.  So don't
resend this patch.  The original code works and it's not even that ugly.
You are a true hacker and have more awesome things to do.  That's my
advice to newbie hackers.

regards,
dan carpenter

Re: [PATCH] Staging: wlan-ng: p80211netdev.c cleanup

[Nils Radtke]

  Hi Dan,

  First off, thanks for your explanatory comments on 
  the patch. 

  Meanwhile some water went down the river as I switched the 
  country (and the job, well.. at least the research for the
  job is ongoing duty.. Anyone looking for some kernel newbie
  to complement his staff? I'm quite willing switching the 
  country again! :-)

On Sat 2010-09-18 @ 10-41-12PM +0200, Dan Carpenter wrote: 
# On Sat, Sep 18, 2010 at 08:30:52PM +0200, Nils Radtke wrote:
# > diff --git a/drivers/staging/wlan-ng/p80211netdev.c b/drivers/staging/wlan-ng/p80211netdev.c
# > index aa1792c..8104e11 100644
# > --- a/drivers/staging/wlan-ng/p80211netdev.c
# > +++ b/drivers/staging/wlan-ng/p80211netdev.c
# > @@ -176,25 +176,30 @@ static struct net_device_stats *p80211knetdev_get_stats(netdevice_t * netdev)
# >  ----------------------------------------------------------------*/
# >  static int p80211knetdev_open(netdevice_t *netdev)
# >  {
# > -	int result = 0;		/* success */
# > +	int ret = 0;
# 
# If it was new code then I also would have prefered "ret" I suppose, but
# really it's not a good idea to rename things for no reason.
Yes, I agree. This happened out of editing an edited edit of a version 
of the patch.. Finally, I wasn't offended and just left it in.

# >  	wlandevice_t *wlandev = netdev->ml_priv;
# >  
# >  	/* Check to make sure the MSD is running */
# > -	if (wlandev->msdstate != WLAN_MSD_RUNNING)
# > -		return -ENODEV;
# > +	if (wlandev->msdstate != WLAN_MSD_RUNNING) {
# > +		ret = -ENODEV;
# > +		goto end;
# > +	}
# 
# It's better to just return directly.  This code makes me wonder what
# happens after I goto end, but actually nothing happens.  So it's
# a little annoying.
Ok, I understand that. I considered this argument but personally
thought it "cleaner" this way.
Having a look at drivers/staging/wlan-ng/hfa384x_usb.c, a quick grep 
for "goto" reveals a multitude of those tags that only have a "return
result;" statement. In some situations I'd prefer a clean exit point
instead of them distributed all over the fct code. If it's rather
unclean to do a fct using a single exit point then one might as well
adopt another scheme. But then, maybe the code should be worked on
anyway. 

# >  	/* Tell the MSD to open */
# > -	if (wlandev->open != NULL) {
# > -		result = wlandev->open(wlandev);
# > -		if (result = 0) {
# > -			netif_start_queue(wlandev->netdev);
# > -			wlandev->state = WLAN_DEVICE_OPEN;
# > -		}
# > -	} else {
# > -		result = -EAGAIN;
# > +	if (wlandev->open = NULL) {
# > +		printk(KERN_ERR "Sorry, got wlandev->open = NULL.\n");
# This printk is not needed.  I haven't looked at this code, but if the
# user can trigger this error message then it can be used to flood
# /var/log/messages and cause a denial of service attack.
Ok. Out of kernel bounds I used to code in a rather conservative way, 
applying informative msgs where useful and required. Dying dead and
not telling nothing nowhere is a bad habit, imo. 

Maybe the ERR code isn't appropriate. But then, it's an error, we're even
checking for that one. Maybe there's a better way in propagating the where,
why and when. I'm new to this kind of code..

# > -	return result;
# > +	ret = wlandev->open(wlandev);
# > +	if (ret) {
# This test is reversed (we return zero on success).  It should be:
True. My bad. Have been "bashing" my head.. ;)

# 	if (ret)
# 		return ret;
# And then the other stuff can be pulled in an indent level.
# 
# > +		netif_start_queue(wlandev->netdev);
# > +		wlandev->state = WLAN_DEVICE_OPEN;
# > +	}
# > +
# > +end:
# > +	return ret;
# >  }
# 
# The other function had many of the same issues.
# 
# The change log for this should have been something like:
# 	This is a cleanup of p80211knetdev_open().  I rearranged the
# 	to unify all the return paths so there was just one return
# 	statement.  I also changed the if statements so I could pull 
# 	the code in one indent level and simplify things a bit.  And I
# 	added a couple printk() to the let the user know about errors.
Very good idea. Thank you. I was a bit short on that. Probably mostly 
because I intended this patch a training one for feedback about the
code, neglecting the importance of workflow and how to do it properly.

# (Except that adding printk()s is a functional change so it goes into a
# separate patch instead of hidden inside a "cleanup" patch.  But you get
# the idea).
Yes, that seems reasonable.
Even at the expense of a growing patch count for tiny changes. But ok.

# It's always dangerous writing patches when you can't test them. Most of
Agreed. Totally. Otoh, I read some mails on kernelnewbies that were about
patches of code for devices the author couldn't test. His notes were: 
Compiled, not tested.

As I was about getting feedback about my approach on cleaning up I thought
it would be acceptable.

# my patches are one liners because and when people ask me to rewrite them
# in a more complicated way I tell them I don't feel comfortable doing
# that because I can't test it.
Fair enough.

# Really though my advice is to start out by fixing bugs instead of
# focusing on cleanup.  I've fixed many of the interesting buffer
I'd really like to do that. Ignoring however whether I'm yet able to handle
that kind of patch in kernel context. I'm still reading LDD 3rd ed. and 
some other books on the topic, doing exercises. 


Other things I noticed:

- In that same directory there are files with many void fcts featuring a
  return statement. Is this worth of cleaning them up?

- When checking skb for NULL, which one is better? (I suppose the former one)
  skb = NULL or !skb 
  Rewrite occurrences of the other one for cleanup?

- drivers/staging/wlan-ng/hfa384x_usb.c +3249 
  Is skb_put a fct that always succeeds? Should I bother to check the retval?
  Ok, I checked skbuff.h, returns a ptr. This ptr is not important in 
  drivers/staging/wlan-ng/hfa384x_usb.c +3249 it seems. Why is this?

I'd be glad to hear your opinion, thanks in advance.

          Cheers,

                  Nils

Re: [PATCH] Staging: wlan-ng: p80211netdev.c cleanup

[Dan Carpenter]

I don't want to be rude, but it's basically a kernel hacker rule that
after you introduce a bug (and your last cleanup patch did) that you
have to fix a bug to make up for it.  One excellent source of easy bugs
is using static checkers.

I've found a static checker bug for you.

drivers/staging/comedi/drivers/usbdux.c +1488
	usbdux_ao_inttrig(38) warn: inconsistent returns sem:&this_usbduxsub->sem:
	locked (1468) unlocked (1457,1462,1479,1488) 

Here are some questions to help guide you.

1)  Does it look like a bug?
2)  Who introduced the bug and in which commit?
	Use: "git log -p" and "git blame"
	If it's a real bug and you fix it, by deductive reasoning that
	means you are smarter than the person who introduced it.
3)  What is the return code on line 1468?
4)  Is it a special case where the caller handles it differently?
	Use cscope for this.
	make cscope
	Use "cscope -d" for speed.
	Configure vim to use cscope.
	:cs find c inttrig
	^] takes you back a step.
	cscope is an essential kernel hacking tool.
5)  usbdux_ai_inttrig() is basically the same as usbdux_ao_inttrig().
    Does the ai version ever return with a lock held?
6)  What are the implications if we return with the lock held?
7)  How is the bug triggered?
8)  Can the user trigger the bug?
9)  Can a regular user trigger the bug or only root?
	I sometimes have to use google for this to try figure out how
	people set up the permissions.
10) Should this go into the -stable kernel?

You don't have to answer all the questions, just enough to know what the
correct fix is.  You can fix a different bug if you want to...  It
doesn't matter so long as it is at least equal to the bug you
introduced.

regards,
dan carpenter

PS:

> - When checking skb for NULL, which one is better? (I suppose the former one)
>   skb = NULL or !skb 
>   Rewrite occurrences of the other one for cleanup?

You shouldn't even think about changing these.  But in new code then
!skb is better as Al Viro explains in this email:
http://lwn.net/Articles/331593/

Re: [PATCH] Staging: wlan-ng: p80211netdev.c cleanup

[Nils Radtke]

  Hi Dan,

  Thank you for your reply.

# I don't want to be rude, but it's basically a kernel hacker rule that
# after you introduce a bug (and your last cleanup patch did) that you
# have to fix a bug to make up for it.  One excellent source of easy bugs
No offense taken. There are already other mails in 
the pipe to actually do that last "multi"-patch properly. Just felt
to finally give feedback at least.

# is using static checkers.
I just used smatch as you proposed. That's where I started to dive into
some code that I had to understand before continuing "fixing" something else.
And I still try to figure out some pieces (had a big break for the locality 
change too).. More in the upcoming mails. 

# I've found a static checker bug for you.
You've been invaluable already, thank you. I'll try myself one after another. 
Maybe first the bug found by smatch (some offset prob), then this one. Hm, we'll see.

Some answer right away, the rest gets pushed on the todo stack.

# 4)  Is it a special case where the caller handles it differently?
# 	Use cscope for this.
# 	^] takes you back a step.
Good hint, thx. Hitting esc+alt+] is so "expensive" with this kb layout (you risk 
breaking your hand doing that).. ;)

# 	cscope is an essential kernel hacking tool.
Thank you very much for your detailed explanation. I am already familiar 
with cscope, in this case.

# !skb is better as Al Viro explains in this email:
# http://lwn.net/Articles/331593/
Great, thank you!

      Cheers,


            Nils


-- 
:x            Think-Future.com                                    :)
   Yevtushenko has... an ego that can crack crystal at a distance of
   twenty feet.   -- John Cheever 
   

Re: [PATCH] Staging: wlan-ng: p80211netdev.c cleanup

[walter harms]

Nils Radtke schrieb:
> Effected preliminary cleanup lead by the idea of kerneljanitor.org .
> As recommended I'm asking for approval of the location of cleanup
> and the manner it happened to know I'm heading in the right
> direction.
> 
> Compiles. Not tested.
> 
> Signed-off-by: Nils Radtke <lkml@Think-Future.com>
> ---
>  drivers/staging/wlan-ng/p80211netdev.c |   50 +++++++++++++++++++-------------
>  1 files changed, 30 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/staging/wlan-ng/p80211netdev.c b/drivers/staging/wlan-ng/p80211netdev.c
> index aa1792c..8104e11 100644
> --- a/drivers/staging/wlan-ng/p80211netdev.c
> +++ b/drivers/staging/wlan-ng/p80211netdev.c
> @@ -176,25 +176,30 @@ static struct net_device_stats *p80211knetdev_get_stats(netdevice_t * netdev)
>  ----------------------------------------------------------------*/
>  static int p80211knetdev_open(netdevice_t *netdev)
>  {
> -	int result = 0;		/* success */
> +	int ret = 0;
>  	wlandevice_t *wlandev = netdev->ml_priv;
>  
>  	/* Check to make sure the MSD is running */
> -	if (wlandev->msdstate != WLAN_MSD_RUNNING)
> -		return -ENODEV;
> +	if (wlandev->msdstate != WLAN_MSD_RUNNING) {
> +		ret = -ENODEV;
> +		goto end;
> +	}
>  
>  	/* Tell the MSD to open */
> -	if (wlandev->open != NULL) {
> -		result = wlandev->open(wlandev);
> -		if (result = 0) {
> -			netif_start_queue(wlandev->netdev);
> -			wlandev->state = WLAN_DEVICE_OPEN;
> -		}
> -	} else {
> -		result = -EAGAIN;
> +	if (wlandev->open = NULL) {
> +		printk(KERN_ERR "Sorry, got wlandev->open = NULL.\n");
> +		ret = -EAGAIN;
> +		goto end;
>  	}
>  
> -	return result;
> +	ret = wlandev->open(wlandev);
> +	if (ret) {
> +		netif_start_queue(wlandev->netdev);
> +		wlandev->state = WLAN_DEVICE_OPEN;
> +	}
> +
> +end:
> +	return ret;
>  }
>  

unwinding is always good :)





>  /*----------------------------------------------------------------
> @@ -211,16 +216,23 @@ static int p80211knetdev_open(netdevice_t *netdev)
>  ----------------------------------------------------------------*/
>  static int p80211knetdev_stop(netdevice_t *netdev)
>  {
> -	int result = 0;
> +	int ret = -EFAULT;
>  	wlandevice_t *wlandev = netdev->ml_priv;
>  
> -	if (wlandev->close != NULL)
> -		result = wlandev->close(wlandev);
> +	if (wlandev->close = NULL) {
> +		printk(KERN_ERR "Sorry, got wlandev->close = NULL.\n");
> +		ret = -EFAULT; /* FIXME: nr: correct return code? */
> +		goto end;
> +	}
>  
> -	netif_stop_queue(wlandev->netdev);
> -	wlandev->state = WLAN_DEVICE_CLOSED;
> +	ret = wlandev->close(wlandev);
> +	if (ret) {
> +		netif_stop_queue(wlandev->netdev);
> +		wlandev->state = WLAN_DEVICE_CLOSED;
> +	}
>  
> -	return result;
> +end:
> +	return ret;
>  }
>  

but i wonder if the the test for wlandev->open in X_close() is needed
can wlandev->close vanish between open() and stop() ?

I am wondering who to use the error message "Sorry, got wlandev->open = NULL"
is true but what help ? Give the reader a change and add __func__ to figure out
what hit him.

re,
 wh


>  /*----------------------------------------------------------------
> @@ -242,8 +254,6 @@ void p80211netdev_rx(wlandevice_t *wlandev, struct sk_buff *skb)
>  	skb_queue_tail(&wlandev->nsd_rxq, skb);
>  
>  	tasklet_schedule(&wlandev->rx_bh);
> -
> -	return;
>  }
>  
>  /*----------------------------------------------------------------

Re: [PATCH] Staging: wlan-ng: p80211netdev.c cleanup

[Greg KH]

On Sat, Sep 18, 2010 at 08:30:52PM +0200, Nils Radtke wrote:
> Effected preliminary cleanup lead by the idea of kerneljanitor.org .
> As recommended I'm asking for approval of the location of cleanup
> and the manner it happened to know I'm heading in the right
> direction.
> 
> Compiles. Not tested.
> 
> Signed-off-by: Nils Radtke <lkml@Think-Future.com>
> ---
>  drivers/staging/wlan-ng/p80211netdev.c |   50 +++++++++++++++++++-------------
>  1 files changed, 30 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/staging/wlan-ng/p80211netdev.c b/drivers/staging/wlan-ng/p80211netdev.c
> index aa1792c..8104e11 100644
> --- a/drivers/staging/wlan-ng/p80211netdev.c
> +++ b/drivers/staging/wlan-ng/p80211netdev.c
> @@ -176,25 +176,30 @@ static struct net_device_stats *p80211knetdev_get_stats(netdevice_t * netdev)
>  ----------------------------------------------------------------*/
>  static int p80211knetdev_open(netdevice_t *netdev)
>  {
> -	int result = 0;		/* success */
> +	int ret = 0;

Why rename the variable if you don't have to?

>  	wlandevice_t *wlandev = netdev->ml_priv;
>  
>  	/* Check to make sure the MSD is running */
> -	if (wlandev->msdstate != WLAN_MSD_RUNNING)
> -		return -ENODEV;
> +	if (wlandev->msdstate != WLAN_MSD_RUNNING) {
> +		ret = -ENODEV;
> +		goto end;
> +	}
>  
>  	/* Tell the MSD to open */
> -	if (wlandev->open != NULL) {
> -		result = wlandev->open(wlandev);
> -		if (result = 0) {
> -			netif_start_queue(wlandev->netdev);
> -			wlandev->state = WLAN_DEVICE_OPEN;
> -		}
> -	} else {
> -		result = -EAGAIN;
> +	if (wlandev->open = NULL) {
> +		printk(KERN_ERR "Sorry, got wlandev->open = NULL.\n");
> +		ret = -EAGAIN;
> +		goto end;

Why are you adding new printk() calls?  Please use dev_err() if you
really want to do this.

>  	}
>  
> -	return result;
> +	ret = wlandev->open(wlandev);
> +	if (ret) {
> +		netif_start_queue(wlandev->netdev);
> +		wlandev->state = WLAN_DEVICE_OPEN;
> +	}
> +
> +end:
> +	return ret;

Please clean up properly for errors.

Same goes for other parts of this patch, care to redo it?

thanks,

greg k-h