On Fri, Dec 08, 2023 at 11:36:31PM +0100, Alexander Bluhm wrote:
> 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.
>
Thanks, I will fix this.
> > 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.
>
The `sc_lock' rwlock(9) protects the `so' socket dereference. Since it
is passed to sosend() which takes netlock within, the `sc_lock' should
be taken first:
void
pflow_output_process(void *arg)
{
struct mbuf_list ml;
struct pflow_softc *sc = arg;
struct mbuf *m;
mq_delist(&sc->sc_outputqueue, &ml);
rw_enter_read(&sc->sc_lock);
while ((m = ml_dequeue(&ml)) != NULL) {
pflow_sendout_mbuf(sc, m);
}
rw_exit_read(&sc->sc_lock);
}
int
pflow_sendout_mbuf(struct pflow_softc *sc, struct mbuf *m)
{
counters_pkt(sc->sc_if.if_counters,
ifc_opackets, ifc_obytes, m->m_pkthdr.len);
if (sc->so == NULL) {
m_freem(m);
return (EINVAL);
}
return (sosend(sc->so, sc->send_nam, NULL, m, NULL, 0));
}
I want to rework netlock relocking in pflowioctl(). Guess it should be
realeased and the `sc_lock' should be taken just after we enter
pflowioctl(). We need to release netlock because pflowioctl() calls
solcose(), socreate() and sobind(). Since the `sc_lock' will not be
relocked, the `sc_dying' should rely on it. This will fix the
pflowioctl() concurrency with pflow_clone_destroy().
> > pflow(4) after committing this.
> > 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.
> OK bluhm@
>
> Feel free to address my remarks later.
>
