From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753660AbcICPZW (ORCPT ); Sat, 3 Sep 2016 11:25:22 -0400 Received: from saturn.retrosnub.co.uk ([178.18.118.26]:44872 "EHLO saturn.retrosnub.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752852AbcICPZU (ORCPT ); Sat, 3 Sep 2016 11:25:20 -0400 Subject: Re: A potential race in drivers/iio/adc/vf610_adc.ko To: Pavel Andrianov References: <8ad6537e-718a-4c5d-4f51-958e585b3727@ispras.ru> Cc: Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald-Stadler , linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, Vaishali Thakkar , ldv-project@linuxtesting.org From: Jonathan Cameron Message-ID: Date: Sat, 3 Sep 2016 16:23:11 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <8ad6537e-718a-4c5d-4f51-958e585b3727@ispras.ru> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/09/16 09:05, Pavel Andrianov wrote: > > Hi! Hi Pavel, > > There is a potential race in drivers/iio/adc/vf610_adc.ko. Handlers > vf610_set_conversion_mode and vf610_write_raw are called via > device_attibute interface, but they are related to different > attributes, so may be executed in parallel. vf610_set_conversion_mode > acquires the mutex indio_dev->mlock, and vf610_write_raw does not. > Thus updating the structure 'info' may be performed simultaneously. > > Should vf610_write_raw also acquire the same mutex indio_dev->mlock? > As Alison observed, mlock is not a general purpose lock for use by drivers. It's there to prevent state changes between direct reads (polled) and buffered/triggered reads (pushed). The write raw simply sets the sampling frequency. That's not a problem whilst buffered capture is running or otherwise. Interesting question of whether changing mode causes any trouble as well. It's possible something is undefined in the hardware during a mode change... Anyhow, that covers mlock. Next question: Is there a race condition in general? Yes there definitely is as we have read modify write cycles on VF610_REG_ADC_CFG in both paths. So what is needed is a local lock to protect these accesses. Whilst in theory mlock could be used it should not be as it has a clearly stated purpose and using it for other purposes makes for much fiddlier and harder to read code! (as an aside IIRC there is no locking in sysfs attributes to prevent a single attribute being read twice at the same time.) Jonathan