[PATCH 0/3] gpiolib: cdev: tidy up kfifo handling

[PATCH 0/3] gpiolib: cdev: tidy up kfifo handling

[Kent Gibson]

This series is a follow up to my recent kfifo initialisation fix[1].

Patch 1 adds calling INIT_KFIFO() on the event kfifo in order to induce
an oops if the kfifo is accessed prior to being allocated.  Not calling
INIT_KFIFO() could be considered an abuse of the kfifo API. I don't
recall, but it is possible that it was not being called as we also make
use of kfifo_initialized(), and the assumption was that it would return
true after the INIT_KFIFO() call. In fact it only returns true once
the kfifo has been allocated.

Patch 2 adds a helper to perform the allocation of the kfifo to reduce
code duplication.

Patch 3 tidies up the handling of kfifo_out() errors, making them
visible where they may currently be obscured.  These errors are not
expected to ever happen, so this should not produce any visible
difference, but if they do occur it will now be more obvious.

Cheers,
Kent.

[1] https://lore.kernel.org/linux-gpio/20240510065342.36191-1-warthog618@gmail.com/

Kent Gibson (3):
  gpiolib: cdev: Add INIT_KFIFO() for linereq events
  gpiolib: cdev: Refactor allocation of linereq events kfifo
  gpiolib: cdev: Cleanup kfifo_out() error handling

 drivers/gpio/gpiolib-cdev.c | 74 ++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 39 deletions(-)

--
2.39.2

[PATCH 1/3] gpiolib: cdev: Add INIT_KFIFO() for linereq events

[Kent Gibson]

The initialisation of the linereq events kfifo relies on the struct being
zeroed and a subsequent call to kfifo_alloc().  The call to kfifo_alloc()
is deferred until edge detection is first enabled for the linereq.  If the
kfifo is inadvertently accessed before the call to kfifo_alloc(), as was
the case in a recently discovered bug, it behaves as a FIFO of size 1 with
an element size of 0, so writes and reads to the kfifo appear successful
but copy no actual data.

As a defensive measure, initialise the kfifo with INIT_KFIFO() when the
events kfifo is constructed.  This initialises the kfifo element size
and zeroes its data pointer, so any inadvertant access prior to the
kfifo_alloc() call will trigger an oops.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 9dad67ea2597..d4e47960cc98 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1774,6 +1774,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
 
 	mutex_init(&lr->config_mutex);
 	init_waitqueue_head(&lr->wait);
+	INIT_KFIFO(lr->events);
 	lr->event_buffer_size = ulr.event_buffer_size;
 	if (lr->event_buffer_size == 0)
 		lr->event_buffer_size = ulr.num_lines * 16;
-- 
2.39.2

[PATCH 2/3] gpiolib: cdev: Refactor allocation of linereq events kfifo

[Kent Gibson]

The allocation of the linereq events kfifo is performed in two separate
places.  Add a helper function to remove the duplication.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index d4e47960cc98..c7218c9f2c5e 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1128,6 +1128,14 @@ static void edge_detector_stop(struct line *line)
 	/* do not change line->level - see comment in debounced_value() */
 }
 
+static int edge_detector_fifo_init(struct linereq *req)
+{
+	if (kfifo_initialized(&req->events))
+		return 0;
+
+	return kfifo_alloc(&req->events, req->event_buffer_size, GFP_KERNEL);
+}
+
 static int edge_detector_setup(struct line *line,
 			       struct gpio_v2_line_config *lc,
 			       unsigned int line_idx, u64 edflags)
