linux-kernel - Re: [PATCH 7/7] iio: light: tsl2583: fix concurrency issue in taos_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <90d80d86-7e1b-ea97-6801-174a6cf438e8@kernel.org>
Date:   Sat, 22 Oct 2016 18:32:09 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Brian Masney <masneyb@...tation.org>
Cc:     knaack.h@....de, lars@...afoo.de, pmeerw@...erw.net,
        gregkh@...uxfoundation.org, linux-iio@...r.kernel.org,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 7/7] iio: light: tsl2583: fix concurrency issue in
 taos_get_lux()

On 19/10/16 11:32, Brian Masney wrote:
> taos_get_lux() calls mutex_trylock(). If the lock could not be acquired,
> then chip->als_cur_info.lux is returned. The issue is that this value
> is updated while the mutex is held and could cause a half written value
> to be returned to the caller. This patch changes the call to
> mutex_trylock() with mutex_lock().
> 
> Signed-off-by: Brian Masney <masneyb@...tation.org>
I'd go with the simple approach you have here.

If someone cares enough they can figure out the complex solution.  Probably
should be pushed off to userspace.  It's not unusual for devices to take
'a little while' to respond to sysfs reads.

Jonathan
> ---
> This is the most controversial change in my patch set. There are two
> other possible solutions that I could envision to work around this
> issue:
> 
> 1) Return -EBUSY and make the caller responsible for backing off
> 2) Change this driver to use RCU instead of a mutex
> 
>  drivers/staging/iio/light/tsl2583.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
> index 47656ae..c4d2e3a 100644
> --- a/drivers/staging/iio/light/tsl2583.c
> +++ b/drivers/staging/iio/light/tsl2583.c
> @@ -206,10 +206,7 @@ static int taos_get_lux(struct iio_dev *indio_dev)
>  	u32 ch0lux = 0;
>  	u32 ch1lux = 0;
>  
> -	if (mutex_trylock(&chip->als_mutex) == 0) {
> -		dev_info(&chip->client->dev, "taos_get_lux device is busy\n");
> -		return chip->als_cur_info.lux; /* busy, so return LAST VALUE */
> -	}
> +	mutex_lock(&chip->als_mutex);
>  
>  	if (chip->taos_chip_status != TSL258X_CHIP_WORKING) {
>  		/* device is not enabled */
>