Messages in this thread | | | Subject | Re: [PATCH] w1: keep balance of mutex locks and refcnts | From | Alexey Khoroshilov <> | Date | Sat, 7 Oct 2017 20:59:27 +0300 |
| |
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" <khoroshilov@ispras.ru>: >> 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; >> } >
| |