From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760120Ab0EDSH2 (ORCPT ); Tue, 4 May 2010 14:07:28 -0400 Received: from mail-fx0-f46.google.com ([209.85.161.46]:62626 "EHLO mail-fx0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757881Ab0EDSH0 (ORCPT ); Tue, 4 May 2010 14:07:26 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=subject:from:reply-to:to:cc:in-reply-to:references:content-type :date:message-id:mime-version:x-mailer:content-transfer-encoding; b=sib6HvQQFf2RinSfQgWjep59vHHKf5W207pi3LyRvW0fPbgUVeuIhuvdhbNMyM1ihv BQ8dRTa6Xt9rJxr5VxG4U7INvmjVA08U9KVnqU0E1EMtSyILT6opyH8N0Z8tjvJoSagk vsWi20M04tOiNleC+cQh47CyjBmiNIM21bS4w= Subject: Re: [PULL] param sysfs oops (simple, leaky) fix, bool arrays fix From: Artem Bityutskiy Reply-To: dedekind1@gmail.com To: Rusty Russell Cc: Linus Torvalds , linux-kernel@vger.kernel.org, Takashi Iwai , Sitsofe Wheeler , Frederic Weisbecker , Christof Schmitt In-Reply-To: <201005041154.24658.rusty@rustcorp.com.au> References: <200910290902.13724.rusty@rustcorp.com.au> <1272364305.26036.21.camel@localhost> <1272365604.26036.30.camel@localhost> <201005041154.24658.rusty@rustcorp.com.au> Content-Type: text/plain; charset="UTF-8" Date: Tue, 04 May 2010 21:07:19 +0300 Message-Id: <1272996439.2458.0.camel@localhost.localdomain> Mime-Version: 1.0 X-Mailer: Evolution 2.26.3 (2.26.3-1.fc11) Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2010-05-04 at 11:53 +0930, Rusty Russell wrote: > On Tue, 27 Apr 2010 08:23:24 pm Artem Bityutskiy wrote: > > On Tue, 2010-04-27 at 13:31 +0300, Artem Bityutskiy wrote: > > > On Thu, 2009-10-29 at 09:02 +1030, Rusty Russell wrote: > > > > (Thanks to Takashi-san, who found the oops and kept reading the code to spot > > > > the others. A more complete fix is pending, but this works for 2.6.32 and > > > > -stable: see commit message and FIXME in code.) > > > > > > > > The following changes since commit 964fe080d94db82a3268443e9b9ece4c60246414: > > > > Linus Torvalds (1): > > > > Merge git://git.kernel.org/.../rusty/linux-2.6-for-linus > > > > > > > > are available in the git repository at: > > > > > > > > ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-param-fixes.git master > > > > > > > > Rusty Russell (3): > > > > param: fix lots of bugs with writing charp params from sysfs, by leaking mem. > > > > param: fix NULL comparison on oom > > > > param: fix setting arrays of bool > > > > > > > > include/linux/moduleparam.h | 1 - > > > > kernel/params.c | 17 ++++++----------- > > > > 2 files changed, 6 insertions(+), 12 deletions(-) > > > > > > > > commit 65afac7d80ab3bc9f81e75eafb71eeb92a3ebdef > > > > Author: Rusty Russell > > > > Date: Thu Oct 29 08:56:16 2009 -0600 > > > > > > > > param: fix lots of bugs with writing charp params from sysfs, by leaking mem. > > > > > > > > e180a6b7759a "param: fix charp parameters set via sysfs" fixed the case > > > > where charp parameters written via sysfs were freed, leaving drivers > > > > accessing random memory. > > > > > > > > Unfortunately, storing a flag in the kparam struct was a bad idea: it's > > > > rodata so setting it causes an oops on some archs. But that's not all: > > > > > > > > 1) module_param_array() on charp doesn't work reliably, since we use an > > > > uninitialized temporary struct kernel_param. > > > > 2) there's a fundamental race if a module uses this parameter and then > > > > it's changed: they will still access the old, freed, memory. > > > > > > > > The simplest fix (ie. for 2.6.32) is to never free the memory. This > > > > prevents all these problems, at cost of a memory leak. In practice, there > > > > are only 18 places where a charp is writable via sysfs, and all are > > > > root-only writable. > > > > > > Hmm, is it really only about changing the parameters via sysfs? We see > > > the following kmemleak complaints in 2.6.32 (I think it will be the same > > > in the latest kernel, but I did not check): > > > > > > kmemleak: unreferenced object 0xdeff3c80 (size 64): > > > kmemleak: comm "modprobe", pid 788, jiffies 4294933427 > > > kmemleak: backtrace: > > > kmemleak: [] __save_stack_trace+0x34/0x40 > > > kmemleak: [] create_object+0x10c/0x208 > > > kmemleak: [] kmemleak_alloc+0x40/0x84 > > > kmemleak: [] __kmalloc_track_caller+0x140/0x154 > > > kmemleak: [] kstrdup+0x38/0x54 > > > kmemleak: [] param_set_charp+0x68/0x94 > > > kmemleak: [] parse_args+0x18c/0x280 > > > kmemleak: [] load_module+0x11e8/0x1644 > > > kmemleak: [] sys_init_module+0x60/0x1f4 > > > kmemleak: [] ret_fast_syscall+0x0/0x38 > > > > > > So we are leaking on every insmod/rmmod, AFAICS, not just in the sysfs > > > case. > > > > Rusty, correct me if I'm wrong, but it looks like the above memleak was > > introduced by e180a6b7759a99a28cbcce3547c4c80822cb6c2a, where you added > > the kstrdup(). So you kinda fixed the sysfs case (it still memleaks > > though), but at the cost of additional insmod/rmmod leak, right? > > Yep! Are you working/planning to work on fixing this regression? -- Best Regards, Artem Bityutskiy (Артём Битюцкий)