Re: [PATCH] gpio: cdev: fix a crash on line-request release

[Kent Gibson <warthog618@gmail.com>]

On Wed, May 24, 2023 at 09:42:13PM +0200, Bartosz Golaszewski wrote:
> On Wed, May 24, 2023 at 6:36 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> Interestingly enough, I did test it just like you and the "fix" seemed
> to address the issue. Upon a further look at the code, it's of course
> clear that the patch is wrong.
> 
> I wanted to debug the code to see what's happening exactly but it
> turned out that enabling the generation of DWARF data hid the issue as
> well even without any fix. It means that it's some kind of a memory
> corruption rather than a regular NULL-pointer dereference.
> 
> I'm not yet sure where the crash happens exactly other that it's in
> the irq domain code.
> 
> Anyway, I'll be back at it tomorrow.
> 

I was also playing with a patch for gpiomon to have it add POLLERR to
its poll() to see if it would notice the chip removal and exit.
It didn't, it just stayed blocked, but it DID made the crash go away
when I killed it.  No idea why that would be.  So yeah, weird things.

This was the patch, btw:

diff --git a/tools/gpiomon.c b/tools/gpiomon.c
index c2684c2..f4251fc 100644
--- a/tools/gpiomon.c
+++ b/tools/gpiomon.c
@@ -431,7 +431,7 @@ int main(int argc, char **argv)
                                   resolver->chips[i].path);
 
                pollfds[i].fd = gpiod_line_request_get_fd(requests[i]);
-               pollfds[i].events = POLLIN;
+               pollfds[i].events = POLLIN | POLLERR;
                gpiod_chip_close(chip);
        }
 
@@ -452,6 +452,9 @@ int main(int argc, char **argv)
                        if (pollfds[i].revents == 0)
                                continue;
 
+                       if (pollfds[i].revents & POLLERR)
+                               die_perror("error polling for events");
+
                        ret = gpiod_line_request_read_edge_events(requests[i],
                                         event_buffer, EVENT_BUF_SIZE);
                        if (ret < 0)


I expect to add that, or something along those lines, if hot removal of
chips from the kernel ever works like it should.
In the meantime it is just another curiosity.

Cheers,
Kent.