[PATCH] fix stack usage in fs/intermezzo/journal.c

[PATCH] fix stack usage in fs/intermezzo/journal.c

[Joern Engel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 2590 bytes --]

Hi!

This moves two 4k buffers from stack to heap. Compiles, untested, but
looks trivial.

Alan, is this something for your tree?

Jörn

-- 
When people work hard for you for a pat on the back, you've got
to give them that pat.
-- Robert Heinlein

--- linux-2.5.64/fs/intermezzo/journal.c	Mon Feb 24 20:05:05 2003
+++ linux-2.5.64-i2o/fs/intermezzo/journal.c	Thu Mar 13 13:14:12 2003
@@ -1245,6 +1245,7 @@
         struct file *f;
         int len;
         loff_t read_off, write_off, bytes;
+        char *buf;
 
         ENTRY;
 
@@ -1255,15 +1256,18 @@
                 return f;
         }
 
+        buf = kmalloc(4096, GFP_KERNEL);
+        if (!buf)
+                return ERR_PTR(-ENOMEM);
+
         write_off = 0;
         read_off = start;
         bytes = fset->fset_kml.fd_offset - start;
         while (bytes > 0) {
-                char buf[4096];
                 int toread;
 
-                if (bytes > sizeof(buf))
-                        toread = sizeof(buf);
+                if (bytes > sizeof(*buf))
+                        toread = sizeof(*buf);
                 else
                         toread = bytes;
 
@@ -1274,6 +1278,7 @@
 
                 if (presto_fwrite(f, buf, len, &write_off) != len) {
                         filp_close(f, NULL);
+                        kfree(buf);
                         EXIT;
                         return ERR_PTR(-EIO);
                 }
@@ -1281,6 +1286,7 @@
                 bytes -= len;
         }
 
+        kfree(buf);
         EXIT;
         return f;
 }
@@ -1589,7 +1595,7 @@
 {
         int opcode = KML_OPCODE_GET_FILEID;
         struct rec_info rec;
-        char *buffer, *path, *logrecord, record[4096]; /*include path*/
+        char *buffer, *path, *logrecord, *record; /*include path*/
         struct dentry *root;
         __u32 uid, gid, pathlen;
         int error, size;
@@ -1597,6 +1603,10 @@
 
         ENTRY;
 
+        record = kmalloc(4096, GFP_KERNEL);
+        if (!record)
+                return -ENOMEM;
+
         root = fset->fset_dentry;
 
         uid = cpu_to_le32(dentry->d_inode->i_uid);
@@ -1610,7 +1620,7 @@
                 sizeof(struct kml_suffix);
 
         CDEBUG(D_FILE, "kml size: %d\n", size);
-        if ( size > sizeof(record) )
+        if ( size > sizeof(*record) )
                 CERROR("InterMezzo: BUFFER OVERFLOW in %s!\n", __FUNCTION__);
 
         memset(&rec, 0, sizeof(rec));
@@ -1633,6 +1643,7 @@
                                    fset->fset_name);
 
         BUFF_FREE(buffer);
+        kfree(record);
         EXIT;
         return error;
 }

Re: [PATCH] fix stack usage in fs/intermezzo/journal.c

[Randy.Dunlap]

On Fri, 14 Mar 2003 16:53:52 +0100 Joern Engel <joern@wohnheim.fh-wedel.de> wrote:

