All of lore.kernel.org
 help / color / mirror / Atom feed
From: Johannes Weiner <hannes@cmpxchg.org>
To: Catalin Marinas <catalin.marinas@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Pekka Enberg <penberg@cs.helsinki.fi>,
	linux-kernel@vger.kernel.org
Subject: [patch] fbcon: don't use vc_resize() on initialization
Date: Wed, 29 Jul 2009 19:21:23 +0200	[thread overview]
Message-ID: <20090729172123.GA14192@cmpxchg.org> (raw)
In-Reply-To: <1248865456.2305.12.camel@pc1117.cambridge.arm.com>

Catalin and kmemleak spotted a leak of a VC screen buffer in
vc_allocate() due to the following chain of events:

	vc_allocate()
	  visual_init(init=1)
	    vc->vc_sw->con_init(init=1)
              fbcon_init()
	        vc_resize()
	          vc->screen_buf = kmalloc()
	  vc->screen_buf = kmalloc()

The common way for the VC drivers is to set the screen dimension
parameters manually in the init case and only call vc_resize() for
!init - which allocates a screen buffer according to the new
dimensions.

fbcon instead would do vc_resize() unconditionally and afterwards set
the dimensions manually (again) for !init - i.e. completely upside
down.  The vc_resize() allocated buffer would then get lost by
vc_allocate() allocating a fresh one.

Use vc_resize() only for actual resizing to close the leak.

Set the dimensions manually only in initialization mode to remove the
redundant setting in resize mode.

The kmemleak trace from Catalin:

unreferenced object 0xde158000 (size 12288):
  comm "Xorg", pid 1439, jiffies 4294961016
  hex dump (first 32 bytes):
    20 00 20 00 20 00 20 00 20 00 20 00 20 00 20 00   . . . . . . . .
    20 00 20 00 20 00 20 00 20 00 20 00 20 00 20 00   . . . . . . . .
  backtrace:
    [<c006f74b>] __save_stack_trace+0x17/0x1c
    [<c006f81d>] create_object+0xcd/0x188
    [<c01f5457>] kmemleak_alloc+0x1b/0x3c
    [<c006e303>] __kmalloc+0xdb/0xe8
    [<c012cc4b>] vc_do_resize+0x73/0x1e0
    [<c012cdf1>] vc_resize+0x15/0x18
    [<c011afc1>] fbcon_init+0x1f9/0x2b8
    [<c0129e87>] visual_init+0x9f/0xdc
    [<c012aff3>] vc_allocate+0x7f/0xfc
    [<c012b087>] con_open+0x17/0x80
    [<c0120e43>] tty_open+0x1f7/0x2e4
    [<c0072fa1>] chrdev_open+0x101/0x118
    [<c006ffad>] __dentry_open+0x105/0x1cc
    [<c00700fd>] nameidata_to_filp+0x2d/0x38
    [<c00788cd>] do_filp_open+0x2c1/0x54c
    [<c006fdff>] do_sys_open+0x3b/0xb4

Reported-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Tested-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Pekka Enberg <penberg@cs.helsinki.fi>
---
 drivers/video/console/fbcon.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index 471a9a6..3a44695 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -1082,7 +1082,6 @@ static void fbcon_init(struct vc_data *vc, int init)
 	new_rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
 	new_cols /= vc->vc_font.width;
 	new_rows /= vc->vc_font.height;
-	vc_resize(vc, new_cols, new_rows);
 
 	/*
 	 * We must always set the mode. The mode of the previous console
@@ -1111,10 +1110,11 @@ static void fbcon_init(struct vc_data *vc, int init)
 	 *  vc_{cols,rows}, but we must not set those if we are only
 	 *  resizing the console.
 	 */
-	if (!init) {
+	if (init) {
 		vc->vc_cols = new_cols;
 		vc->vc_rows = new_rows;
-	}
+	} else
+		vc_resize(vc, new_cols, new_rows);
 
 	if (logo)
 		fbcon_prepare_logo(vc, info, cols, rows, new_cols, new_rows);
-- 
1.6.3

  reply	other threads:[~2009-07-29 17:23 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-07-29  9:31 Another memory leak in drivers/char/vt.c Catalin Marinas
2009-07-29 10:39 ` Johannes Weiner
2009-07-29 11:04   ` Catalin Marinas
2009-07-29 17:21     ` Johannes Weiner [this message]
2009-07-30 23:11       ` [patch] fbcon: don't use vc_resize() on initialization Andrew Morton
2009-07-31  9:09         ` Catalin Marinas
2009-08-01  8:31       ` Dave Young

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20090729172123.GA14192@cmpxchg.org \
    --to=hannes@cmpxchg.org \
    --cc=akpm@linux-foundation.org \
    --cc=catalin.marinas@arm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=penberg@cs.helsinki.fi \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.