@@ -1139,9 +1147,8 @@ static int edge_detector_setup(struct line *line,
 	char *label;
 
 	eflags = edflags & GPIO_V2_LINE_EDGE_FLAGS;
-	if (eflags && !kfifo_initialized(&line->req->events)) {
-		ret = kfifo_alloc(&line->req->events,
-				  line->req->event_buffer_size, GFP_KERNEL);
+	if (eflags) {
+		ret = edge_detector_fifo_init(line->req);
 		if (ret)
 			return ret;
 	}
@@ -1193,8 +1200,6 @@ static int edge_detector_update(struct line *line,
 				struct gpio_v2_line_config *lc,
 				unsigned int line_idx, u64 edflags)
 {
-	u64 eflags;
-	int ret;
 	u64 active_edflags = READ_ONCE(line->edflags);
 	unsigned int debounce_period_us =
 			gpio_v2_line_config_debounce_period(lc, line_idx);
@@ -1210,14 +1215,9 @@ static int edge_detector_update(struct line *line,
 		 * ensure event fifo is initialised if edge detection
 		 * is now enabled.
 		 */
-		eflags = edflags & GPIO_V2_LINE_EDGE_FLAGS;
-		if (eflags && !kfifo_initialized(&line->req->events)) {
-			ret = kfifo_alloc(&line->req->events,
-					  line->req->event_buffer_size,
-					  GFP_KERNEL);
-			if (ret)
-				return ret;
-		}
+		if (edflags & GPIO_V2_LINE_EDGE_FLAGS)
+			return edge_detector_fifo_init(line->req);
+
 		return 0;
 	}
 
-- 
2.39.2

[PATCH 3/3] gpiolib: cdev: Cleanup kfifo_out() error handling

[Kent Gibson]

The handling of kfifo_out() errors in read functions obscures any error.
The error condition should never occur but, while a ret is set to -EIO, it
is subsequently ignored and the read functions instead return the number
of bytes copied to that point, potentially masking the fact that any error
occurred.

Return -EIO in the case of a kfifo_out() error to make it clear something
very odd is going on here.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 drivers/gpio/gpiolib-cdev.c | 47 +++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 26 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index c7218c9f2c5e..6a986d7f1f2f 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1642,16 +1642,13 @@ static ssize_t linereq_read(struct file *file, char __user *buf,
 					return ret;
 			}
 
-			ret = kfifo_out(&lr->events, &le, 1);
-		}
-		if (ret != 1) {
-			/*
-			 * This should never happen - we were holding the
-			 * lock from the moment we learned the fifo is no
-			 * longer empty until now.
-			 */
-			ret = -EIO;
-			break;
+			if (kfifo_out(&lr->events, &le, 1) != 1)
+				/*
+				 * This should never happen - we hold the
+				 * lock from the moment we learned the fifo
+				 * is no longer empty until now.
+				 */
+				return -EIO;
 		}
 
 		if (copy_to_user(buf + bytes_read, &le, sizeof(le)))
@@ -1995,16 +1992,13 @@ static ssize_t lineevent_read(struct file *file, char __user *buf,
 					return ret;
 			}
 
-			ret = kfifo_out(&le->events, &ge, 1);
-		}
-		if (ret != 1) {
-			/*
-			 * This should never happen - we were holding the lock
-			 * from the moment we learned the fifo is no longer
-			 * empty until now.
-			 */
-			ret = -EIO;
-			break;
+			if (kfifo_out(&le->events, &ge, 1) != 1)
+				/*
+				 * This should never happen - we hold the
+				 * lock from the moment we learned the fifo
+				 * is no longer empty until now.
+				 */
+				return -EIO;
 		}
 
 		if (copy_to_user(buf + bytes_read, &ge, ge_size))
@@ -2707,12 +2701,13 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
 			if (count < event_size)
 				return -EINVAL;
 #endif
-			ret = kfifo_out(&cdev->events, &event, 1);
-		}
-		if (ret != 1) {
-			ret = -EIO;
-			break;
-			/* We should never get here. See lineevent_read(). */
+			if (kfifo_out(&cdev->events, &event, 1) != 1)
+				/*
+				 * This should never happen - we hold the
+				 * lock from the moment we learned the fifo
+				 * is no longer empty until now.
+				 */
+				return -EIO;
 		}
 
 #ifdef CONFIG_GPIO_CDEV_V1
-- 
2.39.2

Re: [PATCH 3/3] gpiolib: cdev: Cleanup kfifo_out() error handling

[Bartosz Golaszewski]

