[OPW,kernel] Re: [PATCH] Staging:dgap:Fix stack failure warning frame size larger than 1024 bytes
diff mbox

Message ID CAE+7SwnPYCs0pF6ypT3QE8f-jHbUFgn1n=UDFWA_rnp5EtbBDA@mail.gmail.com
State Changes Requested
Headers show

Commit Message

ritas1188@gmail.com Oct. 30, 2013, 9:26 a.m. UTC
From: Rita Sinha <ritas1188@gmail.com>

This patch fixes stack failure by replacing array with pointers to give
same amount of memory but now only at runtime and freeing it immediately
after use.

Signed-off-by: Rita Sinha <ritas1188@gmail.com>
---
 drivers/staging/dgap/dgap_driver.c |    8 +++++++-
 drivers/staging/dgap/dgap_fep5.c   |   18 +++++++++++++++---
 drivers/staging/dgap/dgap_tty.c    |   11 +++++++++--
 3 files changed, 31 insertions(+), 6 deletions(-)

                return;
@@ -655,6 +661,7 @@ static void dgap_sniff_nowait_nolock(struct channel_t
*ch, uchar *text, uchar *b
                }

        } while (too_much_data);
+       kfree(tmpbuf);
 }

Comments

Waskiewicz Jr, Peter P Oct. 30, 2013, 9:42 a.m. UTC | #1
On Wed, 2013-10-30 at 14:56 +0530, rita wrote:
> From: Rita Sinha <ritas1188@gmail.com>
> 
> This patch fixes stack failure by replacing array with pointers to
> give same amount of memory but now only at runtime and freeing it
> immediately after use.

A couple things to still adjust:

First, please start new emails when sending an updated patch.  Don't
reply to the original.  Also, please make sure to version the patch, so
reviewers can quickly see which version of the patch should be the
latest.  Please look at http://kernelnewbies.org/OPWfirstpatch down at
11. Submit a patch.  There is a set of instructions on how to properly
version an updated patch.

Second, I don't think you used git send-email to send this, since the
mail appears to be rich text formatted.  Can you please make sure to
send your patches from a mail client that won't try and rich-format the
text?

A bit more feedback down below.

