On Sat, Dec 09, 2023 at 12:28:10AM +0100, Alexander Bluhm wrote: > On Sat, Dec 09, 2023 at 02:07:06AM +0300, Vitaliy Makkoveev wrote: > > > > SLIST_ENTRY(pflow_softc) sc_next; > > > > > > This list is protected by net lock. Can you add an [N] here? > > > > > > > This is not true. The netlock is not taken while export_pflow() called > > from pf_purge_states(). I privately shared the diff to fix this, but not > > committed it yet. I will update it and share after committing this diff. > > Why not hold the shared net lock while in pf? >
Because the export_flow() should be called only if the PFSTATE_PFLOW flag is set? The netlock should be taken before PF_LOCK, so there is no reason to always take it in this path only to make optional export_pflow() call: pf_purge_expired_states(const unsigned int limit, const unsigned int collect) { /* skip */ rw_enter_write(&pf_state_list.pfs_rwl); PF_LOCK(); PF_STATE_ENTER_WRITE(); SLIST_FOREACH(st, &gcl, gc_list) { if (st->timeout != PFTM_UNLINKED) pf_remove_state(st); pf_free_state(st); } pf_remove_state(struct pf_state *st) { PF_ASSERT_LOCKED(); /* skip */ RBT_REMOVE(pf_state_tree_id, &tree_id, st); #if NPFLOW > 0 if (st->state_flags & PFSTATE_PFLOW) export_pflow(st); However, I'm not the pf hacker, I's better to ask sashan@ and dlg@. > My stategy is to convert exclusive net lock to shared net lock step > by step. When this is complete, we can remove net lock completely. > Until that happens, we hold exclusive net lock in corner cases and > know that nothing can go wrong. > > Seems to be easier than to be aware of pflowif_list locking when > working on pf. > I already made the diff which turns `pflowif_list' into SMR list. The diff was tested by Hrvoje and ok'ed by sashan@. Please note, the `pflowif_list' modifications rely on kernel lock because it is always taken in the clone create and destroy paths. We never marked SMR lists locking in the locks description, so I left this empty. Do you have any idea how to mark it? I want to commit this diff unless you have objections. After that only `pflowstats' will have inconsistent protection. Index: sys/net/if_pflow.c =================================================================== RCS file: /cvs/src/sys/net/if_pflow.c,v retrieving revision 1.102 diff -u -p -r1.102 if_pflow.c --- sys/net/if_pflow.c 8 Dec 2023 23:15:44 -0000 1.102 +++ sys/net/if_pflow.c 9 Dec 2023 00:02:48 -0000 @@ -62,7 +62,7 @@ #define DPRINTF(x) #endif -SLIST_HEAD(, pflow_softc) pflowif_list; +SMR_SLIST_HEAD(, pflow_softc) pflowif_list; struct pflowstats pflowstats; void pflowattach(int); @@ -113,7 +113,7 @@ struct if_clone pflow_cloner = void pflowattach(int npflow) { - SLIST_INIT(&pflowif_list); + SMR_SLIST_INIT(&pflowif_list); if_clone_attach(&pflow_cloner); } @@ -268,9 +268,8 @@ pflow_clone_create(struct if_clone *ifc, task_set(&pflowif->sc_outputtask, pflow_output_process, pflowif); /* Insert into list of pflows */ - NET_LOCK(); - SLIST_INSERT_HEAD(&pflowif_list, pflowif, sc_next); - NET_UNLOCK(); + KERNEL_ASSERT_LOCKED(); + SMR_SLIST_INSERT_HEAD_LOCKED(&pflowif_list, pflowif, sc_next); return (0); } @@ -284,9 +283,12 @@ pflow_clone_destroy(struct ifnet *ifp) NET_LOCK(); sc->sc_dying = 1; - SLIST_REMOVE(&pflowif_list, sc, pflow_softc, sc_next); NET_UNLOCK(); + KERNEL_ASSERT_LOCKED(); + SMR_SLIST_REMOVE_LOCKED(&pflowif_list, sc, pflow_softc, sc_next); + smr_barrier(); + timeout_del(&sc->sc_tmo); timeout_del(&sc->sc_tmo6); timeout_del(&sc->sc_tmo_tmpl); @@ -812,7 +814,7 @@ export_pflow(struct pf_state *st) sk = st->key[st->direction == PF_IN ? PF_SK_WIRE : PF_SK_STACK]; - SLIST_FOREACH(sc, &pflowif_list, sc_next) { + SMR_SLIST_FOREACH(sc, &pflowif_list, sc_next) { mtx_enter(&sc->sc_mtx); switch (sc->sc_version) { case PFLOW_PROTO_5: Index: sys/net/if_pflow.h =================================================================== RCS file: /cvs/src/sys/net/if_pflow.h,v retrieving revision 1.20 diff -u -p -r1.20 if_pflow.h --- sys/net/if_pflow.h 8 Dec 2023 23:13:40 -0000 1.20 +++ sys/net/if_pflow.h 9 Dec 2023 00:02:48 -0000 @@ -169,6 +169,8 @@ struct pflow_ipfix_flow6 { #ifdef _KERNEL +#include <sys/smr.h> + /* * Locks used to protect struct members and global data * I immutable after creation @@ -207,7 +209,7 @@ struct pflow_softc { mbuf */ struct mbuf *sc_mbuf6; /* [m] current cumulative mbuf */ - SLIST_ENTRY(pflow_softc) sc_next; + SMR_SLIST_ENTRY(pflow_softc) sc_next; }; extern struct pflow_softc *pflowif;