On Mon, May 27, 2024 at 1:55 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> The handling of kfifo_out() errors in read functions obscures any error.
> The error condition should never occur but, while a ret is set to -EIO, it
> is subsequently ignored and the read functions instead return the number
> of bytes copied to that point, potentially masking the fact that any error
> occurred.
>
> Return -EIO in the case of a kfifo_out() error to make it clear something
> very odd is going on here.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>  drivers/gpio/gpiolib-cdev.c | 47 +++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index c7218c9f2c5e..6a986d7f1f2f 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -1642,16 +1642,13 @@ static ssize_t linereq_read(struct file *file, char __user *buf,
>                                         return ret;
>                         }
>
> -                       ret = kfifo_out(&lr->events, &le, 1);
> -               }
> -               if (ret != 1) {
> -                       /*
> -                        * This should never happen - we were holding the
> -                        * lock from the moment we learned the fifo is no
> -                        * longer empty until now.
> -                        */
> -                       ret = -EIO;
> -                       break;
> +                       if (kfifo_out(&lr->events, &le, 1) != 1)
> +                               /*
> +                                * This should never happen - we hold the

I'm not a native speaker but this looks odd to me - shouldn't it be
"we held the lock from the moment..."?

> +                                * lock from the moment we learned the fifo
> +                                * is no longer empty until now.
> +                                */
> +                               return -EIO;

Since this is so unlikely maybe a WARN() would be justified here too?

Bart

>                 }
>
>                 if (copy_to_user(buf + bytes_read, &le, sizeof(le)))
> @@ -1995,16 +1992,13 @@ static ssize_t lineevent_read(struct file *file, char __user *buf,
>                                         return ret;
>                         }
>
> -                       ret = kfifo_out(&le->events, &ge, 1);
> -               }
> -               if (ret != 1) {
> -                       /*
> -                        * This should never happen - we were holding the lock
> -                        * from the moment we learned the fifo is no longer
> -                        * empty until now.
> -                        */
> -                       ret = -EIO;
> -                       break;
> +                       if (kfifo_out(&le->events, &ge, 1) != 1)
> +                               /*
> +                                * This should never happen - we hold the
> +                                * lock from the moment we learned the fifo
> +                                * is no longer empty until now.
> +                                */
> +                               return -EIO;
>                 }
>
>                 if (copy_to_user(buf + bytes_read, &ge, ge_size))
> @@ -2707,12 +2701,13 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
>                         if (count < event_size)
>                                 return -EINVAL;
>  #endif
> -                       ret = kfifo_out(&cdev->events, &event, 1);
> -               }
> -               if (ret != 1) {
> -                       ret = -EIO;
> -                       break;
> -                       /* We should never get here. See lineevent_read(). */
> +                       if (kfifo_out(&cdev->events, &event, 1) != 1)
> +                               /*
> +                                * This should never happen - we hold the
> +                                * lock from the moment we learned the fifo
> +                                * is no longer empty until now.
> +                                */
> +                               return -EIO;
>                 }
>
>  #ifdef CONFIG_GPIO_CDEV_V1
> --
> 2.39.2
>

Re: [PATCH 3/3] gpiolib: cdev: Cleanup kfifo_out() error handling

[Kent Gibson]

On Wed, May 29, 2024 at 01:24:45PM +0200, Bartosz Golaszewski wrote:
> On Mon, May 27, 2024 at 1:55 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > The handling of kfifo_out() errors in read functions obscures any error.
> > The error condition should never occur but, while a ret is set to -EIO, it
> > is subsequently ignored and the read functions instead return the number
> > of bytes copied to that point, potentially masking the fact that any error
> > occurred.
> >
> > Return -EIO in the case of a kfifo_out() error to make it clear something
> > very odd is going on here.
> >
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > ---
> >  drivers/gpio/gpiolib-cdev.c | 47 +++++++++++++++++--------------------
> >  1 file changed, 21 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > index c7218c9f2c5e..6a986d7f1f2f 100644
> > --- a/drivers/gpio/gpiolib-cdev.c
> > +++ b/drivers/gpio/gpiolib-cdev.c
> > @@ -1642,16 +1642,13 @@ static ssize_t linereq_read(struct file *file, char __user *buf,
> >                                         return ret;
> >                         }
> >
> > -                       ret = kfifo_out(&lr->events, &le, 1);
> > -               }
> > -               if (ret != 1) {
> > -                       /*
> > -                        * This should never happen - we were holding the
> > -                        * lock from the moment we learned the fifo is no
> > -                        * longer empty until now.
> > -                        */
> > -                       ret = -EIO;
> > -                       break;
> > +                       if (kfifo_out(&lr->events, &le, 1) != 1)
> > +                               /*
> > +                                * This should never happen - we hold the
>
> I'm not a native speaker but this looks odd to me - shouldn't it be
> "we held the lock from the moment..."?
>

Unlike the original, it is within the scoped_guard here, and we still hold the
lock, so using the past tense would be incorrect.

> > +                                * lock from the moment we learned the fifo
> > +                                * is no longer empty until now.
> > +                                */
> > +                               return -EIO;
>
> Since this is so unlikely maybe a WARN() would be justified here too?
>

Yeah, that makes sense.  I'll add them for v2.

Cheers,
Kent.