The following MFC to stable/13 removed checks for NULL for "cred". https://cgit.freebsd.org/src/commit/sys/netinet/in_pcb.c?h=stable/13&id=3e27fcf057937ef93258dc4f8e67a308e02f349c Now users of the port net-mgmt/ng_ipacct suffer from "bootloops" due to kernel panic at boot time because of the following code in the ng_ipacct.c: #if __FreeBSD_version >= 700110 pcb = in_pcblookup_local(pcbinfo, ina, port, 1, NOCRED); #else pcb = in_pcblookup_local(pcbinfo, ina, port, 1); #endif NOCRED is NULL. (kgdb) bt #0 __curthread () at /data/src/sys/amd64/include/pcpu_aux.h:53 #1 doadump (textdump=<optimized out>) at /data/src/sys/kern/kern_shutdown.c:394 #2 0xffffffff80c0798e in kern_reboot (howto=260) at /data/src/sys/kern/kern_shutdown.c:482 #3 0xffffffff80c07e2f in vpanic (fmt=0xffffffff812442eb "%s", ap=ap@entry=0xfffffe000859a700) at /data/src/sys/kern/kern_shutdown.c:921 #4 0xffffffff80c07c63 in panic (fmt=<unavailable>) at /data/src/sys/kern/kern_shutdown.c:845 #5 0xffffffff810e806d in trap_fatal (frame=0xfffffe000859a7f0, eva=128) at /data/src/sys/amd64/amd64/trap.c:940 #6 0xffffffff810e80bf in trap_pfault (frame=0xfffffe000859a7f0, usermode=false, signo=<optimized out>, ucode=<optimized out>) at /data/src/sys/amd64/amd64/trap.c:759 #7 <signal handler called> #8 in_pcblookup_local (pcbinfo=<optimized out>, laddr=..., lport=<optimized out>, lookupflags=<optimized out>, cred=0x0) at /data/src/sys/netinet/in_pcb.c:2039 #9 0xffffffff8273042f in pcb_get_cred (r=0xfffffe000859a9a4, pcbinfo=0xfffffe00092b7ad8) at ng_ipacct.c:987 Note "cred=0x0" for 8th frame.
Adding maintainer of the port to CC
It should also have __attribute__((nonnull)) to prevent surprises at the compile time...
(In reply to Vsevolod Stakhov from comment #2) IMO this is not worth it. We have many interfaces with this constraint, and _Nonnull has downsides. It is also (ab)used by the compiler as an optimization hint, resulting in silent elision of null checks. I can restore the checks in stable/13 and 13.3, but this code is broken on 14.0 as well so will require a proper solution.
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=fe8df7ed1aae444a09361c080d52bfcb6aaae64f commit fe8df7ed1aae444a09361c080d52bfcb6aaae64f Author: Mark Johnston <markj@FreeBSD.org> AuthorDate: 2024-02-07 14:43:25 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2024-02-07 14:46:34 +0000 inpcb: Restore some NULL checks of credential pointers At least one out-of-tree port (net-mgmt/ng_ipacct) depends on being able to call in_pcblookup_local() with cred == NULL, so the MFC of commit ac1750dd143e ("inpcb: Remove NULL checks of credential references") broke compatibility. Restore a subset of the NULL checks to avoid breaking the module in the 13.3 release. This is a direct commit to stable/13. PR: 276868 sys/netinet/in_pcb.c | 6 ++++-- sys/netinet6/in6_pcb.c | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-)
Eugene, are you able to confirm that this commit to stable/13 is sufficient? If so, I'll work on getting it merged to releng/13.3.
(In reply to Mark Johnston from comment #3) I'm sorry but I'm not with you on this page. Could you please elaborate on compiler's elision of NULL checks in the case where there are *no NULL checks* at all?
(In reply to Vsevolod Stakhov from comment #6) In this case, there is no problem. I meant in general, I've had to fix bugs in the kernel caused by excessive use of the _Nonnull annotation.
(In reply to Mark Johnston from comment #5) I'll test and report back.
(In reply to Mark Johnston from comment #5) Yes, it helps. No more panics with same ng_ipacct code.
(In reply to Mark Johnston from comment #3) Ok, for version 14, what should we pass to the in_pcblookup_local() function? As I understand it, the function checks access rights to the jail. Will it be enough to pass the 'cred' of the current thread (curthread->td_ucred)? https://github.com/vstakhov/ng_ipacct/blob/master/ng_ipacct/ng_ipacct.c#L992
(In reply to Alexander Fedorov from comment #10) It depends on the context of the caller. From my reading, in_pcblookup_local() is only called from the rcvhook implementation, so it is not clear to me what the correct ucred is. How should the feature interact with (classic) jails, if two sockets are listening on a port, one inside the jail and one outside the jail? If you want to just ignore the question of jails, just pass `thread0.td_ucred`.
(In reply to Mark Johnston from comment #11) The code in question pre-dates FreeBSD jails.
(In reply to Eugene Grosbein from comment #12) Sure, but it's not obvious to me whether the code was later adapted to work with jails somehow. If not, and you don't care about them, then passing `thread0.td_ucred` will give the same behaviour as before.
(In reply to Mark Johnston from comment #13) > Sure, but it's not obvious to me whether the code was later adapted to work with jails somehow. I doubt it. > If not, and you don't care about them, then passing `thread0.td_ucred` will give the same behaviour as before. I don't think we have alternatives.
A commit in branch releng/13.3 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=3e6382c1eda5ea4451a64ec69fd8a92f621aca55 commit 3e6382c1eda5ea4451a64ec69fd8a92f621aca55 Author: Mark Johnston <markj@FreeBSD.org> AuthorDate: 2024-02-07 14:43:25 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2024-02-09 23:12:49 +0000 inpcb: Restore some NULL checks of credential pointers At least one out-of-tree port (net-mgmt/ng_ipacct) depends on being able to call in_pcblookup_local() with cred == NULL, so the MFC of commit ac1750dd143e ("inpcb: Remove NULL checks of credential references") broke compatibility. Restore a subset of the NULL checks to avoid breaking the module in the 13.3 release. This is a direct commit to stable/13. PR: 276868 Approved by: re (cperciva) (cherry picked from commit fe8df7ed1aae444a09361c080d52bfcb6aaae64f) sys/netinet/in_pcb.c | 6 ++++-- sys/netinet6/in6_pcb.c | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-)