From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753293AbbCBARZ (ORCPT ); Sun, 1 Mar 2015 19:17:25 -0500 Received: from SpacedOut.fries.net ([67.64.210.234]:53618 "EHLO SpacedOut.fries.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751781AbbCBARY (ORCPT ); Sun, 1 Mar 2015 19:17:24 -0500 Date: Sun, 1 Mar 2015 18:17:13 -0600 From: David Fries To: Thorsten Bschorr Cc: Evgeniy Polyakov , linux-kernel@vger.kernel.org Subject: Re: [PATCH] Avoid null-pointer access in w1/slaves/w1_therm Message-ID: <20150302001713.GA6151@spacedout.fries.net> References: <54F02E22.5050901@bschorr.de> <20150228201737.GU6151@spacedout.fries.net> <369891425174502@web4m.yandex.ru> <20150301021744.GW6151@spacedout.fries.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.3.9 (SpacedOut.fries.net [127.0.0.1]); Sun, 01 Mar 2015 18:17:15 -0600 (CST) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Mar 01, 2015 at 02:04:53PM +0100, Thorsten Bschorr wrote: > Hi David, > > thanks for your feedback on my first patch, I wasn't aware of checkpatch.pl. > > Initially, I had just if-ed the usage of family-data, which did not > look that nice. I was referring to this proof-of-concept workaround in > my initial bug report. > > The patch I've submitted is different from my proof-of-concept > workaround. Not unlocking the bus before returning clearly is an > error, I did not extensively test this patch. > > > > or just increment it while sleeping, which is when it's needed, which > > also looks simpler. > > > > if (external_power) { > > + int refcnt; > > mutex_unlock(&dev->bus_mutex); > > > > + /* prevent the slave from going away */ > > + atomic_inc(&sl->refcnt); > > sleep_rem = msleep_interruptible(tm); > > + refcnt = w1_unref_slave(sl); > > - if (sleep_rem != 0) > > + if (sleep_rem != 0 || !refcnt) > > return -EINTR; > > > > i = mutex_lock_interruptible(&dev->bus_mutex); > > if (i != 0) > > return i; > > } else if (!w1_strong_pullup) { > > > I like this better than my workaround-patch. > > One thought occurred to me when looking at this proposal: wouldn't it > be even better to increase sl->refcnt before unlocking the mutex? > I was asking myself if it is possible that the current thread gets > suspended between mutex_unlock(&dev->bus_mutex); and > atomic_inc(&sl->refcnt); thus leaving another thread the change to > unref the device? > (I'm not that familiar with linux scheduling, so my assumption might be void.) You are correct, it would be a race condition if it doesn't increment the refcnt before unlocking the mutex, and it should get the mutex before unref. Here's an updated version, I haven't even tried to compile it. What do you think Evgeniy? if (external_power) { int refcnt; /* prevent the slave from going away in sleep */ atomic_inc(&sl->refcnt); mutex_unlock(&dev->bus_mutex); sleep_rem = msleep_interruptible(tm); if (sleep_rem != 0) { w1_unref_slave(sl); return -EINTR; } i = mutex_lock_interruptible(&dev->bus_mutex); refcnt = w1_unref_slave(sl); if (i != 0) { /* failed to lock */ return i; } if (!refcnt) /* got lock, but slave went away */ mutex_unlock(&dev->bus_mutex); return -EINTR; } } else if (!w1_strong_pullup) { -- David Fries PGP pub CB1EE8F0 http://fries.net/~david/