From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756046AbbCSAJX (ORCPT ); Wed, 18 Mar 2015 20:09:23 -0400 Received: from SpacedOut.fries.net ([67.64.210.234]:53296 "EHLO SpacedOut.fries.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752304AbbCSAJV (ORCPT ); Wed, 18 Mar 2015 20:09:21 -0400 Date: Wed, 18 Mar 2015 19:09:08 -0500 From: David Fries To: Evgeniy Polyakov Cc: Thorsten Bschorr , Jonathan ALIBERT , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH] Avoid null-pointer access in w1/slaves/w1_therm Message-ID: <20150319000908.GG21067@spacedout.fries.net> References: <20150302001713.GA6151@spacedout.fries.net> <4333121425483401@web21m.yandex.ru> <20150308211449.GG11991@spacedout.fries.net> <20150309230924.GJ11991@spacedout.fries.net> <2663771425995520@web30h.yandex.ru> <20150312005408.GD23778@spacedout.fries.net> <2283421426366516@web2o.yandex.ru> <20150318042037.GE21067@spacedout.fries.net> <702441426691933@web2g.yandex.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <702441426691933@web2g.yandex.ru> User-Agent: Mutt/1.5.23 (2014-03-12) X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.5.11 (SpacedOut.fries.net [127.0.0.1]); Wed, 18 Mar 2015 19:09:09 -0500 (CDT) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Mar 18, 2015 at 06:18:53PM +0300, Evgeniy Polyakov wrote: > Hi > > 18.03.2015, 07:20, "David Fries" : > >  static void w1_therm_remove_slave(struct w1_slave *sl) > >  { > > + int refcnt = atomic_sub_return(1, THERM_REFCNT(sl->family_data)); > > + while(refcnt) { > > + msleep(1000); > > + refcnt = atomic_read(THERM_REFCNT(sl->family_data)); > > + } > >          kfree(sl->family_data); > >          sl->family_data = NULL; > >  } > > Can we replace this whole atomic manipulations with kref_t and free family data in the place > which actually drops reference counter to zero? > > I.e. we return from remove_slave() function potentially leaving family data floating around, it will be freed > when the last user drops the reference. There is still a race between increasing reference when starting > reading and removing slave device, i.e. one starts reading, while attached slave device is being removed, > but that's a different problem. With the two while loops I posted, I see with two clients reading w1_slave, the other command to remove a slave gets permanently stuck in w1_therm_remove_slave, which keeps the slave around while the clients continue to read. I wouldn't predict things going better by keeping family_data around longer, the slave data would still go away with readers around. -- David Fries PGP pub CB1EE8F0 http://fries.net/~david/