Skip to content

fix INTRNG interrupt release #1281

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ehem wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix INTRNG interrupt release #1281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9fce87a
intrng: allocate event during isrc setup
ehem Aug 21, 2023
d276365
intrng: change isrc_free_irq()'s return to void
ehem Mar 10, 2023
19f0238
intrng: destroy event when deregistering source
ehem Nov 12, 2022
384a66a
intrng: invalidate ->isrc_index and ->isrc_count when releasing counters
ehem Apr 23, 2025
67e532d
kern/intr: have intr_event_destroy() return success on NULL event
ehem Feb 15, 2023
2a32e20
kern/intr: declare intr_event_destroy() __result_use_check
ehem Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions sys/kern/kern_intr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,8 +530,10 @@ int
intr_event_destroy(struct intr_event *ie)
{

if (ie == NULL)
return (EINVAL);
if (ie == NULL) {
printf("ERROR: %s(): passed NULL event!\n", __func__);
return (0);

@bsdimp bsdimp Jun 15, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is wrong. We want this to be an error. It's a programming error to call this with NULL.

@ehem ehem Jun 15, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

% find sys -type f -print0 | xargs -0 grep -eintr_event_destroy -l
sys/netpfil/pf/if_pfsync.c
sys/netpfil/pf/pf_ioctl.c
sys/netpfil/pf/pflow.c
sys/kern/kern_intr.c
sys/kern/subr_intr.c
sys/sys/interrupt.h
%

The ones in sys/netpfil/pf/ are unlikely to trigger this. The only present way for them to trigger this is for something to scribble zeros over memory or some other sort of memory corruption. My concern is they're all calling intr_event_destroy() during shutdown, then either doing MPASS(rc == 0); or returning the error to the caller. If this isn't a debug kernel then the error could simply be lost (sys/netpfil/pf/if_pfsync.c:vnet_pfsync_uninit()). If this is a debug kernel, is a panic() truly a desirable result? I suspect these callers would instead prefer:

if (ie == NULL) {
        printf("ERROR: %s(): called with NULL event\n", __func__);
        return (0);
}

As the rest of their shutdowns can still occur.

The situation for INTRNG (sys/kern/subr_intr.c) is very different. INTRNG does lazy event allocation. After intr_isrc_register() returns, ->isrc_event is still NULL. ->isrc_event doesn't become non-NULL until isrc_add_handler() is called. As such:

struct intr_irqsrc *isrc;
int rc;
intr_isrc_register(isrc);
rc = intr_isrc_deregister(isrc);
printf("intr_isrc_deregister() returned: %d\n", rc);

Will reliably output intr_isrc_deregister() returned: 22. So there is a disagreement in the implementation about what intr_event_destroy(NULL) should do/return. Since I'm looking at a driver which needs intr_isrc_deregister() to work, this is blocking for me.

I suppose you could modify isrc_event_destroy() to do if (isrc->isrc_event != NULL) return(0);. My concern with this is you're testing the same value twice and giving exactly opposite results for a problematic value. I cannot abide this, it is just too ugly (I will hold my nose and ignore it if someone else implements, but I will not implement).

Another viable approach would be to remove lazy event allocation from INTRNG. I don't think lazy event allocation significantly helps INTRNG. I also observe major downsides to lazy event allocation (both here and other places you must first test ->isrc_event, you cannot assume non-NULL). I think it was a mistake to allow PowerPC and INTRNG to use lazy event allocation.
Issue is this would basically invalidate the premise behind INTR_SOLO and someone would be rather annoyed with that (D35607).

@ehem ehem Jun 15, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, one other approach I would be willing to implement. It would be acceptable to me to revert 39888ed and then have isrc_event_destroy() check for isrc->isrc_event == NULL. It is the double checking for NULL which I have a problem with.

I though would be quite happy if lazy event allocation was deprecated, and assured removing that would be accepted.

@ehem ehem Aug 3, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In different terms, this situation is highly similar to c1287a3, Yes, it might be an error, but handing the error off to the caller drastically escalates the damage. Whereas handling it mitigates things.

}

mtx_lock(&event_lock);
mtx_lock(&ie->ie_lock);
Expand Down
Loading
Loading