| Hi!
| 
| This moves two 4k buffers from stack to heap. Compiles, untested, but
| looks trivial.
| -- 
| 
| --- linux-2.5.64/fs/intermezzo/journal.c	Mon Feb 24 20:05:05 2003
| +++ linux-2.5.64-i2o/fs/intermezzo/journal.c	Thu Mar 13 13:14:12 2003
| @@ -1245,6 +1245,7 @@
|          struct file *f;
|          int len;
|          loff_t read_off, write_off, bytes;
| +        char *buf;
|  
|          ENTRY;
|  
| @@ -1255,15 +1256,18 @@
|                  return f;
|          }
|  
| +        buf = kmalloc(4096, GFP_KERNEL);
| +        if (!buf)
| +                return ERR_PTR(-ENOMEM);
| +
|          write_off = 0;
|          read_off = start;
|          bytes = fset->fset_kml.fd_offset - start;
|          while (bytes > 0) {
| -                char buf[4096];
|                  int toread;
|  
| -                if (bytes > sizeof(buf))
| -                        toread = sizeof(buf);
| +                if (bytes > sizeof(*buf))
| +                        toread = sizeof(*buf);

I guess one of us needs some guidance here.
I thought that sizeof(*buf) == 1 here, not 4096.  Anybody?
I don't see how sizeof() can determine the kmalloc-ed size,
so I would use BUF_SIZE instead, with
#define BUF_SIZE	4096


Same for <record> below (snipped).

--
~Randy

Re: [PATCH] fix stack usage in fs/intermezzo/journal.c

[Joern Engel]

On Fri, 14 March 2003 08:09:30 -0800, Randy.Dunlap wrote:
> 
> I guess one of us needs some guidance here.
> I thought that sizeof(*buf) == 1 here, not 4096.  Anybody?
> I don't see how sizeof() can determine the kmalloc-ed size,
> so I would use BUF_SIZE instead, with
> #define BUF_SIZE	4096

I'd love to say you're wrong, but you're not. New patch is below.
FUNCTION_NAME_BUFSIZE should have less name collision than BUF_SIZE,
but someone who knows intermezzo better than me might find a much
nicer name.

Jörn

-- 
Jörn Engel
mailto: joern@wohnheim.fh-wedel.de
http://wohnheim.fh-wedel.de/~joern
Phone: +49 179 6704074

--- linux-2.5.64/fs/intermezzo/journal.c	Mon Feb 24 20:05:05 2003
+++ linux-2.5.64-i2o/fs/intermezzo/journal.c	Fri Mar 14 17:37:18 2003
@@ -1239,12 +1239,15 @@
         return izo_rcvd_write(fset, &rec);
 }
 
+/* FIXME: should the below go into some header file? */
+#define PRESTO_COPY_KML_TAIL_BUFSIZE 4096
 struct file * presto_copy_kml_tail(struct presto_file_set *fset,
                                    unsigned long int start)
 {
         struct file *f;
         int len;
         loff_t read_off, write_off, bytes;
+        char *buf;
 
         ENTRY;
 
@@ -1255,15 +1258,18 @@
                 return f;
         }
 
+        buf = kmalloc(PRESTO_COPY_KML_TAIL_BUFSIZE, GFP_KERNEL);
+        if (!buf)
+                return ERR_PTR(-ENOMEM);
+
         write_off = 0;
         read_off = start;
         bytes = fset->fset_kml.fd_offset - start;
         while (bytes > 0) {
-                char buf[4096];
                 int toread;
 
-                if (bytes > sizeof(buf))
-                        toread = sizeof(buf);
+                if (bytes > PRESTO_COPY_KML_TAIL_BUFSIZE)
+                        toread = PRESTO_COPY_KML_TAIL_BUFSIZE;
                 else
                         toread = bytes;
 
@@ -1274,6 +1280,7 @@
 
                 if (presto_fwrite(f, buf, len, &write_off) != len) {
                         filp_close(f, NULL);
+                        kfree(buf);
                         EXIT;
                         return ERR_PTR(-EIO);
                 }
@@ -1281,6 +1288,7 @@
                 bytes -= len;
         }
 
+        kfree(buf);
         EXIT;
         return f;
 }
@@ -1584,12 +1592,14 @@
         return error;
 }
 
