On Tue, May 03, 2022 at 07:42:34PM +0200, Moritz Buhl wrote:
> commit 4b3977248902c22d96aaebdb5784840debc2631c
> Author: mikeb <mi...@openbsd.org>
> Date: Mon Nov 24 13:22:09 2008 +0000
>
> Fix splasserts seen in pr 5987 by propagating a flag that discribes
> whether we're called from the interrupt context to the functions
> performing allocations.
These days pf was protected with kernel lock and spl. Both are
released when sleeping. Now we have netlock and pflock. These are
rwlocks and not released during sleep. So this old race should not
exist anymore.
> And we are not in interrupt context.
Yes, it is ioctl(2). I think we should always malloc with M_WAITOK
when in syscall. Otherwise userland would have to cope with randomly
failing syscalls.
> If this is sound, then the only reason why pfr_destroy_ktable was called
> is that pool_get is called with PR_NOWAIT. And then the following diff
> would help.
The code is too complex to be sure what the reason of the syzkaller
panic is. Sleep in malloc is correct anyway and may improve the
situation.
Functions with argument values 0 or 1 are hard to read. It would
be much nicer to pass M_WAITOK or M_NOWAIT. And the variable name
"intr" does not make sense anymore. pf does not run in interrupt
context. Call it "mflags" like in pfi_kif_alloc(). Or "wait" like
in other functions.
Could you cleanup that also?
bluhm
> Index: pf_if.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf_if.c,v
> retrieving revision 1.104
> diff -u -p -r1.104 pf_if.c
> --- pf_if.c 29 Apr 2022 09:55:43 -0000 1.104
> +++ pf_if.c 3 May 2022 16:01:23 -0000
> @@ -464,7 +464,7 @@ pfi_dynaddr_setup(struct pf_addr_wrap *a
> goto _bad;
> }
>
> - if ((dyn->pfid_kt = pfr_attach_table(ruleset, tblname, 1)) == NULL) {
> + if ((dyn->pfid_kt = pfr_attach_table(ruleset, tblname, 0)) == NULL) {
> rv = 1;
> goto _bad;
> }
