gpioib: do not free unrequested descriptors

Message ID 1522348152-15024-1-git-send-email-timur@codeaurora.org
State New
Headers show
Series
  • gpioib: do not free unrequested descriptors
Related show

Commit Message

Timur Tabi March 29, 2018, 6:29 p.m.
If the main loop in linehandle_create() encounters an error, it
unwinds completely by freeing all previously requested GPIO
descriptors.  However, if the error occurs in the beginning of
the loop before that GPIO is requested, then the exit code
attempts to free a null descriptor.  If extrachecks is enabled,
gpiod_free() triggers a WARN_ON.

Instead, keep a separate count of legitimate GPIOs so that only
those are freed.

Cc: stable@vger.kernel.org
Fixes: d7c51b47ac11 ("gpio: userspace ABI for reading/writing GPIO lines")
Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/gpio/gpiolib.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Timur Tabi April 16, 2018, 3:20 a.m. | #1
On 3/29/18 1:29 PM, Timur Tabi wrote:
> If the main loop in linehandle_create() encounters an error, it
> unwinds completely by freeing all previously requested GPIO
> descriptors.  However, if the error occurs in the beginning of
> the loop before that GPIO is requested, then the exit code
> attempts to free a null descriptor.  If extrachecks is enabled,
> gpiod_free() triggers a WARN_ON.
> 
> Instead, keep a separate count of legitimate GPIOs so that only
> those are freed.

Linus, this is an important fix that's needed for sparse GPIO support. 
Any chance that it can make 4.17?

Also, my other patchset for qdf2xxx support has been reviewed by Bjorn 
and Stephen.  Can you add that to 4.17 also?
Timur Tabi April 20, 2018, 8:36 p.m. | #2
On 04/15/2018 10:20 PM, Timur Tabi wrote:
> 
> Linus, this is an important fix that's needed for sparse GPIO support. 
> Any chance that it can make 4.17?
> 
> Also, my other patchset for qdf2xxx support has been reviewed by Bjorn 
> and Stephen.  Can you add that to 4.17 also?

Linus, I don't want to be a pest, but I noticed you updated your 
git.kernel.org repo and none of my patches are there.  I really would 
like to see my patches, which have been reviewed, in 4.17.
Linus Walleij April 26, 2018, 9:17 a.m. | #3
On Thu, Mar 29, 2018 at 8:29 PM, Timur Tabi <timur@codeaurora.org> wrote:

> If the main loop in linehandle_create() encounters an error, it
> unwinds completely by freeing all previously requested GPIO
> descriptors.  However, if the error occurs in the beginning of
> the loop before that GPIO is requested, then the exit code
> attempts to free a null descriptor.  If extrachecks is enabled,
> gpiod_free() triggers a WARN_ON.
>
> Instead, keep a separate count of legitimate GPIOs so that only
> those are freed.
>
> Cc: stable@vger.kernel.org
> Fixes: d7c51b47ac11 ("gpio: userspace ABI for reading/writing GPIO lines")
> Signed-off-by: Timur Tabi <timur@codeaurora.org>

Patch applied for fixes.

Bartosz, can you have a quick look at this? Did you run into the
problem at any point?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij April 26, 2018, 9:22 a.m. | #4
On Fri, Apr 20, 2018 at 10:36 PM, Timur Tabi <timur@codeaurora.org> wrote:
> On 04/15/2018 10:20 PM, Timur Tabi wrote:
>>
>>
>> Linus, this is an important fix that's needed for sparse GPIO support. Any
>> chance that it can make 4.17?
>>
>> Also, my other patchset for qdf2xxx support has been reviewed by Bjorn and
>> Stephen.  Can you add that to 4.17 also?
>
> Linus, I don't want to be a pest, but I noticed you updated your
> git.kernel.org repo and none of my patches are there.  I really would like
> to see my patches, which have been reviewed, in 4.17.

I am sorry that the process is not always as expedient as one could
wish.

The v4.17-rc1 was tagged on april 15 and at that point we rarely
add new functionality.

I usually need to have review finished some ~2 weeks before the
merge window, with some exceptions where things are smooth
or if stuff dragged out.

I understand that sometimes the kernel "process" can be perceived
as bureaucratic, permeated by the bureaucratic incentive to act
slowly and impersonal, such as the planets, or the plants. I am
honestly trying to do my best.

Yours.
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartosz Golaszewski April 26, 2018, 4:44 p.m. | #5
2018-04-26 11:17 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
> On Thu, Mar 29, 2018 at 8:29 PM, Timur Tabi <timur@codeaurora.org> wrote:
>
>> If the main loop in linehandle_create() encounters an error, it
>> unwinds completely by freeing all previously requested GPIO
>> descriptors.  However, if the error occurs in the beginning of
>> the loop before that GPIO is requested, then the exit code
>> attempts to free a null descriptor.  If extrachecks is enabled,
>> gpiod_free() triggers a WARN_ON.
>>
>> Instead, keep a separate count of legitimate GPIOs so that only
>> those are freed.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: d7c51b47ac11 ("gpio: userspace ABI for reading/writing GPIO lines")
>> Signed-off-by: Timur Tabi <timur@codeaurora.org>
>
> Patch applied for fixes.
>
> Bartosz, can you have a quick look at this? Did you run into the
> problem at any point?
>

I have never seen this issue, but the patch looks correct to me.

Thanks,
Bartosz
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Timur Tabi April 26, 2018, 4:46 p.m. | #6
On 04/26/2018 11:44 AM, Bartosz Golaszewski wrote:
>>
>> Bartosz, can you have a quick look at this? Did you run into the
>> problem at any point?
>>
> I have never seen this issue, but the patch looks correct to me.

The issue can only occur if you have sparse GPIOs, and you tried to 
request one that doesn't exist, which probably never happened prior to 4.17.

Thanks for the review.

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 43aeb07343ec..d07771797707 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -497,7 +497,7 @@  static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 	struct gpiohandle_request handlereq;
 	struct linehandle_state *lh;
 	struct file *file;
-	int fd, i, ret;
+	int fd, i, count = 0, ret;
 	u32 lflags;
 
 	if (copy_from_user(&handlereq, ip, sizeof(handlereq)))
@@ -558,6 +558,7 @@  static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 		if (ret)
 			goto out_free_descs;
 		lh->descs[i] = desc;
+		count = i;
 
 		if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
 			set_bit(FLAG_ACTIVE_LOW, &desc->flags);
@@ -628,7 +629,7 @@  static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 out_put_unused_fd:
 	put_unused_fd(fd);
 out_free_descs:
-	for (; i >= 0; i--)
+	for (i = 0; i < count; i++)
 		gpiod_free(lh->descs[i]);
 	kfree(lh->label);
 out_free_lh: