Re: [PATCH] fix memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()

2006-08-10 Thread David Miller 
From: Stephen Hemminger <[EMAIL PROTECTED]>
Date: Thu, 10 Aug 2006 16:52:16 -0700

> Dave, here is my version...
> Don't leak memory on interrupted read. And only allocate
> as much memory as needed.
> 
> Signed-off-by: Stephen Hemminger <[EMAIL PROTECTED]>

I think I'm going to go with James's safe original fix
for now, thanks.

commit a7fc5b24a4921a6582ce47c0faf3a31858a80468
Author: David S. Miller <[EMAIL PROTECTED]>
Date:   Thu Aug 10 16:53:33 2006 -0700

[TCP]: Fix botched memory leak fix to tcpprobe_read().

Somehow I clobbered James's original fix and only my
subsequent compiler warning change went in for that
changeset.

Get the real fix in there.

Noticed by Jesper Juhl.

Signed-off-by: David S. Miller <[EMAIL PROTECTED]>

diff --git a/net/ipv4/tcp_probe.c b/net/ipv4/tcp_probe.c
index b343532..dab37d2 100644
--- a/net/ipv4/tcp_probe.c
+++ b/net/ipv4/tcp_probe.c
@@ -130,11 +130,12 @@ static ssize_t tcpprobe_read(struct file
error = wait_event_interruptible(tcpw.wait,
 __kfifo_len(tcpw.fifo) != 0);
if (error)
-   return error;
+   goto out_free;
 
cnt = kfifo_get(tcpw.fifo, tbuf, len);
error = copy_to_user(buf, tbuf, cnt);
 
+out_free:
vfree(tbuf);
 
return error ? error : cnt;
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()

2006-08-10 Thread Stephen Hemminger 
Dave, here is my version...
Don't leak memory on interrupted read. And only allocate
as much memory as needed.

Signed-off-by: Stephen Hemminger <[EMAIL PROTECTED]>


--- linux-2.6.orig/net/ipv4/tcp_probe.c 2006-08-10 16:32:36.0 -0700
+++ linux-2.6/net/ipv4/tcp_probe.c  2006-08-10 16:45:30.0 -0700
@@ -114,7 +114,7 @@
 static ssize_t tcpprobe_read(struct file *file, char __user *buf,
 size_t len, loff_t *ppos)
 {
-   int error = 0, cnt = 0;
+   int error, cnt;
unsigned char *tbuf;
 
if (!buf || len < 0)
@@ -123,15 +123,16 @@
if (len == 0)
return 0;
 
-   tbuf = vmalloc(len);
-   if (!tbuf)
-   return -ENOMEM;
-
error = wait_event_interruptible(tcpw.wait,
 __kfifo_len(tcpw.fifo) != 0);
if (error)
return error;
 
+   len = min(len, kfifo_len(tcpw.fifo));
+   tbuf = vmalloc(len);
+   if (!tbuf)
+   return -ENOMEM;
+
cnt = kfifo_get(tcpw.fifo, tbuf, len);
error = copy_to_user(buf, tbuf, cnt);
 
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()

2006-08-10 Thread David Miller 
From: "Jesper Juhl" <[EMAIL PROTECTED]>
Date: Fri, 11 Aug 2006 00:18:44 +0200

> Hmm, perhaps I'm going blind, but I don't see it.

I definitely screwed that changeset up somehow.  Thanks for catching
this.

Somehow James's fix got clobbered into just my subsequent warning fix,
like this:

commit 118075b3cdc90e0815362365f3fc64d672ace0d6
Author: James Morris <[EMAIL PROTECTED]>
Date:   Sun Jul 30 20:21:45 2006 -0700

[TCP]: fix memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()

Based upon a patch by Jesper Juhl.

Signed-off-by: James Morris <[EMAIL PROTECTED]>
Acked-by: Stephen Hemminger <[EMAIL PROTECTED]>
Acked-by: Jesper Juhl <[EMAIL PROTECTED]>
Signed-off-by: David S. Miller <[EMAIL PROTECTED]>

diff --git a/net/ipv4/tcp_probe.c b/net/ipv4/tcp_probe.c
index d7d517a..b343532 100644
--- a/net/ipv4/tcp_probe.c
+++ b/net/ipv4/tcp_probe.c
@@ -114,7 +114,7 @@ static int tcpprobe_open(struct inode * 
 static ssize_t tcpprobe_read(struct file *file, char __user *buf,
 size_t len, loff_t *ppos)
 {
-   int error = 0, cnt;
+   int error = 0, cnt = 0;
unsigned char *tbuf;
 
if (!buf || len < 0)

Anyways, I'll fix this up, thanks again.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()

2006-08-10 Thread Jesper Juhl 

On 05/08/06, David Miller <[EMAIL PROTECTED]> wrote:

From: "Jesper Juhl" <[EMAIL PROTECTED]>
Date: Sat, 5 Aug 2006 01:30:49 +0200

> On 31/07/06, David Miller <[EMAIL PROTECTED]> wrote:
> > From: "Jesper Juhl" <[EMAIL PROTECTED]>
> > Date: Sun, 30 Jul 2006 23:51:20 +0200
> >
> > > Looks ok to me.
> >
> > I've applied James's version of the fix, thanks everyone.
> >
> Hmm, if you are refering to commit
> 118075b3cdc90e0815362365f3fc64d672ace0d6 -
> 
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=118075b3cdc90e0815362365f3fc64d672ace0d6
> then I think a mistake has crept in. That commit only initializes
> 'cnt' to 0 - I don't see how that would fix the leak - looks like you
> forgot the business end of the patch...

See the commit right before that, the initialize of cnt to
zero is just to fix a compiler warning that resulted from
James's version of the fix.



Hmm, perhaps I'm going blind, but I don't see it.

The commit right before the one I linked to above is completely
unrelated : "[ATALK]: Make CONFIG_DEV_APPLETALK a tristate."
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=9cac2c35e26cc44978df654306bb92d7cfe7e2de

And if I download 2.6.18-rc4 the tcpprobe_read() function (still)
looks like this :


static ssize_t tcpprobe_read(struct file *file, char __user *buf,
size_t len, loff_t *ppos)
{
   int error = 0, cnt = 0;
   unsigned char *tbuf;

   if (!buf || len < 0)
   return -EINVAL;

   if (len == 0)
   return 0;

   tbuf = vmalloc(len);
   if (!tbuf)
   return -ENOMEM;

   error = wait_event_interruptible(tcpw.wait,
__kfifo_len(tcpw.fifo) != 0);
   if (error)
   return error;

   cnt = kfifo_get(tcpw.fifo, tbuf, len);
   error = copy_to_user(buf, tbuf, cnt);

   vfree(tbuf);

   return error ? error : cnt;
}


That function still contains the 'tbuf' leak.

I also couldn't find the fix in your git trees at
http://www.kernel.org/git/?p=linux/kernel/git/davem/net-2.6.19.git;a=summary
http://www.kernel.org/git/?p=linux/kernel/git/davem/net-2.6.git;a=summary


So either I'm going blind or a mistake has been made getting the fix
into mainline...


--
Jesper Juhl <[EMAIL PROTECTED]>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please  http://www.expita.com/nomime.html
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()

2006-08-04 Thread David Miller 
From: "Jesper Juhl" <[EMAIL PROTECTED]>
Date: Sat, 5 Aug 2006 01:30:49 +0200

> On 31/07/06, David Miller <[EMAIL PROTECTED]> wrote:
> > From: "Jesper Juhl" <[EMAIL PROTECTED]>
> > Date: Sun, 30 Jul 2006 23:51:20 +0200
> >
> > > Looks ok to me.
> >
> > I've applied James's version of the fix, thanks everyone.
> >
> Hmm, if you are refering to commit
> 118075b3cdc90e0815362365f3fc64d672ace0d6 -
> http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=118075b3cdc90e0815362365f3fc64d672ace0d6
> then I think a mistake has crept in. That commit only initializes
> 'cnt' to 0 - I don't see how that would fix the leak - looks like you
> forgot the business end of the patch...

See the commit right before that, the initialize of cnt to
zero is just to fix a compiler warning that resulted from
James's version of the fix.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()

2006-08-04 Thread Jesper Juhl 

On 31/07/06, David Miller <[EMAIL PROTECTED]> wrote:

From: "Jesper Juhl" <[EMAIL PROTECTED]>
Date: Sun, 30 Jul 2006 23:51:20 +0200

> Looks ok to me.

I've applied James's version of the fix, thanks everyone.


Hmm, if you are refering to commit
118075b3cdc90e0815362365f3fc64d672ace0d6 -
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=118075b3cdc90e0815362365f3fc64d672ace0d6
then I think a mistake has crept in. That commit only initializes
'cnt' to 0 - I don't see how that would fix the leak - looks like you
forgot the business end of the patch...

The function still looks like this after that commit :

114 static ssize_t tcpprobe_read(struct file *file, char __user *buf,
115 size_t len, loff_t *ppos)
116 {
117 int error = 0, cnt = 0;
118 unsigned char *tbuf;
119
120 if (!buf || len < 0)
121 return -EINVAL;
122
123 if (len == 0)
124 return 0;
125
126 tbuf = vmalloc(len);
127 if (!tbuf)
128 return -ENOMEM;
129
130 error = wait_event_interruptible(tcpw.wait,
131 __kfifo_len(tcpw.fifo) != 0);
132 if (error)
133 return error;
134

   ^^^--- we are still leaking 'tbuf' here.


135 cnt = kfifo_get(tcpw.fifo, tbuf, len);
136 error = copy_to_user(buf, tbuf, cnt);
137
138 vfree(tbuf);
139
140 return error ? error : cnt;
141 }


--
Jesper Juhl <[EMAIL PROTECTED]>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please  http://www.expita.com/nomime.html
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()

2006-07-30 Thread David Miller 
From: "Jesper Juhl" <[EMAIL PROTECTED]>
Date: Sun, 30 Jul 2006 23:51:20 +0200

> Looks ok to me.

I've applied James's version of the fix, thanks everyone.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()

2006-07-30 Thread Jesper Juhl 

On 30/07/06, James Morris <[EMAIL PROTECTED]> wrote:

On Sun, 30 Jul 2006, Stephen Hemminger wrote:

> On Sun, 30 Jul 2006 21:38:02 +0200
> Jesper Juhl <[EMAIL PROTECTED]> wrote:
>
> > There's an obvious memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()
> > We are not freeing 'tbuf' on error.
> > Patch below fixes that.
> >
> >
> > Signed-off-by: Jesper Juhl <[EMAIL PROTECTED]>
> Agreed, thanks for catching it.  The whole kfifo interface is kind of
> annoying have to do an extra copy.

Might be cleaner to make a single return path for cleanup:

Signed-off-by: James Morris <[EMAIL PROTECTED]>


Looks ok to me.

--
Jesper Juhl <[EMAIL PROTECTED]>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please  http://www.expita.com/nomime.html
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()

2006-07-30 Thread James Morris 
On Sun, 30 Jul 2006, Stephen Hemminger wrote:

> On Sun, 30 Jul 2006 21:38:02 +0200
> Jesper Juhl <[EMAIL PROTECTED]> wrote:
> 
> > There's an obvious memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()
> > We are not freeing 'tbuf' on error.
> > Patch below fixes that.
> > 
> > 
> > Signed-off-by: Jesper Juhl <[EMAIL PROTECTED]>
> Agreed, thanks for catching it.  The whole kfifo interface is kind of
> annoying have to do an extra copy.

Might be cleaner to make a single return path for cleanup:

Signed-off-by: James Morris <[EMAIL PROTECTED]>

---


--- linux-2.6.18-rc2-mm1.o/net/ipv4/tcp_probe.c 2006-07-28 11:01:46.0 
-0400
+++ linux-2.6.18-rc2-mm1.w/net/ipv4/tcp_probe.c 2006-07-30 17:45:53.0 
-0400
@@ -130,11 +130,12 @@ static ssize_t tcpprobe_read(struct file
error = wait_event_interruptible(tcpw.wait,
 __kfifo_len(tcpw.fifo) != 0);
if (error)
-   return error;
+   goto out_free;
 
cnt = kfifo_get(tcpw.fifo, tbuf, len);
error = copy_to_user(buf, tbuf, cnt);
 
+out_free:
vfree(tbuf);
 
return error ? error : cnt;
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()

2006-07-30 Thread Stephen Hemminger 
On Sun, 30 Jul 2006 21:38:02 +0200
Jesper Juhl <[EMAIL PROTECTED]> wrote:

> There's an obvious memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()
> We are not freeing 'tbuf' on error.
> Patch below fixes that.
> 
> 
> Signed-off-by: Jesper Juhl <[EMAIL PROTECTED]>
Agreed, thanks for catching it.  The whole kfifo interface is kind of
annoying have to do an extra copy.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] fix memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()

2006-07-30 Thread Jesper Juhl 
There's an obvious memory leak in net/ipv4/tcp_probe.c::tcpprobe_read()
We are not freeing 'tbuf' on error.
Patch below fixes that.


Signed-off-by: Jesper Juhl <[EMAIL PROTECTED]>
---

 net/ipv4/tcp_probe.c |4 +++-
 1 files changed, 3 insertions(+), 1 deletion(-)

--- linux-2.6.18-rc3-orig/net/ipv4/tcp_probe.c  2006-07-30 13:21:53.0 
+0200
+++ linux-2.6.18-rc3/net/ipv4/tcp_probe.c   2006-07-30 21:32:04.0 
+0200
@@ -129,8 +129,10 @@ static ssize_t tcpprobe_read(struct file
 
error = wait_event_interruptible(tcpw.wait,
 __kfifo_len(tcpw.fifo) != 0);
-   if (error)
+   if (error) {
+   vfree(tbuf);
return error;
+   }
 
cnt = kfifo_get(tcpw.fifo, tbuf, len);
error = copy_to_user(buf, tbuf, cnt);



-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html