Re: [PATCH 10/22] gpiolib: cdev: fix minor race in GET_LINEINFO_WATCH

[Kent Gibson]

On Thu, Jun 25, 2020 at 11:44:21AM +0300, Andy Shevchenko wrote:
> On Thu, Jun 25, 2020 at 1:58 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > On Wed, Jun 24, 2020 at 11:57:14PM +0800, Kent Gibson wrote:
> > > On Wed, Jun 24, 2020 at 05:46:33PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Jun 23, 2020 at 7:03 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> 
> ...
> 
> > > > I stumbled over this myself, but...
> > > >
> > > > > -               if (test_bit(hwgpio, gcdev->watched_lines))
> > > > > +               if (test_and_set_bit(hwgpio, gcdev->watched_lines))
> > > > >                         return -EBUSY;
> > > > >
> > > > >                 gpio_desc_to_lineinfo(desc, &lineinfo);
> > > > > @@ -897,7 +897,6 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > > > >                 if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
> > > > >                         return -EFAULT;
> > > > >
> > > > > -               set_bit(hwgpio, gcdev->watched_lines);
> > > > >                 return 0;
> > > >
> > > > ...I think it's not an equivalent despite races involved. If you set
> > > > bit and return error code, you will have the wrong state.
> 
> > Perhaps you are referring to the case where the copy_to_user fails?
> 
> Yes.
> 
> > To be honest I considered that to be so unlikely that I ignored it.
> > Is there a relevant failure mode that I'm missing?
> 
> The traditional question for such cases is "what can possibly go wrong?"
> I wouldn't underestimate the probability of failure.
> 

The worst case is the watch is enabled and the userspace gets an
EFAULT so it thinks it failed.  If userspace retries then they get
EBUSY, so userspace accounting gets muddled.

We can clear the watch bit if the copy_to_user fails - before
returning the EFAULT. Would that be satisfactory?

Back to the failure, is it possible for the copy_to_user fail here,
given that the corresponding copy_from_user has succeeded?
If so, can that be manually triggered for test purposes?

Cheers,
Kent.