Bug 256334
| Summary: | [genet] Race condition in Pi4's gen_attach() can cause SIGSEGV. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | ghuckriede | ||||||
| Component: | arm | Assignee: | Mike Karels <karels> | ||||||
| Status: | Closed FIXED | ||||||||
| Severity: | Affects Only Me | CC: | karels | ||||||
| Priority: | --- | ||||||||
| Version: | CURRENT | ||||||||
| Hardware: | arm64 | ||||||||
| OS: | Any | ||||||||
| Attachments: |
|
||||||||
|
Description
ghuckriede
2021-06-01 13:06:40 UTC
Thanks! Can you reproduce this? Do you know how? I'm trying to reproduce this again, without much success. When I do, I'll post the steps and the core file. No luck, with reproducing this. Clearing the interrupts before enabling them would also prevent this race. Created attachment 225600 [details]
proposed patch
I have attached a proposed patch. Simply reordering the blocks seems like it should work, and this version runs. Could you test it?
I tested the change (While sending iperf traffic rebooted the system) and it started fine. Although I couldn't reproduce the issue before the change either. Regardless, this change will prevent an ifp de-reference. I'm curious why the second bus_setup_intr() call was not also moved? I understand the gen_intr2 function does nothing currently, but it may have functionality added later that might also rely on ifp being set? Plus having both the "Install interrupt handlers" in the same place seems like a good idea to me. Thanks for pointing out the inconsistency with the second interrupt; that was an oversight, fixed. Do you want to test that version, or should I just commit it? Created attachment 225786 [details]
revised patch
Just in case, I have attached the revised patch.
(In reply to Mike Karels from comment #7) The second interrupt does nothing currently, but I tested it anyways. Works for me. A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=13604fb0fd43c85e6bb3a0ad6400a684f150bdea commit 13604fb0fd43c85e6bb3a0ad6400a684f150bdea Author: Mike Karels <karels@FreeBSD.org> AuthorDate: 2021-06-20 16:10:26 +0000 Commit: Mike Karels <karels@FreeBSD.org> CommitDate: 2021-06-20 16:17:13 +0000 genet: Fix potential crash during attach As pointed out in the bug, the genet driver (RPi4 Ethernet) was attaching the interrupts before the data structures were fully initialized, causing a crash if an interrupt came in during the attach. Fix by reordering code blocks. PR: 256334 Reported by: < ghuckriede at blackberry.com > Reviewed by: < ghuckriede at blackberry.com > (informally) MFC after: 3 days sys/arm64/broadcom/genet/if_genet.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=0a4ad905bf44d5286ae8e1c4c9693c98fca43183 commit 0a4ad905bf44d5286ae8e1c4c9693c98fca43183 Author: Mike Karels <karels@FreeBSD.org> AuthorDate: 2021-06-20 16:10:26 +0000 Commit: Mike Karels <karels@FreeBSD.org> CommitDate: 2021-07-05 14:12:09 +0000 genet: Fix potential crash during attach As pointed out in the bug, the genet driver (RPi4 Ethernet) was attaching the interrupts before the data structures were fully initialized, causing a crash if an interrupt came in during the attach. Fix by reordering code blocks. PR: 256334 Reported by: < ghuckriede at blackberry.com > Reviewed by: < ghuckriede at blackberry.com > (informally) (cherry picked from commit 13604fb0fd43c85e6bb3a0ad6400a684f150bdea) sys/arm64/broadcom/genet/if_genet.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) Resolved and MFC'd to stable/13. |