> Signed-off-by: Rita Sinha <ritas1188@gmail.com>
> ---
>  drivers/staging/dgap/dgap_driver.c |    8 +++++++-
>  drivers/staging/dgap/dgap_fep5.c   |   18 +++++++++++++++---
>  drivers/staging/dgap/dgap_tty.c    |   11 +++++++++--
>  3 files changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/dgap/dgap_driver.c
> b/drivers/staging/dgap/dgap_driver.c
> index 4c1515e..e09fc79 100644
> --- a/drivers/staging/dgap/dgap_driver.c
> +++ b/drivers/staging/dgap/dgap_driver.c
> @@ -945,11 +945,16 @@ void *dgap_driver_kzmalloc(size_t size, int
> priority)
>   */
>  static void dgap_mbuf(struct board_t *brd, const char *fmt, ...) {
>         va_list         ap;
> -       char            buf[1024];
> +       char            *buf;
>         int             i;
>         unsigned long   flags;
>         size_t          length;
> 
> +       buf = kmalloc(1024, GFP_ATOMIC);

It may be safe to use GFP_KERNEL here.  I think Greg's comment was for
you to make sure it's safe to allocate; you only want GFP_ATOMIC if you
can't sleep here.  Those types of situations are usually interrupts,
which I don't think this is.  Did you uncover that GFP_ATOMIC is
necessary here?

> +       if(buf == NULL){

This introduces a style issue.  There should be a space between the if
and the (, and also a space between the ) and the {.  You can also
simplify the check (this is just a matter of personal preference):

	if (!buf) {

> +          kfree(buf);

No need to kfree() the buffer if it didn't succeed to get allocated.
And if you don't need the kfree(), you can dump the curly braces on the
if ().

> +          return -ENOMEM;
> +       }
>         DGAP_LOCK(dgap_global_lock, flags);
> 
>         /* Format buf using fmt and arguments contained in ap. */
> @@ -972,6 +977,7 @@ static void dgap_mbuf(struct board_t *brd, const
> char *fmt, ...) {
>         brd->msgbuf += length;
> 
>         DGAP_UNLOCK(dgap_global_lock, flags);
> +       kfree(buf);
>  }
> 
> 
> diff --git a/drivers/staging/dgap/dgap_fep5.c
> b/drivers/staging/dgap/dgap_fep5.c
> index 794cf9d..c68057f 100644
> --- a/drivers/staging/dgap/dgap_fep5.c
> +++ b/drivers/staging/dgap/dgap_fep5.c
> @@ -72,9 +72,14 @@ void dgap_do_config_load(uchar __user *uaddr, int
> len)
>         int orig_len = len;
>         char *to_addr;
>         uchar __user *from_addr = uaddr;
> -       char buf[U2BSIZE];
> +       char *buf;
>         int n;
> 
> +       buf = kmalloc(U2BSIZE, GFP_ATOMIC);

Same comment, is GFP_ATOMIC really necessary?

> +       if(buf == NULL) {

Same style issue on the if needing a space before the (.  You can also
do the same simplification of:

	if (!buf)

> +          kfree(buf);

Don't need the kfree() if it doesn't get allocated.

> +          return buf;

I don't think you want to return NULL here, probably the same as above,
return -ENOMEM?

> +       }
>         to_addr = dgap_config_buf = dgap_driver_kzmalloc(len + 1,
> GFP_ATOMIC);
>         if (!dgap_config_buf) {
>                 DPR_INIT(("dgap_do_config_load - unable to allocate
> memory for file\n"));
> @@ -107,7 +112,8 @@ void dgap_do_config_load(uchar __user *uaddr, int
> len)
>         dgap_parsefile(&to_addr, TRUE);
> 
>         DPR_INIT(("dgap_config_load() finish\n"));
> -
> +
> +       kfree(buf);
>         return;
>  }
> 
> @@ -146,9 +152,14 @@ int dgap_after_config_loaded(void)
> 
> *=======================================================================*/
>  static int dgap_usertoboard(struct board_t *brd, char *to_addr, char
> __user *from_addr, int len)
>  {
> -       char buf[U2BSIZE];
> +       char *buf;
>         int n = U2BSIZE;
> 
> +       buf = kmalloc(U2BSIZE, GFP_ATOMIC);
> +       if(buf == NULL){
> +         kfree(buf);
> +         return -ENOMEM;
> +       }

Same comments as the above comments.

>         if (!brd || brd->magic != DGAP_BOARD_MAGIC)
>                 return -EFAULT;
> 
> @@ -169,6 +180,7 @@ static int dgap_usertoboard(struct board_t *brd,
> char *to_addr, char __user *fro
>                 from_addr += n;
>                 n = U2BSIZE;
>          }
> +       kfree(buf);
>         return 0;
>  }
> 
> diff --git a/drivers/staging/dgap/dgap_tty.c
> b/drivers/staging/dgap/dgap_tty.c
> index 2a7a372..e056da3 100644
> --- a/drivers/staging/dgap/dgap_tty.c
> +++ b/drivers/staging/dgap/dgap_tty.c
> @@ -556,10 +556,16 @@ static void dgap_sniff_nowait_nolock(struct
> channel_t *ch, uchar *text, uchar *b
>         int nbuf;
>         int i;
>         int tmpbuflen;
> -       char tmpbuf[TMPBUFLEN];
> -       char *p = tmpbuf;
> +       char *tmpbuf;
> +       char *p;
>         int too_much_data;
> 
> +       tmpbuf = kmalloc(TMPBUFLEN, GFP_ATOMIC);

Same feedback.

> +       if(tmpbuf == NULL){
> +          kfree(tmpbuf);

Same feedback.

> +          return -ENOMEM;
> +       }
> +       p = tmpbuf;
>         /* Leave if sniff not open */
>         if (!(ch->ch_sniff_flags & SNIFF_OPEN))
>                 return;
> @@ -655,6 +661,7 @@ static void dgap_sniff_nowait_nolock(struct
> channel_t *ch, uchar *text, uchar *b
>                 }
> 
>         } while (too_much_data);
> +       kfree(tmpbuf);
>  }
>
Josh Triplett Nov. 3, 2013, 6:17 p.m. UTC | #2
On Wed, Oct 30, 2013 at 02:56:58PM +0530, rita wrote:
> --- a/drivers/staging/dgap/dgap_driver.c
> +++ b/drivers/staging/dgap/dgap_driver.c
> @@ -945,11 +945,16 @@ void *dgap_driver_kzmalloc(size_t size, int priority)
>   */
>  static void dgap_mbuf(struct board_t *brd, const char *fmt, ...) {
>         va_list         ap;
> -       char            buf[1024];
> +       char            *buf;
>         int             i;
>         unsigned long   flags;
>         size_t          length;
> 
> +       buf = kmalloc(1024, GFP_ATOMIC);
> +       if(buf == NULL){
> +          kfree(buf);
> +          return -ENOMEM;
> +       }

This function is void, so you can't return -ENOMEM here.

- Josh Triplett

Patch
diff mbox

diff --git a/drivers/staging/dgap/dgap_driver.c b/drivers/staging/dgap/dgap_
driver.c
index 4c1515e..e09fc79 100644
--- a/drivers/staging/dgap/dgap_driver.c
+++ b/drivers/staging/dgap/dgap_driver.c
@@ -945,11 +945,16 @@  void *dgap_driver_kzmalloc(size_t size, int priority)
  */
 static void dgap_mbuf(struct board_t *brd, const char *fmt, ...) {
        va_list         ap;
-       char            buf[1024];
+       char            *buf;
        int             i;
        unsigned long   flags;
        size_t          length;

+       buf = kmalloc(1024, GFP_ATOMIC);
+       if(buf == NULL){
+          kfree(buf);
+          return -ENOMEM;
+       }
        DGAP_LOCK(dgap_global_lock, flags);

        /* Format buf using fmt and arguments contained in ap. */
@@ -972,6 +977,7 @@  static void dgap_mbuf(struct board_t *brd, const char
*fmt, ...) {
        brd->msgbuf += length;

        DGAP_UNLOCK(dgap_global_lock, flags);
+       kfree(buf);
 }


diff --git a/drivers/staging/dgap/dgap_fep5.c b/drivers/staging/dgap/dgap_
fep5.c
index 794cf9d..c68057f 100644
--- a/drivers/staging/dgap/dgap_fep5.c
+++ b/drivers/staging/dgap/dgap_fep5.c
@@ -72,9 +72,14 @@  void dgap_do_config_load(uchar __user *uaddr, int len)
        int orig_len = len;
        char *to_addr;
        uchar __user *from_addr = uaddr;
-       char buf[U2BSIZE];
+       char *buf;
        int n;

+       buf = kmalloc(U2BSIZE, GFP_ATOMIC);
+       if(buf == NULL) {
+          kfree(buf);
+          return buf;
+       }
        to_addr = dgap_config_buf = dgap_driver_kzmalloc(len + 1,
GFP_ATOMIC);
        if (!dgap_config_buf) {
                DPR_INIT(("dgap_do_config_load - unable to allocate memory
for file\n"));
@@ -107,7 +112,8 @@  void dgap_do_config_load(uchar __user *uaddr, int len)
        dgap_parsefile(&to_addr, TRUE);

        DPR_INIT(("dgap_config_load() finish\n"));
-
+
+       kfree(buf);
        return;
 }

@@ -146,9 +152,14 @@  int dgap_after_config_loaded(void)
  *=======================================================================*/
 static int dgap_usertoboard(struct board_t *brd, char *to_addr, char
__user *from_addr, int len)
 {
-       char buf[U2BSIZE];
+       char *buf;
        int n = U2BSIZE;

+       buf = kmalloc(U2BSIZE, GFP_ATOMIC);
+       if(buf == NULL){
+         kfree(buf);
+         return -ENOMEM;
+       }
        if (!brd || brd->magic != DGAP_BOARD_MAGIC)
                return -EFAULT;

@@ -169,6 +180,7 @@  static int dgap_usertoboard(struct board_t *brd, char
*to_addr, char __user *fro
                from_addr += n;
                n = U2BSIZE;
         }
+       kfree(buf);
        return 0;
 }

diff --git a/drivers/staging/dgap/dgap_tty.c b/drivers/staging/dgap/dgap_
tty.c
index 2a7a372..e056da3 100644
--- a/drivers/staging/dgap/dgap_tty.c
+++ b/drivers/staging/dgap/dgap_tty.c
@@ -556,10 +556,16 @@  static void dgap_sniff_nowait_nolock(struct channel_t
*ch, uchar *text, uchar *b
        int nbuf;
        int i;
        int tmpbuflen;
-       char tmpbuf[TMPBUFLEN];
-       char *p = tmpbuf;
+       char *tmpbuf;
+       char *p;
        int too_much_data;

+       tmpbuf = kmalloc(TMPBUFLEN, GFP_ATOMIC);
+       if(tmpbuf == NULL){
+          kfree(tmpbuf);
+          return -ENOMEM;
+       }
+       p = tmpbuf;
        /* Leave if sniff not open */
        if (!(ch->ch_sniff_flags & SNIFF_OPEN))