Re: [PATCH 10/22] gpiolib: cdev: fix minor race in GET_LINEINFO_WATCH
On Thu, Jun 25, 2020 at 12:23:49PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 25, 2020 at 12:13 PM Kent Gibson wrote:
> > On Thu, Jun 25, 2020 at 11:44:21AM +0300, Andy Shevchenko wrote:
> > > On Thu, Jun 25, 2020 at 1:58 AM Kent Gibson wrote:
> > > > On Wed, Jun 24, 2020 at 11:57:14PM +0800, Kent Gibson wrote:
>
> ...
>
> > > > 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?
>
> Perhaps. I didn't check that scenario.
>
To be clear I'm suggesting this:
gpio_desc_to_lineinfo(desc, );
- if (copy_to_user(ip, , sizeof(lineinfo)))
+ if (copy_to_user(ip, , sizeof(lineinfo))) {
+ clear_bit(lineinfo.offset, gcdev->watched_lines);
return -EFAULT;
+ }
That undoes the set, returning the watch state to what it was before the
call.
Cheers,
Kent.
Re: [PATCH 10/22] gpiolib: cdev: fix minor race in GET_LINEINFO_WATCH
On Thu, Jun 25, 2020 at 12:13 PM Kent Gibson wrote: > On Thu, Jun 25, 2020 at 11:44:21AM +0300, Andy Shevchenko wrote: > > On Thu, Jun 25, 2020 at 1:58 AM Kent Gibson wrote: > > > On Wed, Jun 24, 2020 at 11:57:14PM +0800, Kent Gibson wrote: ... > > > 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? Perhaps. I didn't check that scenario. > Back to the failure, is it possible for the copy_to_user fail here, > given that the corresponding copy_from_user has succeeded? Of course. The general rule is if on SMP system you have not strongly serialized sequence of calls (means no preemption, no interrupts, etc) anything can happen in between. > If so, can that be manually triggered for test purposes? Unfortunately not an expert in mm, no idea, sorry. -- With Best Regards, Andy Shevchenko
Re: [PATCH 10/22] gpiolib: cdev: fix minor race in GET_LINEINFO_WATCH
On Thu, Jun 25, 2020 at 11:44:21AM +0300, Andy Shevchenko wrote: > On Thu, Jun 25, 2020 at 1:58 AM Kent Gibson 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 > > > > 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, ); > > > > > @@ -897,7 +897,6 @@ static long gpio_ioctl(struct file *file, > > > > > unsigned int cmd, unsigned long arg) > > > > > if (copy_to_user(ip, , 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.
Re: [PATCH 10/22] gpiolib: cdev: fix minor race in GET_LINEINFO_WATCH
On Thu, Jun 25, 2020 at 1:58 AM Kent Gibson 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 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, ); > > > > @@ -897,7 +897,6 @@ static long gpio_ioctl(struct file *file, unsigned > > > > int cmd, unsigned long arg) > > > > if (copy_to_user(ip, , 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. -- With Best Regards, Andy Shevchenko
Re: [PATCH 10/22] gpiolib: cdev: fix minor race in GET_LINEINFO_WATCH
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 wrote: > > > > > > Merge separate usage of test_bit/set_bit into test_and_set_bit to remove > > > the possibility of a race between the test and set. > > > > > > Similarly test_bit and clear_bit. > > > > > > In the existing code it is possible for two threads to race past the > > > test_bit and then set or clear the watch bit, and neither return EBUSY. > > > > 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, ); > > > @@ -897,7 +897,6 @@ static long gpio_ioctl(struct file *file, unsigned > > > int cmd, unsigned long arg) > > > if (copy_to_user(ip, , 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. > > > > Not quite sure what you mean. There is only an error if the bit is > already set, so you've changed nothing. > > And the watched state is not part of the lineinfo, so the state returned is > the same either way. > Perhaps you are referring to the case where the copy_to_user fails? To be honest I considered that to be so unlikely that I ignored it. Is there a relevant failure mode that I'm missing? Cheers, Kent.
Re: [PATCH 10/22] gpiolib: cdev: fix minor race in GET_LINEINFO_WATCH
On Wed, Jun 24, 2020 at 05:46:33PM +0300, Andy Shevchenko wrote: > On Tue, Jun 23, 2020 at 7:03 AM Kent Gibson wrote: > > > > Merge separate usage of test_bit/set_bit into test_and_set_bit to remove > > the possibility of a race between the test and set. > > > > Similarly test_bit and clear_bit. > > > > In the existing code it is possible for two threads to race past the > > test_bit and then set or clear the watch bit, and neither return EBUSY. > > 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, ); > > @@ -897,7 +897,6 @@ static long gpio_ioctl(struct file *file, unsigned int > > cmd, unsigned long arg) > > if (copy_to_user(ip, , 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. > Not quite sure what you mean. There is only an error if the bit is already set, so you've changed nothing. And the watched state is not part of the lineinfo, so the state returned is the same either way. Cheers, Kent.
Re: [PATCH 10/22] gpiolib: cdev: fix minor race in GET_LINEINFO_WATCH
On Tue, Jun 23, 2020 at 7:03 AM Kent Gibson wrote: > > Merge separate usage of test_bit/set_bit into test_and_set_bit to remove > the possibility of a race between the test and set. > > Similarly test_bit and clear_bit. > > In the existing code it is possible for two threads to race past the > test_bit and then set or clear the watch bit, and neither return EBUSY. 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, ); > @@ -897,7 +897,6 @@ static long gpio_ioctl(struct file *file, unsigned int > cmd, unsigned long arg) > if (copy_to_user(ip, , 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. ... > - if (!test_bit(hwgpio, gcdev->watched_lines)) > + if (!test_and_clear_bit(hwgpio, gcdev->watched_lines)) > return -EBUSY; > > - clear_bit(hwgpio, gcdev->watched_lines); > return 0; OTOH, this is okay as long as we have no code in between. So, I really prefer something better to do such checks. (Alas, I can't come up with a proposal right now) -- With Best Regards, Andy Shevchenko
Re: [PATCH 10/22] gpiolib: cdev: fix minor race in GET_LINEINFO_WATCH
wt., 23 cze 2020 o 06:02 Kent Gibson napisał(a): > > Merge separate usage of test_bit/set_bit into test_and_set_bit to remove > the possibility of a race between the test and set. > > Similarly test_bit and clear_bit. > > In the existing code it is possible for two threads to race past the > test_bit and then set or clear the watch bit, and neither return EBUSY. > > Signed-off-by: Kent Gibson > Reviewed-by: Bartosz Golaszewski
[PATCH 10/22] gpiolib: cdev: fix minor race in GET_LINEINFO_WATCH
Merge separate usage of test_bit/set_bit into test_and_set_bit to remove
the possibility of a race between the test and set.
Similarly test_bit and clear_bit.
In the existing code it is possible for two threads to race past the
test_bit and then set or clear the watch bit, and neither return EBUSY.
Signed-off-by: Kent Gibson
---
drivers/gpio/gpiolib-cdev.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 5f5b715ed7f7..a727709b24a9 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -889,7 +889,7 @@ static long gpio_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
hwgpio = gpio_chip_hwgpio(desc);
- if (test_bit(hwgpio, gcdev->watched_lines))
+ if (test_and_set_bit(hwgpio, gcdev->watched_lines))
return -EBUSY;
gpio_desc_to_lineinfo(desc, );
@@ -897,7 +897,6 @@ static long gpio_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
if (copy_to_user(ip, , sizeof(lineinfo)))
return -EFAULT;
- set_bit(hwgpio, gcdev->watched_lines);
return 0;
} else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) {
if (copy_from_user(, ip, sizeof(offset)))
@@ -909,10 +908,9 @@ static long gpio_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
hwgpio = gpio_chip_hwgpio(desc);
- if (!test_bit(hwgpio, gcdev->watched_lines))
+ if (!test_and_clear_bit(hwgpio, gcdev->watched_lines))
return -EBUSY;
- clear_bit(hwgpio, gcdev->watched_lines);
return 0;
}
return -EINVAL;
--
2.27.0
