[libgpiod][PATCH v2 0/2] fix potential discarding of events by read events

[libgpiod][PATCH v2 0/2] fix potential discarding of events by read events

[Kent Gibson]

This patch set addresses a bug where reading events can quietly discard
events.  The problem occurs any time a request made is for a number of
events that differs from the number available at the time - a number
that userspace is unaware of.

The problem is due to the read multiple always reading up to 16 events
from the kernel - independent of how many events have been requested.

The problem applies to reading both single and multiple events, as the
single event read implementation wraps the multiple.

The first patch extends test coverage to highlight the problem.
The second fixes the bug.

Changes for v2:
 - restored usleep() when creating multiple events in patch 1.

Kent Gibson (2):
  tests: event: reading test coverage extended to cover reading a subset
    of available events
  core: fix reading subset of available events

 lib/core.c          |   5 +-
 tests/tests-event.c | 157 ++++++++++++++++++++++++++++++++++++++------
 2 files changed, 141 insertions(+), 21 deletions(-)


base-commit: fc61e740fcbe3c6594295766759888c96c45bd29
-- 
2.28.0

[libgpiod][PATCH v2 1/2] tests: event: reading test coverage extended to cover reading a subset of available events

[Kent Gibson]

Add tests for gpiod_line_event_read(), including reading multiple
entries from the kernel event kfifo, and extend the existing
read_multiple_event tests to read a subset of the available events as
well as all the available events.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---

Ignore my previous usleep() comments - edge detection hangs off the intr.
It was pre-coffee, OK.

 tests/tests-event.c | 157 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 137 insertions(+), 20 deletions(-)

diff --git a/tests/tests-event.c b/tests/tests-event.c
index e323060..d9f75e4 100644
--- a/tests/tests-event.c
+++ b/tests/tests-event.c
@@ -663,13 +663,76 @@ GPIOD_TEST_CASE(invalid_fd, 0, { 8 })
 	g_assert_cmpint(errno, ==, EINVAL);
 }
 
