From patchwork Thu May 19 12:35:08 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pavel Machek X-Patchwork-Id: 797492 Received: from bombadil.infradead.org (bombadil.infradead.org [18.85.46.34]) by demeter1.kernel.org (8.14.4/8.14.3) with ESMTP id p4JCamb8016363 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Thu, 19 May 2011 12:37:10 GMT Received: from canuck.infradead.org ([2001:4978:20e::1]) by bombadil.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1QN2RN-0006zd-E2; Thu, 19 May 2011 12:34:57 +0000 Received: from localhost ([127.0.0.1] helo=canuck.infradead.org) by canuck.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1QN2RL-0003wy-ML; Thu, 19 May 2011 12:34:55 +0000 Received: from ksp.mff.cuni.cz ([195.113.26.206] helo=atrey.karlin.mff.cuni.cz) by canuck.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1QN2RH-0003wf-Bv for linux-arm-kernel@lists.infradead.org; Thu, 19 May 2011 12:34:52 +0000 Received: by atrey.karlin.mff.cuni.cz (Postfix, from userid 512) id 81138F0615; Thu, 19 May 2011 14:34:48 +0200 (CEST) Date: Thu, 19 May 2011 14:35:08 +0200 From: Pavel Machek To: Russell King - ARM Linux Subject: Re: [PATCH] MAX1111: Fix race condition causing NULL pointer exception Message-ID: <20110519123507.GA4950@elf.ucw.cz> References: <1305731918-20164-1-git-send-email-morpheus.ibis@gmail.com> <20110518152935.GJ5913@n2100.arm.linux.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20110518152935.GJ5913@n2100.arm.linux.org.uk> X-Warning: Reading this can be dangerous to your mental health. User-Agent: Mutt/1.5.20 (2009-06-14) X-CRM114-Version: 20090807-BlameThorstenAndJenny ( TRE 0.7.6 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20110519_083451_718747_7986086C X-CRM114-Status: GOOD ( 24.66 ) X-Spam-Score: 0.0 (/) X-Spam-Report: SpamAssassin version 3.3.1 on canuck.infradead.org summary: Content analysis details: (0.0 points) pts rule name description ---- ---------------------- -------------------------------------------------- Cc: eric.y.miao@gmail.com, metan@ucw.cz, Pavel Herrmann , utx@penguin.cz, marek.vasut@gmail.com, zaurus-devel@www.linuxtogo.org, linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Thu, 19 May 2011 12:37:10 +0000 (UTC) Hi! On Wed 2011-05-18 16:29:35, Russell King - ARM Linux wrote: > On Wed, May 18, 2011 at 05:18:38PM +0200, Pavel Herrmann wrote: > > spi_sync call uses its spi_message parameter to keep completion information, > > having this structure static is not thread-safe, potentially causing one > > thread having pointers to memory on or above other threads stack. use > > per-call spi_message on stack to fix this > > I assume this has not been tested with DMA debugging enabled. > > The DMA API does not like mapping memory from the stack, which is what > you're potentially doing with this: In some other mail, you said "just add the locking". Pavel H. actually produced patch doing so... From: Pavel Herrmann To: Marek Vasut >From 14063b017123233a8b56d6706a9ff046a791eaf4 Mon Sep 17 00:00:00 2001 From: Pavel Herrmann Date: Mon, 16 May 2011 14:18:18 +0200 Subject: [PATCH] Fix NULL pointer exception in max1111 Signed-off-by: Pavel Herrmann --- drivers/hwmon/max1111.c | 16 ++++++++++++++++ 1 files changed, 16 insertions(+), 0 deletions(-) diff --git a/drivers/hwmon/max1111.c b/drivers/hwmon/max1111.c index 12a54aa..7be50e5 100644 --- a/drivers/hwmon/max1111.c +++ b/drivers/hwmon/max1111.c @@ -40,6 +40,7 @@ struct max1111_data { struct spi_transfer xfer[2]; uint8_t *tx_buf; uint8_t *rx_buf; + struct mutex msg_lock_mutex; }; static int max1111_read(struct device *dev, int channel) @@ -48,6 +49,11 @@ static int max1111_read(struct device *dev, int channel) uint8_t v1, v2; int err; + /* spi_sync requires data not to be freed before function returns + * for static data, any access is dangerous, use locks + */ + mutex_lock(&data->msg_lock_mutex); + data->tx_buf[0] = (channel << MAX1111_CTRL_SEL_SH) | MAX1111_CTRL_PD0 | MAX1111_CTRL_PD1 | MAX1111_CTRL_SGL | MAX1111_CTRL_UNI | MAX1111_CTRL_STR; @@ -55,12 +61,15 @@ static int max1111_read(struct device *dev, int channel) err = spi_sync(data->spi, &data->msg); if (err < 0) { dev_err(dev, "spi_sync failed with %d\n", err); + mutex_unlock(&data->msg_lock_mutex); return err; } v1 = data->rx_buf[0]; v2 = data->rx_buf[1]; + mutex_unlock(&data->msg_lock_mutex); + if ((v1 & 0xc0) || (v2 & 0x3f)) return -EINVAL; @@ -138,6 +147,8 @@ static int setup_transfer(struct max1111_data *data) return -ENOMEM; } + mutex_lock(&data->msg_lock_mutex); + m = &data->msg; x = &data->xfer[0]; @@ -152,6 +163,8 @@ static int setup_transfer(struct max1111_data *data) x->len = 2; spi_message_add_tail(x, m); + mutex_unlock(&data->msg_lock_mutex); + return 0; } @@ -172,6 +185,8 @@ static int __devinit max1111_probe(struct spi_device *spi) return -ENOMEM; } + mutex_init(&data->msg_lock_mutex); + err = setup_transfer(data); if (err) goto err_free_data; @@ -213,6 +228,7 @@ static int __devexit max1111_remove(struct spi_device *spi) hwmon_device_unregister(data->hwmon_dev); sysfs_remove_group(&spi->dev.kobj, &max1111_attr_group); + mutex_destroy(data->msg_lock_mutex); kfree(data->rx_buf); kfree(data->tx_buf); kfree(data);