linux-kernel - Re: [PATCH] iio: adc: exynos: do not rely on 'users' counter in ISR

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]

Date:   Mon, 5 Oct 2020 10:36:36 -0700
From:   dmitry.torokhov@...il.com
To:     Michał Mirosław <mirq-linux@...e.qmqm.pl>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Kukjin Kim <kgene@...nel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Andrzej Pietrasiewicz <andrzej.p@...labora.com>,
        linux-iio@...r.kernel.org, linux-samsung-soc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: adc: exynos: do not rely on 'users' counter in ISR

Hi Michał,

On Mon, Oct 05, 2020 at 01:09:08PM +0200, Michał Mirosław wrote:
> On Sun, Oct 04, 2020 at 10:24:20PM -0700, dmitry.torokhov@...il.com wrote:
> > The order in which 'users' counter is decremented vs calling drivers'
> > close() method is implementation specific, and we should not rely on
> > it. Let's introduce driver private flag and use it to signal ISR
> > to exit when device is being closed.
> > 
> > This has a side-effect of fixing issue of accessing inut->users
> > outside of input->mutex protection.
> > 
> > Reported-by: Andrzej Pietrasiewicz <andrzej.p@...labora.com>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
> > ---
> >  drivers/iio/adc/exynos_adc.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> > index 22131a677445..7eb2a5df6e98 100644
> > --- a/drivers/iio/adc/exynos_adc.c
> > +++ b/drivers/iio/adc/exynos_adc.c
> > @@ -135,6 +135,8 @@ struct exynos_adc {
> >  	u32			value;
> >  	unsigned int            version;
> >  
> > +	bool			ts_enabled;
> > +
> >  	bool			read_ts;
> >  	u32			ts_x;
> >  	u32			ts_y;
> > @@ -633,7 +635,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
> >  	bool pressed;
> >  	int ret;
> >  
> > -	while (info->input->users) {
> > +	while (info->ts_enabled) {
> >  		ret = exynos_read_s3c64xx_ts(dev, &x, &y);
> >  		if (ret == -ETIMEDOUT)
> >  			break;
> > @@ -712,6 +714,8 @@ static int exynos_adc_ts_open(struct input_dev *dev)
> >  {
> >  	struct exynos_adc *info = input_get_drvdata(dev);
> >  
> > +	info->ts_enabled = true;
> > +	mb();
> >  	enable_irq(info->tsirq);
> >  
> >  	return 0;
> > @@ -721,6 +725,8 @@ static void exynos_adc_ts_close(struct input_dev *dev)
> >  {
> >  	struct exynos_adc *info = input_get_drvdata(dev);
> >  
> > +	info->ts_enabled = false;
> > +	mb();
> >  	disable_irq(info->tsirq);
> 
> This should be WRITE_ONCE paired with READ_ONCE in the ISR.

Why? I can see that maybe full memory barrier is too heavy when we set
the flag to true, but the only requirement is for the flag to be set
before we disable IRQ, so any additional guarantees provided by
WRITE_ONCE are not needed. On the read side we want the flag to be
noticed eventually, and there is no additional dependencies on the data,
so it is unclear what READ_ONCE() will give us here.

> 
> But is the check really needed? I see that this is to break waiting for
> a touch release event, so I would assume this shouldn't wait forever
> (unless the hardware is buggy)

It is not hardware, it is user. Do you want to delay indefinitely
close() just because user has a finger on the touchscreen?

> and breaking the loop will desync touch
> state (I would guess this would be noticable by next user).

Upon next open driver will service the interrupt and provide new set of
touch coordinates. Userspace is supposed to query current state of
device when opening it before starting processing events. Or you are
concerned about some other state?

In any case, this is current driver behavior and if it needs to be
changed it is a topic for a separate patch.

Thanks.

-- 
Dmitry