+GPIOD_TEST_CASE(read_event, 0, { 8 })
+{
+	g_autoptr(gpiod_chip_struct) chip = NULL;
+	struct timespec ts = { 1, 0 };
+	struct gpiod_line_event ev;
+	struct gpiod_line *line;
+	gint ret;
+	unsigned int i;
+
+	chip = gpiod_chip_open(gpiod_test_chip_path(0));
+	g_assert_nonnull(chip);
+	gpiod_test_return_if_failed();
+
+	line = gpiod_chip_get_line(chip, 7);
+	g_assert_nonnull(line);
+	gpiod_test_return_if_failed();
+
+	ret = gpiod_line_request_both_edges_events_flags(line,
+		GPIOD_TEST_CONSUMER, GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP);
+	g_assert_cmpint(ret, ==, 0);
+
+	/* generate multiple events */
+	for (i = 0; i < 3; i++) {
+		gpiod_test_chip_set_pull(0, 7, i & 1);
+		usleep(10000);
+	}
+
+	/* read them individually... */
+	ret = gpiod_line_event_wait(line, &ts);
+	g_assert_cmpint(ret, ==, 1);
+	if (!ret)
+		return;
+
+	ret = gpiod_line_event_read(line, &ev);
+	g_assert_cmpint(ret, ==, 0);
+
+	g_assert_cmpint(ev.event_type, ==, GPIOD_LINE_EVENT_FALLING_EDGE);
+
+	ret = gpiod_line_event_wait(line, &ts);
+	g_assert_cmpint(ret, ==, 1);
+	if (!ret)
+		return;
+
+	ret = gpiod_line_event_read(line, &ev);
+	g_assert_cmpint(ret, ==, 0);
+
+	g_assert_cmpint(ev.event_type, ==, GPIOD_LINE_EVENT_RISING_EDGE);
+
+	ret = gpiod_line_event_wait(line, &ts);
+	g_assert_cmpint(ret, ==, 1);
+	if (!ret)
+		return;
+
+	ret = gpiod_line_event_read(line, &ev);
+	g_assert_cmpint(ret, ==, 0);
+
+	g_assert_cmpint(ev.event_type, ==, GPIOD_LINE_EVENT_FALLING_EDGE);
+
+	ret = gpiod_line_event_wait(line, &ts);
+	g_assert_cmpint(ret, ==, 0);
+}
+
 GPIOD_TEST_CASE(read_multiple_events, 0, { 8 })
 {
 	g_autoptr(gpiod_chip_struct) chip = NULL;
-	struct gpiod_line_event events[3];
+	struct gpiod_line_event events[5];
 	struct timespec ts = { 1, 0 };
 	struct gpiod_line *line;
 	gint ret;
+	unsigned int i;
 
 	chip = gpiod_chip_open(gpiod_test_chip_path(0));
 	g_assert_nonnull(chip);
@@ -682,22 +745,25 @@ GPIOD_TEST_CASE(read_multiple_events, 0, { 8 })
 	ret = gpiod_line_request_both_edges_events(line, GPIOD_TEST_CONSUMER);
 	g_assert_cmpint(ret, ==, 0);
 
-	gpiod_test_chip_set_pull(0, 4, 1);
-	/*
-	 * We sleep for a short period of time here and in other test cases
-	 * for multiple events to let the kernel service each simulated
-	 * interrupt. Otherwise we'd risk triggering an interrupt while the
-	 * previous one is still being handled.
-	 */
-	usleep(10000);
-	gpiod_test_chip_set_pull(0, 4, 0);
-	usleep(10000);
-	gpiod_test_chip_set_pull(0, 4, 1);
-	usleep(10000);
+	/* generate multiple events */
+	for (i = 0; i < 7; i++) {
+		gpiod_test_chip_set_pull(0, 4, !(i & 1));
+		/*
+		 * We sleep for a short period of time here and in other
+		 * test cases for multiple events to let the kernel service
+		 * each simulated interrupt. Otherwise we'd risk triggering
+		 * an interrupt while the previous one is still being
+		 * handled.
+		 */
+		usleep(10000);
+	}
 
 	ret = gpiod_line_event_wait(line, &ts);
 	g_assert_cmpint(ret, ==, 1);
+	if (!ret)
+		return;
 
+	/* read a chunk */
 	ret = gpiod_line_event_read_multiple(line, events, 3);
 	g_assert_cmpint(ret, ==, 3);
 
@@ -707,15 +773,40 @@ GPIOD_TEST_CASE(read_multiple_events, 0, { 8 })
 			GPIOD_LINE_EVENT_FALLING_EDGE);
 	g_assert_cmpint(events[2].event_type, ==,
 			GPIOD_LINE_EVENT_RISING_EDGE);