+/* FIXME: should the below go into some header file? */
+#define PRESTO_GET_FILEID_BUFSIZE 4096
 int presto_get_fileid(int minor, struct presto_file_set *fset,
                       struct dentry *dentry)
 {
         int opcode = KML_OPCODE_GET_FILEID;
         struct rec_info rec;
-        char *buffer, *path, *logrecord, record[4096]; /*include path*/
+        char *buffer, *path, *logrecord, *record; /*include path*/
         struct dentry *root;
         __u32 uid, gid, pathlen;
         int error, size;
@@ -1597,6 +1607,10 @@
 
         ENTRY;
 
+        record = kmalloc(PRESTO_GET_FILEID_BUFSIZE, GFP_KERNEL);
+        if (!record)
+                return -ENOMEM;
+
         root = fset->fset_dentry;
 
         uid = cpu_to_le32(dentry->d_inode->i_uid);
@@ -1610,7 +1624,7 @@
                 sizeof(struct kml_suffix);
 
         CDEBUG(D_FILE, "kml size: %d\n", size);
-        if ( size > sizeof(record) )
+        if ( size > PRESTO_GET_FILEID_BUFSIZE )
                 CERROR("InterMezzo: BUFFER OVERFLOW in %s!\n", __FUNCTION__);
 
         memset(&rec, 0, sizeof(rec));
@@ -1633,6 +1647,7 @@
                                    fset->fset_name);
 
         BUFF_FREE(buffer);
+        kfree(record);
         EXIT;
         return error;
 }

Re: [PATCH] fix stack usage in fs/intermezzo/journal.c

[Randy.Dunlap]

On Fri, 14 Mar 2003 17:44:45 +0100 Joern Engel <joern@wohnheim.fh-wedel.de> wrote:

| On Fri, 14 March 2003 08:09:30 -0800, Randy.Dunlap wrote:
| > 
| > I guess one of us needs some guidance here.
| > I thought that sizeof(*buf) == 1 here, not 4096.  Anybody?
| > I don't see how sizeof() can determine the kmalloc-ed size,
| > so I would use BUF_SIZE instead, with
| > #define BUF_SIZE	4096
| 
| I'd love to say you're wrong, but you're not. New patch is below.
| FUNCTION_NAME_BUFSIZE should have less name collision than BUF_SIZE,
| but someone who knows intermezzo better than me might find a much
| nicer name.

If you are concerned about namespace & collisions, you can
#undef BUF_SIZE
after each function.

--
~Randy

Re: [PATCH] fix stack usage in fs/intermezzo/journal.c

[Joern Engel]

On Fri, 14 March 2003 08:45:36 -0800, Randy.Dunlap wrote:
> 
> If you are concerned about namespace & collisions, you can
> #undef BUF_SIZE
> after each function.

Not, if BUF_SIZE was #defined before and should maintain that value.
I could go and check for this specific case, but this is better left
to the maintainer, imho.

Jörn

-- 
"Error protection by error detection and correction."
-- from a university class

Re: [PATCH] fix stack usage in fs/intermezzo/journal.c

[Randy.Dunlap]

On Fri, 14 Mar 2003 17:55:21 +0100 Joern Engel <joern@wohnheim.fh-wedel.de> wrote:

| On Fri, 14 March 2003 08:45:36 -0800, Randy.Dunlap wrote:
| > 
| > If you are concerned about namespace & collisions, you can
| > #undef BUF_SIZE
| > after each function.
| 
| Not, if BUF_SIZE was #defined before and should maintain that value.
| I could go and check for this specific case, but this is better left
| to the maintainer, imho.

Yes, that's right.

Actually I meant for however someone chose to spell BUF_SIZE,
but that's enough said.

Bye.

--
~Randy

Re: [PATCH] fix stack usage in fs/intermezzo/journal.c

[Peter Braam]

Go for it.  Chen Yang, put it in our tree too and fix it for 2.5 as
well. 

- peter -


On Fri, Mar 14, 2003 at 08:54:53AM -0800, Randy.Dunlap wrote:
> On Fri, 14 Mar 2003 17:55:21 +0100 Joern Engel <joern@wohnheim.fh-wedel.de> wrote:
> 
> | On Fri, 14 March 2003 08:45:36 -0800, Randy.Dunlap wrote:
> | > 
> | > If you are concerned about namespace & collisions, you can
> | > #undef BUF_SIZE
> | > after each function.
> | 
> | Not, if BUF_SIZE was #defined before and should maintain that value.
> | I could go and check for this specific case, but this is better left
> | to the maintainer, imho.
> 
> Yes, that's right.
> 
> Actually I meant for however someone chose to spell BUF_SIZE,
> but that's enough said.
> 
> Bye.
> 
> --
> ~Randy
- Peter -