From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752942AbdJGR7d (ORCPT ); Sat, 7 Oct 2017 13:59:33 -0400 Received: from mail.ispras.ru ([83.149.199.45]:56384 "EHLO mail.ispras.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751279AbdJGR7c (ORCPT ); Sat, 7 Oct 2017 13:59:32 -0400 Subject: Re: [PATCH] w1: keep balance of mutex locks and refcnts To: Evgeniy Polyakov , Greg Kroah-Hartman Cc: "linux-kernel@vger.kernel.org" , "ldv-project@linuxtesting.org" References: <1506716604-23455-1-git-send-email-khoroshilov@ispras.ru> <152531506837309@web6g.yandex.ru> From: Alexey Khoroshilov Message-ID: Date: Sat, 7 Oct 2017 20:59:27 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <152531506837309@web6g.yandex.ru> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Evgeniy, mutex_lock() and atomic_inc() are not nested currently: ret = mutex_lock_interruptible(&dev->bus_mutex); ... atomic_inc(THERM_REFCNT(family_data)); ... mutex_unlock(&dev->bus_mutex); ... atomic_dec(THERM_REFCNT(family_data)); As a result, error handling without returns will be still quite messy. Is it possible to switch to a nested variant: mutex_lock-atomic_inc-atomic_dec-mutex_unlock or atomic_inc-mutex_lock-mutex_unlock-atomic_dec ? -- Alexey On 01.10.2017 08:55, Evgeniy Polyakov wrote: > Hi Alex > > 29.09.2017, 23:23, "Alexey Khoroshilov" : >> w1_therm_eeprom() and w1_DS18B20_precision() decrement THERM_REFCNT >> on error paths, while they did not increment it yet. >> >> read_therm() unlocks bus mutex on some error paths, >> while it is not acquired. >> >> The patch makes sure all the functions keep the balance in usage of >> the mutex and the THERM_REFCNT. >> >> Found by Linux Driver Verification project (linuxtesting.org). > > Yes, this looks like a bug, thanks for finding it! > > Please update your patch to use single exit point and not a mix of returns in the body of the function. > >>          ret = mutex_lock_interruptible(&dev->bus_mutex); >>          if (ret != 0) >> - goto post_unlock; >> + return ret; >> >>          if (!sl->family_data) { >> - ret = -ENODEV; >> - goto pre_unlock; >> + mutex_unlock(&dev->bus_mutex); >> + return -ENODEV; >>          } >