Bug 256334 - [genet] Race condition in Pi4's gen_attach() can cause SIGSEGV.
Summary: [genet] Race condition in Pi4's gen_attach() can cause SIGSEGV.
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: arm (show other bugs)
Version: CURRENT
Hardware: arm64 Any
: --- Affects Only Me
Assignee: Mike Karels
Depends on:
Reported: 2021-06-01 13:06 UTC by ghuckriede
Modified: 2021-06-20 16:18 UTC (History)
1 user (show)

See Also:

proposed patch (1.09 KB, patch)
2021-06-06 17:17 UTC, Mike Karels
no flags Details | Diff
revised patch (1.47 KB, patch)
2021-06-13 23:43 UTC, Mike Karels
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ghuckriede 2021-06-01 13:06:40 UTC
'genet' driver starts interrupt handlers before ifp is allocated.

Version: git main @ d3f7975fcb346ea28dde079a9c04cff5ef20a8d7

gen_intr() uses sc->ifp here:

gen_attach() calls bus_setup_intr() here:

gen_attach() calls if_alloc() here:

Possible fixes:
gen_attach() could either hold GEN_LOCK() or complete the init before the bus_setup_intr() calls.
Comment 1 Mike Karels freebsd_committer 2021-06-01 14:45:29 UTC
Thanks! Can you reproduce this?  Do you know how?
Comment 2 ghuckriede 2021-06-01 15:11:10 UTC
I'm trying to reproduce this again, without much success.  When I do, I'll post the steps and the core file.
Comment 3 ghuckriede 2021-06-03 13:13:13 UTC
No luck, with reproducing this.
Comment 4 ghuckriede 2021-06-03 13:16:21 UTC
Clearing the interrupts before enabling them would also prevent this race.
Comment 5 Mike Karels freebsd_committer 2021-06-06 17:17:50 UTC
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?
Comment 6 ghuckriede 2021-06-07 13:58:13 UTC
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.
Comment 7 Mike Karels freebsd_committer 2021-06-12 16:42:57 UTC
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?
Comment 8 Mike Karels freebsd_committer 2021-06-13 23:43:08 UTC
Created attachment 225786 [details]
revised patch

Just in case, I have attached the revised patch.
Comment 9 ghuckriede 2021-06-14 13:23:42 UTC
(In reply to Mike Karels from comment #7)
The second interrupt does nothing currently, but I tested it anyways. Works for me.
Comment 10 commit-hook freebsd_committer 2021-06-20 16:18:58 UTC
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(-)