On Thu, Dec 07, 2023 at 06:14:30PM +0300, Vitaliy Makkoveev wrote: > Here id the diff. I introduces `sc_mtx' mutex(9) to protect the most of > pflow_softc structure. The `send_nam', `sc_flowsrc' and `sc_flowdst' are > prtected by `sc_lock' rwlock(9). `sc_tmpl_ipfix' is immutable. > > Also, the pflow_sendout_ipfix_tmpl() calls pflow_get_mbuf() with NULL > instead of `sc'. This fix also included to this diff. > > Please note, this diff does not fix all the problems in the pflow(4). > The ifconfig create/destroy sequence could still break the kernel. I > have ok'ed diff to fix this but did not commit it yet for some reason. > Also the `pflowstats' data left unprotected. I will fix this with > separate diff. > > Please test this diff and let us know the result. I will continue with > pflow(4) after committing this.
Your changes make sense. > sc->sc_gcounter=pflowstats.pflow_flows; Could you add some spaces around = sc_gcounter is set without spaced twice. > case SIOCGETPFLOW: > bzero(&pflowr, sizeof(pflowr)); > > + /* XXXSMP: enforce lock order */ > + NET_UNLOCK(); > + rw_enter_read(&sc->sc_lock); > + NET_LOCK(); This looks ugly. I did not find the sc_lock -> NET_LOCK() seqence. Where is it? Why do we need it? In general I think the global NET_LOCK() should be taken before the specific sc_lock. Moving the net lock to very narrow places within other locks makes things more complicated. Better take a shared net lock and hold it for more code. Not sure if this is a usable solution in this case. > SLIST_ENTRY(pflow_softc) sc_next; This list is protected by net lock. Can you add an [N] here? OK bluhm@ Feel free to address my remarks later.