+
+	ret = gpiod_line_event_wait(line, &ts);
+	g_assert_cmpint(ret, ==, 1);
+	if (!ret)
+		return;
+
+	/*
+	 * read the remainder
+	 * - note the attempt to read more than are available
+	 */
+	ret = gpiod_line_event_read_multiple(line, events, 5);
+	g_assert_cmpint(ret, ==, 4);
+
+	g_assert_cmpint(events[0].event_type, ==,
+			GPIOD_LINE_EVENT_FALLING_EDGE);
+	g_assert_cmpint(events[1].event_type, ==,
+			GPIOD_LINE_EVENT_RISING_EDGE);
+	g_assert_cmpint(events[2].event_type, ==,
+			GPIOD_LINE_EVENT_FALLING_EDGE);
+	g_assert_cmpint(events[3].event_type, ==,
+			GPIOD_LINE_EVENT_RISING_EDGE);
+
+	ret = gpiod_line_event_wait(line, &ts);
+	g_assert_cmpint(ret, ==, 0);
 }
 
 GPIOD_TEST_CASE(read_multiple_events_fd, 0, { 8 })
 {
 	g_autoptr(gpiod_chip_struct) chip = NULL;
-	struct gpiod_line_event events[3];
+	struct gpiod_line_event events[5];
 	struct timespec ts = { 1, 0 };
 	struct gpiod_line *line;
 	gint ret, fd;
+	unsigned int i;
 
 	chip = gpiod_chip_open(gpiod_test_chip_path(0));
 	g_assert_nonnull(chip);
@@ -728,19 +819,21 @@ GPIOD_TEST_CASE(read_multiple_events_fd, 0, { 8 })
 	ret = gpiod_line_request_both_edges_events(line, GPIOD_TEST_CONSUMER);
 	g_assert_cmpint(ret, ==, 0);
 
-	gpiod_test_chip_set_pull(0, 4, 1);
-	usleep(10000);
-	gpiod_test_chip_set_pull(0, 4, 0);
-	usleep(10000);
-	gpiod_test_chip_set_pull(0, 4, 1);
-	usleep(10000);
+	/* generate multiple events */
+	for (i = 0; i < 7; i++) {
+		gpiod_test_chip_set_pull(0, 4, !(i & 1));
+		usleep(10000);
+	}
 
 	ret = gpiod_line_event_wait(line, &ts);
 	g_assert_cmpint(ret, ==, 1);
+	if (!ret)
+		return;
 
 	fd = gpiod_line_event_get_fd(line);
 	g_assert_cmpint(fd, >=, 0);
 
+	/* read a chunk */
 	ret = gpiod_line_event_read_fd_multiple(fd, events, 3);
 	g_assert_cmpint(ret, ==, 3);
 
@@ -750,4 +843,28 @@ GPIOD_TEST_CASE(read_multiple_events_fd, 0, { 8 })
 			GPIOD_LINE_EVENT_FALLING_EDGE);
 	g_assert_cmpint(events[2].event_type, ==,
 			GPIOD_LINE_EVENT_RISING_EDGE);
+
+	ret = gpiod_line_event_wait(line, &ts);
+	g_assert_cmpint(ret, ==, 1);
+	if (!ret)
+		return;
+
+	/*
+	 * read the remainder
+	 * - note the attempt to read more than are available
+	 */
+	ret = gpiod_line_event_read_fd_multiple(fd, events, 5);
+	g_assert_cmpint(ret, ==, 4);
+
+	g_assert_cmpint(events[0].event_type, ==,
+			GPIOD_LINE_EVENT_FALLING_EDGE);
+	g_assert_cmpint(events[1].event_type, ==,
+			GPIOD_LINE_EVENT_RISING_EDGE);
+	g_assert_cmpint(events[2].event_type, ==,
+			GPIOD_LINE_EVENT_FALLING_EDGE);
+	g_assert_cmpint(events[3].event_type, ==,
+			GPIOD_LINE_EVENT_RISING_EDGE);
+
+	ret = gpiod_line_event_wait(line, &ts);
+	g_assert_cmpint(ret, ==, 0);
 }
-- 
2.28.0

[libgpiod][PATCH v2 2/2] core: fix reading subset of available events

[Kent Gibson]

Only read the requested number of events from the kernel rather than
reading up to 16 and quietly discarding any surplus.

The previous behavour is particularly bad for reading single events as
userspace must read the events as quickly as they arrive, effectively
negating the presence of the kernel event kfifo.

