TTY: memory leakage in tty_buffer_find()
PATCH 1/1 TTY: memory leakage in tty_buffer_find()
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.
--- drivers/tty/tty_buffer.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
drivers/tty/tty_buffer.c +6 -1
@@ -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 */
Signature · 1 line
1.7.1
Quoted 4 lines · channing
In
tty_buffer_find(), it scans all tty buffers in free buffer queue, if it finds matched one,tty->buf.freewill 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?
Signature · 2 lines
js suse labs
Quoted 11 lines · Jiri Slaby
Quoted 4 lines · channing
In
tty_buffer_find(), it scans all tty buffers in free buffer queue, if it finds matched one,tty->buf.freewill 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.
Signature · 2 lines
js suse labs
Quoted 7 lines · channing
In
tty_buffer_find(), it scans all tty buffers in free buffer queue, if it finds matched one,tty->buf.freewill 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
Quoted 9 lines · Peter Hurley
Quoted 7 lines · channing
In
tty_buffer_find(), it scans all tty buffers in free buffer queue, if it finds matched one,tty->buf.freewill 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.
Quoted 3 lines
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.
Quoted 9 lines
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
Quoted 26 lines · channing
Quoted 9 lines · Peter Hurley
Quoted 7 lines · channing
In
tty_buffer_find(), it scans all tty buffers in free buffer queue, if it finds matched one,tty->buf.freewill 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.Quoted 3 lines
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 oftty_buffer_find(), I understand it shall scan the free list, but now it doesn't make sense becausetty_buffer_free()makes all the free list buffers with size of 256:
tty_buffer_free(){ if (b->size >= 512) kfree(b); }
Quoted 6 lines
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()andtty_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.