Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753126AbbCACR5 (ORCPT ); Sat, 28 Feb 2015 21:17:57 -0500 Received: from SpacedOut.fries.net ([67.64.210.234]:53566 "EHLO SpacedOut.fries.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752411AbbCACR4 (ORCPT ); Sat, 28 Feb 2015 21:17:56 -0500 Date: Sat, 28 Feb 2015 20:17:45 -0600 From: David Fries To: ??????? ??????? Cc: "Thorsten.Bschorr" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH] Avoid null-pointer access in w1/slaves/w1_therm Message-ID: <20150301021744.GW6151@spacedout.fries.net> References: <54F02E22.5050901@bschorr.de> <20150228201737.GU6151@spacedout.fries.net> <369891425174502@web4m.yandex.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <369891425174502@web4m.yandex.ru> 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]); Sat, 28 Feb 2015 20:17:46 -0600 (CST) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1826 Lines: 57 On Sun, Mar 01, 2015 at 04:48:22AM +0300, ??????? ??????? wrote: > Hi everyone > > 28.02.2015, 23:18, "David Fries" : > > Thanks for preparing the patch, it looks like it will go another > > round, but that happens to everyone. > > Patch itself doesn't solve the problem, it only adds a workaround. > There is a reference counter issue - reading data via sysfs only holds > a reference to device, w1 slave structure will be deleted by w1 search core. > > I believe it only accidentally doesn't crash because of kernel memory allocator > hasn't yet reclaimed memory. Good point, it does look like that's the case. w1_unref_slave calls w1_family_notify(BUS_NOTIFY_DEL_DEVICE, sl); which calls sl->family->fops->remove_slave(sl); which kfree and sl->family_data = NULL; In that case what do you think about in w1_slave_show after getting bus_mutex, atomic_inc(&sl->refcnt); then add w1_unref_slave(sl); in each return path, or do a goto and consolidate them. 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) { -- David Fries PGP pub CB1EE8F0 http://fries.net/~david/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/