Fixes: 44921ecc9a00 (core: provide functions for reading multiple
line events at once)

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 lib/core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/core.c b/lib/core.c
index ad76051..b964272 100644
--- a/lib/core.c
+++ b/lib/core.c
@@ -1090,7 +1090,10 @@ int gpiod_line_event_read_fd_multiple(int fd, struct gpiod_line_event *events,
 
 	memset(evdata, 0, sizeof(evdata));
 
-	rd = read(fd, evdata, sizeof(evdata));
+	if (num_events > 16)
+		num_events = 16;
+
+	rd = read(fd, evdata, num_events * sizeof(*evdata));
 	if (rd < 0) {
 		return -1;
 	} else if ((unsigned int)rd < sizeof(*evdata)) {
-- 
2.28.0

Re: [libgpiod][PATCH v2 2/2] core: fix reading subset of available events

[Bartosz Golaszewski]

On Sat, Sep 12, 2020 at 10:11 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> Only read the requested number of events from the kernel rather than
> reading up to 16 and quietly discarding any surplus.
>
> The previous behavour is particularly bad for reading single events as
> userspace must read the events as quickly as they arrive, effectively
> negating the presence of the kernel event kfifo.
>
> Fixes: 44921ecc9a00 (core: provide functions for reading multiple
> line events at once)
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>  lib/core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lib/core.c b/lib/core.c
> index ad76051..b964272 100644
> --- a/lib/core.c
> +++ b/lib/core.c
> @@ -1090,7 +1090,10 @@ int gpiod_line_event_read_fd_multiple(int fd, struct gpiod_line_event *events,
>
>         memset(evdata, 0, sizeof(evdata));
>
> -       rd = read(fd, evdata, sizeof(evdata));
> +       if (num_events > 16)
> +               num_events = 16;
> +
> +       rd = read(fd, evdata, num_events * sizeof(*evdata));
>         if (rd < 0) {
>                 return -1;
>         } else if ((unsigned int)rd < sizeof(*evdata)) {
> --
> 2.28.0
>

Wow this is a bad one, thanks for catching this!

Will apply shortly and backport it to stable branches too.

Thanks!
Bartosz

Re: [libgpiod][PATCH v2 2/2] core: fix reading subset of available events

[Andy Shevchenko]

On Mon, Sep 14, 2020 at 11:16 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> On Sat, Sep 12, 2020 at 10:11 AM Kent Gibson <warthog618@gmail.com> wrote:

> > Fixes: 44921ecc9a00 (core: provide functions for reading multiple
> > line events at once)
> >
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>


> Will apply shortly and backport it to stable branches too.

Please note that
 - Fixes (or actually any tag) should be one per line
 - no blank lines in the tag block

-- 
With Best Regards,
Andy Shevchenko

Re: [libgpiod][PATCH v2 2/2] core: fix reading subset of available events

[Bartosz Golaszewski]

On Mon, Sep 14, 2020 at 5:16 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Sep 14, 2020 at 11:16 AM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> > On Sat, Sep 12, 2020 at 10:11 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> > > Fixes: 44921ecc9a00 (core: provide functions for reading multiple
> > > line events at once)
> > >
> > > Signed-off-by: Kent Gibson <warthog618@gmail.com>
>
>
> > Will apply shortly and backport it to stable branches too.
>
> Please note that
>  - Fixes (or actually any tag) should be one per line
>  - no blank lines in the tag block
>

Kent: no need to resend this, I fixed it when applying, but yeah for
the future it's true.

Bart

Re: [libgpiod][PATCH v2 2/2] core: fix reading subset of available events

[Andy Shevchenko]

On Mon, Sep 14, 2020 at 6:20 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> On Mon, Sep 14, 2020 at 5:16 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Mon, Sep 14, 2020 at 11:16 AM Bartosz Golaszewski
> > <bgolaszewski@baylibre.com> wrote:
> > > On Sat, Sep 12, 2020 at 10:11 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > > > Fixes: 44921ecc9a00 (core: provide functions for reading multiple
> > > > line events at once)
> > > >
> > > > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> >
> >
> > > Will apply shortly and backport it to stable branches too.
> >
> > Please note that
> >  - Fixes (or actually any tag) should be one per line
> >  - no blank lines in the tag block
> >
>
> Kent: no need to resend this, I fixed it when applying, but yeah for
> the future it's true.

JFYI: I'm about to look at v8 patch 4 of uAPI v2 series.

-- 
With Best Regards,
Andy Shevchenko

Re: [libgpiod][PATCH v2 2/2] core: fix reading subset of available events

[Kent Gibson]

On Mon, Sep 14, 2020 at 06:16:35PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 14, 2020 at 11:16 AM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> > On Sat, Sep 12, 2020 at 10:11 AM Kent Gibson <warthog618@gmail.com> wrote:
> 
> > > Fixes: 44921ecc9a00 (core: provide functions for reading multiple
> > > line events at once)
> > >
> > > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> 
> 
> > Will apply shortly and backport it to stable branches too.
> 
> Please note that
>  - Fixes (or actually any tag) should be one per line
>  - no blank lines in the tag block
> 

Sorry - I wasn't aware that tags are exempt to the line length limits.
And checkpatch.pl doesn't pick up those rules either :-(.

Cheers,
Kent.