From patchwork Thu May 19 22:13:20 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pavel Herrmann X-Patchwork-Id: 800482 Received: from bombadil.infradead.org (bombadil.infradead.org [18.85.46.34]) by demeter2.kernel.org (8.14.4/8.14.3) with ESMTP id p4JMFEvS032758 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Thu, 19 May 2011 22:15:41 GMT Received: from canuck.infradead.org ([2001:4978:20e::1]) by bombadil.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1QNBTI-00021T-2a; Thu, 19 May 2011 22:13:32 +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 1QNBTG-0001Xi-Ad; Thu, 19 May 2011 22:13:30 +0000 Received: from mail-fx0-f49.google.com ([209.85.161.49]) by canuck.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1QNBTC-0001XP-O6 for linux-arm-kernel@lists.infradead.org; Thu, 19 May 2011 22:13:27 +0000 Received: by fxm14 with SMTP id 14so3036768fxm.36 for ; Thu, 19 May 2011 15:13:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:from:to:subject:date:user-agent:cc:references :in-reply-to:mime-version:content-type:content-transfer-encoding :message-id; bh=i1MZqrzjo1eqDRF7rxVyGV/x+ujsroqhoSBFbV0L35k=; b=esYbACVhZ7gc21oWjS/PIEEAT+PWoKQbrLBqRKKIsy0+MGAHeZc6nStSXRH5Tkpxdj mXFaXgpsCvJSUFAFDqXCZwHHHgWKXfKsfLAwVZ1keW4yd/pfh0hHPsGnBhgozs6MpJKO Yv6YUce7EOOqNyFT8Lzk87T8+u6yxEF9NjBvM= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=from:to:subject:date:user-agent:cc:references:in-reply-to :mime-version:content-type:content-transfer-encoding:message-id; b=BkJvfHKroaPBLUUI7FKA04f4NOohvDVcZoIKLRFgK+2iMDnEHkhsa3PrxUnfRyJU6p OzCTdje/W7lQDOFYl9GKgR4A92jYSYQokY9UqISTUoSQhzLkIGN90+t9LRzfTRnrtyoQ S7oftu34W6PJd0RUhaKyklk0fYUtfl7HyjgKQ= Received: by 10.223.60.7 with SMTP id n7mr31938fah.51.1305843203983; Thu, 19 May 2011 15:13:23 -0700 (PDT) Received: from merom.localnet ([92.62.226.228]) by mx.google.com with ESMTPS id d16sm1224984faa.12.2011.05.19.15.13.22 (version=SSLv3 cipher=OTHER); Thu, 19 May 2011 15:13:23 -0700 (PDT) From: Pavel Herrmann To: "Russell King - ARM Linux" Subject: Re: [PATCH] MAX1111: Fix race condition causing NULL pointer exception Date: Fri, 20 May 2011 00:13:20 +0200 User-Agent: KMail/1.13.6 (Linux/2.6.34-gentoo; KDE/4.6.2; x86_64; ; ) References: <1305731918-20164-1-git-send-email-morpheus.ibis@gmail.com> <201105191451.40416.morpheus.ibis@gmail.com> <20110519193121.GC7445@n2100.arm.linux.org.uk> In-Reply-To: <20110519193121.GC7445@n2100.arm.linux.org.uk> MIME-Version: 1.0 Message-Id: <201105200013.20380.morpheus.ibis@gmail.com> X-CRM114-Version: 20090807-BlameThorstenAndJenny ( TRE 0.7.6 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20110519_181327_080407_D8788A14 X-CRM114-Status: GOOD ( 23.17 ) X-Spam-Score: -0.8 (/) X-Spam-Report: SpamAssassin version 3.3.1 on canuck.infradead.org summary: Content analysis details: (-0.8 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.161.49 listed in list.dnswl.org] 0.0 FREEMAIL_FROM Sender email is freemail (morpheus.ibis[at]gmail.com) -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature 0.0 RFC_ABUSE_POST Both abuse and postmaster missing on sender domain Cc: eric.y.miao@gmail.com, metan@ucw.cz, utx@penguin.cz, marek.vasut@gmail.com, Pavel Machek , 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 (demeter2.kernel.org [140.211.167.43]); Thu, 19 May 2011 22:15:41 +0000 (UTC) On Thursday, May 19, 2011 09:31:21 PM Russell King - ARM Linux wrote: > I'm not sure that this is right. Taking the lock around spi_sync() ensures > that two concurrent spi_sync()s can't happen in parallel, but with this you > could end up with another happening as soon as this lock is released - > before you've accessed the data which was transferred. > > I think you want to hold the mutex at the point you setup the data to be > transferred, do the transfer, and then release the mutex once you've read > the results of the transfer. oh no, not again... this was the earliest version, not the cleaned-up one (notice the lock in setup-transfer, which I said was unnecessary) here is the cleaner (newest) version: From bd55d6b18fa4fcb884980825b43b43df01767149 Mon Sep 17 00:00:00 2001 From: Pavel Herrmann Date: Mon, 16 May 2011 14:18:18 +0200 Subject: [PATCH] MAX1111: Fix Race condition causing NULL pointer exception 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 mutex to prevent multiple access Signed-off-by: Pavel Herrmann --- drivers/hwmon/max1111.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-) kfree(data); diff --git a/drivers/hwmon/max1111.c b/drivers/hwmon/max1111.c index 12a54aa..d872f57 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; @@ -176,6 +185,8 @@ static int __devinit max1111_probe(struct spi_device *spi) if (err) goto err_free_data; + mutex_init(&data->msg_lock_mutex); + data->spi = spi; spi_set_drvdata(spi, data); @@ -213,6 +224,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);