On Thu, Nov 24, 2022 at 07:09:39PM +0100, Alexandr Nedvedicky wrote:
> Hello,
>
>
> On Thu, Nov 24, 2022 at 10:29:37AM -0500, David Hill wrote:
> > > >
> >
> > With this diff against -current - my dmesg is spammed with:
> >
> > splassert: pfsync_delete_state: want 2 have 0
> > Starting stack trace...
> > pfsync_delete_state(fffffd820af9f940) at pfsync_delete_state+0x58
> > pf_remove_state(fffffd820af9f940) at pf_remove_state+0x14b
> > pf_purge_expired_states(42,40) at pf_purge_expired_states+0x202
> > pf_purge_states(0) at pf_purge_states+0x1c
> > taskq_thread(ffffffff822c78f0) at taskq_thread+0x11a
> >
I also found them on my test machines now. But I did not check the
logs when I tested the diff. Stupid me.
> there are forgotten NET_ASSERT_LOCK() which are no longer valid,
> in pfsync. diff below removes those which are either hit
> from purge_thread() or from ioctl().
>
> I think remaining NET_ASSERT_LOCK() should stay at least for now.
> those belong to path which runs under NET_LOCK()
>
> can you give a try diff below?
pfsync_undefer() is called without PF_LOCK from pf_test(). Now
that we have neither net nor pf lock, how can CLR(pd->pd_st->state_flags,
PFSTATE_ACK) in pfsync_undefer() be MP safe?
And pfsync_undefer() calls pfsync_undefer() which calls ip_output().
How can this work without net lock?
bluhm
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sys/net/if_pfsync.c b/sys/net/if_pfsync.c
> index f69790ee98d..24963a546de 100644
> --- a/sys/net/if_pfsync.c
> +++ b/sys/net/if_pfsync.c
> @@ -1865,8 +1865,6 @@ pfsync_undefer(struct pfsync_deferral *pd, int drop)
> {
> struct pfsync_softc *sc = pfsyncif;
>
> - NET_ASSERT_LOCKED();
> -
> if (sc == NULL)
> return;
>
> @@ -2128,8 +2126,6 @@ pfsync_delete_state(struct pf_state *st)
> {
> struct pfsync_softc *sc = pfsyncif;
>
> - NET_ASSERT_LOCKED();
> -
> if (sc == NULL || !ISSET(sc->sc_if.if_flags, IFF_RUNNING))
> return;
>
> @@ -2188,8 +2184,6 @@ pfsync_clear_states(u_int32_t creatorid, const char
> *ifname)
> struct pfsync_clr clr;
> } __packed r;
>
> - NET_ASSERT_LOCKED();
> -
> if (sc == NULL || !ISSET(sc->sc_if.if_flags, IFF_RUNNING))
> return;
>
