[PATCH] TTY: memory leakage in tty_buffer_find()

[PATCH] TTY: memory leakage in tty_buffer_find()

[channing]

In tty_buffer_find(), it scans all tty buffers in
free buffer queue, if it finds matched one,
tty->buf.free will point to matched one's next buffer,
so tty buffers that ahead of matched one are removed
from free queue, they will never be used but they
are not released, then memory leak happen.

This patch is to make tty_buffer_find() only extract
the matched tty buffer, and keep others left inside
free queue, so that they could be found and used next
time.

Signed-off-by: Chao Bi <chao.bi@intel.com>
---
 drivers/tty/tty_buffer.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 9121c1f..7b10f7a 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -178,10 +178,14 @@ void tty_buffer_flush(struct tty_struct *tty)
 static struct tty_buffer *tty_buffer_find(struct tty_port *port, size_t size)
 {
 	struct tty_buffer **tbh = &port->buf.free;
+	struct tty_buffer *prev = NULL;
 	while ((*tbh) != NULL) {
 		struct tty_buffer *t = *tbh;
 		if (t->size >= size) {
-			*tbh = t->next;
+			if (prev == NULL)
+				*tbh = t->next;
+			else
+				prev->next = t->next;
 			t->next = NULL;
 			t->used = 0;
 			t->commit = 0;
@@ -189,6 +193,7 @@ static struct tty_buffer *tty_buffer_find(struct tty_port *port, size_t size)
 			port->buf.memory_used += t->size;
 			return t;
 		}
+		prev = t;
 		tbh = &((*tbh)->next);
 	}
 	/* Round the buffer size out */
-- 
1.7.1

Re: [PATCH] TTY: memory leakage in tty_buffer_find()

[Jiri Slaby]

On 06/26/2013 10:51 AM, channing wrote:
> 
> In tty_buffer_find(), it scans all tty buffers in
> free buffer queue, if it finds matched one,
> tty->buf.free will point to matched one's next buffer,

Oh, how is that true? tbh is moved with every iteration, right? Then:
  *tbh = t->next;
't' is what we return, 't->next' is the next one and '*tbh' is where
'next' of the previous one will point. So we just set it so we remove
't' from the list, or am I missing something?

-- 
js
suse labs

Re: [PATCH] TTY: memory leakage in tty_buffer_find()

[Jiri Slaby]

On 06/26/2013 10:55 AM, Jiri Slaby wrote:
> On 06/26/2013 10:51 AM, channing wrote:
>>
>> In tty_buffer_find(), it scans all tty buffers in
>> free buffer queue, if it finds matched one,
>> tty->buf.free will point to matched one's next buffer,
> 
> Oh, how is that true? tbh is moved with every iteration, right? Then:
>   *tbh = t->next;
> 't' is what we return, 't->next' is the next one and '*tbh' is where
> 'next' of the previous one will point. So we just set it so we remove
> 't' from the list, or am I missing something?

Actually yes. The code is pretty messy and is hiding that bug pretty
nicely. Let me figure out if there is a nice solution which would make
the code more understandable.

And we should CC: stable with the fix as it is there forever.

-- 
js
suse labs

Re: [PATCH] TTY: memory leakage in tty_buffer_find()

[Peter Hurley]

On 06/26/2013 04:51 AM, channing wrote:
>
> In tty_buffer_find(), it scans all tty buffers in
> free buffer queue, if it finds matched one,
> tty->buf.free will point to matched one's next buffer,
> so tty buffers that ahead of matched one are removed
> from free queue, they will never be used but they
> are not released, then memory leak happen.

Actually, the whole scan loop is wrong: only tty buffers of
size 256 are added to the free list.

So this can't leak because a buffer will never be found
mid-list.

Greg has a patch series from me that reduces this but it's not
yet in next.

Regards,
Peter Hurley

Re: [PATCH] TTY: memory leakage in tty_buffer_find()

[channing]

On Wed, 2013-06-26 at 08:43 -0400, Peter Hurley wrote:
> On 06/26/2013 04:51 AM, channing wrote:
> >
> > In tty_buffer_find(), it scans all tty buffers in
> > free buffer queue, if it finds matched one,
> > tty->buf.free will point to matched one's next buffer,
> > so tty buffers that ahead of matched one are removed
> > from free queue, they will never be used but they
> > are not released, then memory leak happen.
> 
> Actually, the whole scan loop is wrong: only tty buffers of
> size 256 are added to the free list.
> 
Agree that currently all tty buffers of free list are with size
of 256, but are we sure that the scan loop in tty_buffer_find()
is wrong and should abandon? From the purpose of tty_buffer_find(), 
I understand it shall scan the free list, but now it doesn't make 
sense because tty_buffer_free() makes all the free list buffers
with size of 256:

tty_buffer_free()
{
	if (b->size >= 512)
		kfree(b);
}

I don't know why it's 512? looks like a hard configuration?
Can we make it configurable instead of a fixed value?

I understand, although no memory leak, there is logic mess between 
tty_buffer_find() and tty_buffer_free(), either one shall make 
change to keep accordance? 
which one to make change might depends on original purpose of 
creating the free list. I tried to find the history of tty_buffer_free(),
but "512" is here since 2.6.32.61, I didn't find older version.

> So this can't leak because a buffer will never be found
> mid-list.
> 
> Greg has a patch series from me that reduces this but it's not
> yet in next.
> 
> Regards,
> Peter Hurley
> 

Re: [PATCH] TTY: memory leakage in tty_buffer_find()

[Peter Hurley]

On 06/26/2013 10:37 PM, channing wrote:
> On Wed, 2013-06-26 at 08:43 -0400, Peter Hurley wrote:
>> On 06/26/2013 04:51 AM, channing wrote:
>>>
>>> In tty_buffer_find(), it scans all tty buffers in
>>> free buffer queue, if it finds matched one,
>>> tty->buf.free will point to matched one's next buffer,
>>> so tty buffers that ahead of matched one are removed
>>> from free queue, they will never be used but they
>>> are not released, then memory leak happen.
>>
>> Actually, the whole scan loop is wrong: only tty buffers of
>> size 256 are added to the free list.
>>
> Agree that currently all tty buffers of free list are with size
> of 256, but are we sure that the scan loop in tty_buffer_find()
> is wrong and should abandon? From the purpose of tty_buffer_find(),
> I understand it shall scan the free list, but now it doesn't make
> sense because tty_buffer_free() makes all the free list buffers
> with size of 256:
>
> tty_buffer_free()
> {
> 	if (b->size >= 512)
> 		kfree(b);
> }
>
> I don't know why it's 512? looks like a hard configuration?
> Can we make it configurable instead of a fixed value?
>
> I understand, although no memory leak, there is logic mess between
> tty_buffer_find() and tty_buffer_free(), either one shall make
> change to keep accordance?

The approach I took in the 'lockless tty buffers' patchset was to
abandon the scan loop because that precluded the free list being
shared locklessly. My thought is that if, in the future, tty buffers
of sizes other than 256 were to be free-listed, then additional
free-list buckets could be added for the other sizes, thus retaining
the lockless behavior.