On 20/05/21(Thu) 03:23, Alexandr Nedvedicky wrote: > Hrvoje gave a try to experimental diff, which trades rw-locks in pf(4) > for mutexes [1]. Hrvoje soon discovered machine panics, when doing 'pfctl -ss' > The callstack looks as follows: > > [...] > specific to experimental diff [1]. However this made me thinking, that > it's not a good idea to do copyout() while holding NET_LOCK() and state_lock.
malloc(9) and copyout(9) are kind of ok while using the NET_LOCK() but if a deadlock occurs while a global rwlock is held, debugging becomes harder. As long as the `state_lock' and PF_LOCK() are mutexes all allocations and copyin/copyout(9) must be done without holding them. > Diff below moves copyout() at line 1784 outside of protection of both locks. > The approach I took is relatively straightforward: > > let DIOCGETSTATES to allocate hold_states array, which will keep > references to states. > > grab locks and take references, keep those references in hold > array. > > drop locks, export states and do copyout, while walking > array of references. > > drop references, release hold_states array. > > does it make sense? If we agree that this approach makes sense In my opinion it does. The other approach would be to (ab)use the NET_LOCK() to serialize updates, like bluhm@'s diff does. Both approaches have pros and cons. > I'll commit this diff and revisit other such places we currently > have in pfioctl(). > > thanks and > regards > sashan > > [1] https://marc.info/?l=openbsd-tech&m=162138181106887&w=2 > > --------8<---------------8<---------------8<------------------8<-------- > diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c > index ae7bb008351..0d4ac97a92c 100644 > --- a/sys/net/pf_ioctl.c > +++ b/sys/net/pf_ioctl.c > @@ -1762,43 +1762,58 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int > flags, struct proc *p) > struct pf_state *state; > struct pfsync_state *p, *pstore; > u_int32_t nr = 0; > + struct pf_state **hold_states; > + u_int32_t hold_len, i; > > if (ps->ps_len == 0) { > nr = pf_status.states; > ps->ps_len = sizeof(struct pfsync_state) * nr; > break; > + } else { > + hold_len = ps->ps_len / sizeof(struct pfsync_state); > + hold_len = MIN(hold_len, pf_status.states); > } > > pstore = malloc(sizeof(*pstore), M_TEMP, M_WAITOK); > + hold_states = mallocarray(hold_len + 1, > + sizeof(struct pf_state *), M_TEMP, M_WAITOK | M_ZERO); > > p = ps->ps_states; > > + i = 0; > NET_LOCK(); > PF_STATE_ENTER_READ(); > - state = TAILQ_FIRST(&state_list); > - while (state) { > + TAILQ_FOREACH(state, &state_list, entry_list) { > + hold_states[i++] = pf_state_ref(state); > + if (i >= hold_len) > + break; > + } > + PF_STATE_EXIT_READ(); > + NET_UNLOCK(); > + > + i = 0; > + while ((state = hold_states[i++]) != NULL) { > if (state->timeout != PFTM_UNLINKED) { > - if ((nr+1) * sizeof(*p) > ps->ps_len) > - break; > pf_state_export(pstore, state); > error = copyout(pstore, p, sizeof(*p)); > - if (error) { > - free(pstore, M_TEMP, sizeof(*pstore)); > - PF_STATE_EXIT_READ(); > - NET_UNLOCK(); > - goto fail; > - } > + if (error) > + break; > p++; > nr++; > } > - state = TAILQ_NEXT(state, entry_list); > + pf_state_unref(state); > } > - PF_STATE_EXIT_READ(); > - NET_UNLOCK(); > > - ps->ps_len = sizeof(struct pfsync_state) * nr; > + if (error) { > + pf_state_unref(state); > + while ((state = hold_states[i++]) != NULL) > + pf_state_unref(state); > + } else > + ps->ps_len = sizeof(struct pfsync_state) * nr; > > free(pstore, M_TEMP, sizeof(*pstore)); > + free(hold_states, M_TEMP, > + sizeof(struct pf_state *) * (hold_len + 1)); > break; > } > >
