On Sun, May 08, 2022 at 06:15:49PM +0200, Alexandr Nedvedicky wrote:
> OK ?
3 comments inline
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
> index 8315b115474..1f036e1368f 100644
> --- a/sys/net/pf_ioctl.c
> +++ b/sys/net/pf_ioctl.c
> @@ -2217,12 +2217,8 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int
> flags, struct proc *p)
> error = ENODEV;
> goto fail;
> }
> - NET_LOCK();
> - PF_LOCK();
> error = pfr_add_tables(io->pfrio_buffer, io->pfrio_size,
> &io->pfrio_nadd, io->pfrio_flags | PFR_FLAG_USERIOCTL);
> - PF_UNLOCK();
> - NET_UNLOCK();
> break;
> }
>
> diff --git a/sys/net/pf_table.c b/sys/net/pf_table.c
> index fb23bcabe04..79fd3e0447b 100644
> --- a/sys/net/pf_table.c
> +++ b/sys/net/pf_table.c
> @@ -189,6 +189,7 @@ void pfr_clstats_ktable(struct
> pfr_ktable *, time_t, int);
> struct pfr_ktable *pfr_create_ktable(struct pfr_table *, time_t, int,
> int);
> void pfr_destroy_ktables(struct pfr_ktableworkq *, int);
> +void pfr_destroy_ktables_aux(struct pfr_ktableworkq *);
> void pfr_destroy_ktable(struct pfr_ktable *, int);
> int pfr_ktable_compare(struct pfr_ktable *,
> struct pfr_ktable *);
> @@ -1493,14 +1494,16 @@ pfr_clr_tables(struct pfr_table *filter, int *ndel,
> int flags)
> int
> pfr_add_tables(struct pfr_table *tbl, int size, int *nadd, int flags)
> {
> - struct pfr_ktableworkq addq, changeq;
> - struct pfr_ktable *p, *q, *r, key;
> + struct pfr_ktableworkq addq, changeq, auxq;
> + struct pfr_ktable *p, *q, *r, *n, *w, key;
> int i, rv, xadd = 0;
> time_t tzero = gettime();
>
> ACCEPT_FLAGS(flags, PFR_FLAG_DUMMY);
> SLIST_INIT(&addq);
> SLIST_INIT(&changeq);
> + SLIST_INIT(&auxq);
> + /* pre-allocate all memory outside of locks */
> for (i = 0; i < size; i++) {
> YIELD(flags & PFR_FLAG_USERIOCTL);
> if (COPYIN(tbl+i, &key.pfrkt_t, sizeof(key.pfrkt_t), flags))
> @@ -1509,65 +1512,142 @@ pfr_add_tables(struct pfr_table *tbl, int size, int
> *nadd, int flags)
> flags & PFR_FLAG_USERIOCTL))
> senderr(EINVAL);
> key.pfrkt_flags |= PFR_TFLAG_ACTIVE;
> - p = RB_FIND(pfr_ktablehead, &pfr_ktables, &key);
> + p = pfr_create_ktable(&key.pfrkt_t, tzero, 0,
> + !(flags & PFR_FLAG_USERIOCTL));
> + if (p == NULL)
> + senderr(ENOMEM);
> +
> + /*
> + * Note: we also pre-allocate a root table here. We keep it
> + * at ->pfrkt_root, which we must not forget about.
> + */
> + key.pfrkt_flags = 0;
> + memset(key.pfrkt_anchor, 0, sizeof(key.pfrkt_anchor));
> + p->pfrkt_root = pfr_create_ktable(&key.pfrkt_t, 0, 0,
> + !(flags & PFR_FLAG_USERIOCTL));
> + if (p->pfrkt_root == NULL) {
> + pfr_destroy_ktable(p, 0);
> + senderr(ENOMEM);
> + }
> +
> + SLIST_FOREACH(q, &auxq, pfrkt_workq) {
> + if (!pfr_ktable_compare(p, q)) {
> + /*
> + * We need no lock here, because `p` is empty,
> + * there are no rules or shadow tables
> + * attached.
> + */
> + pfr_destroy_ktable(p->pfrkt_root, 0);
> + p->pfrkt_root = NULL;
> + pfr_destroy_ktable(p, 0);
> + break;
This break...
> + }
> + }
... end here
> +
>
... and then we insert a destroyed p
> + SLIST_INSERT_HEAD(&auxq, p, pfrkt_workq);
> + }
> +
> + /*
> + * auxq contains freshly allocated tables with no dups.
> + * also note there are no rulesets attached, because
> + * the attach operation requires PF_LOCK().
> + */
> + NET_LOCK();
> + PF_LOCK();
> + SLIST_FOREACH_SAFE(n, &auxq, pfrkt_workq, w) {
> + p = RB_FIND(pfr_ktablehead, &pfr_ktables, n);
> if (p == NULL) {
> - p = pfr_create_ktable(&key.pfrkt_t, tzero, 1,
> - !(flags & PFR_FLAG_USERIOCTL));
> - if (p == NULL)
> - senderr(ENOMEM);
> - SLIST_FOREACH(q, &addq, pfrkt_workq) {
> - if (!pfr_ktable_compare(p, q)) {
> - pfr_destroy_ktable(p, 0);
> - goto _skip;
> - }
> + SLIST_REMOVE(&auxq, n, pfr_ktable, pfrkt_workq);
> + SLIST_INSERT_HEAD(&addq, n, pfrkt_workq);
> + } else if (!(flags & PFR_FLAG_DUMMY)) {
I compared the old and new code to see if it is equivalent.
Before the condtion looked like this.
} else if (!(p->pfrkt_flags & PFR_TFLAG_ACTIVE)) {
> + p->pfrkt_nflags = (p->pfrkt_flags &
> + ~PFR_TFLAG_USRMASK) | key.pfrkt_flags;
> + SLIST_INSERT_HEAD(&changeq, p, pfrkt_workq);
> + }
> + xadd++;
> + }
> +
> + if (!(flags & PFR_FLAG_DUMMY)) {
> + /*
> + * addq contains tables we have to insert and attach rules to
> + * them
> + *
> + * changeq contains tables we need to update
> + *
> + * auxq contains pre-allocated tables, we won't use and we must
> + * free them
> + */
> + SLIST_FOREACH_SAFE(p, &addq, pfrkt_workq, w) {
> + p->pfrkt_rs = pf_find_or_create_ruleset(
> + p->pfrkt_anchor);
> + if (p->pfrkt_rs == NULL) {
> + xadd--;
> + SLIST_REMOVE(&addq, p, pfr_ktable, pfrkt_workq);
> + SLIST_INSERT_HEAD(&auxq, p, pfrkt_workq);
> + continue;
> }
> - SLIST_INSERT_HEAD(&addq, p, pfrkt_workq);
> - xadd++;
> - if (!key.pfrkt_anchor[0])
> - goto _skip;
> + p->pfrkt_rs->tables++;
>
> - /* find or create root table */
> - bzero(key.pfrkt_anchor, sizeof(key.pfrkt_anchor));
> - r = RB_FIND(pfr_ktablehead, &pfr_ktables, &key);
> + if (!p->pfrkt_anchor[0]) {
> + q = p->pfrkt_root;
> + p->pfrkt_root = NULL;
> + SLIST_INSERT_HEAD(&auxq, q, pfrkt_workq);
> + continue;
> + }
> +
> + /* use pre-allocated root table as a key */
> + q = p->pfrkt_root;
> + p->pfrkt_root = NULL;
> + r = RB_FIND(pfr_ktablehead, &pfr_ktables, q);
> if (r != NULL) {
> p->pfrkt_root = r;
> - goto _skip;
> + SLIST_INSERT_HEAD(&auxq, q, pfrkt_workq);
> + continue;
> }
> - SLIST_FOREACH(q, &addq, pfrkt_workq) {
> - if (!pfr_ktable_compare(&key, q)) {
> - p->pfrkt_root = q;
> - goto _skip;
> + /*
> + * there is a chance we could create root table in
> + * earlier iteration. such table may exist in addq only
> + * then.
> + */
> + SLIST_FOREACH(r, &addq, pfrkt_workq) {
> + if (!pfr_ktable_compare(r, q)) {
> + /*
> + * `r` is our root table we've found
> + * earlier, `q` can get dropped.
> + */
> + p->pfrkt_root = r;
> + SLIST_INSERT_HEAD(&auxq, q,
> + pfrkt_workq);
> + continue;
This continue goes to the r list, but I think you want to continue p list.
> }
> }
> - key.pfrkt_flags = 0;
> - r = pfr_create_ktable(&key.pfrkt_t, 0, 1,
> - !(flags & PFR_FLAG_USERIOCTL));
> - if (r == NULL)
> - senderr(ENOMEM);
> - SLIST_INSERT_HEAD(&addq, r, pfrkt_workq);
> - p->pfrkt_root = r;
> - } else if (!(p->pfrkt_flags & PFR_TFLAG_ACTIVE)) {
> - SLIST_FOREACH(q, &changeq, pfrkt_workq)
> - if (!pfr_ktable_compare(&key, q))
> - goto _skip;
> - p->pfrkt_nflags = (p->pfrkt_flags &
> - ~PFR_TFLAG_USRMASK) | key.pfrkt_flags;
> - SLIST_INSERT_HEAD(&changeq, p, pfrkt_workq);
> - xadd++;
> + q->pfrkt_rs = pf_find_or_create_ruleset(
> + q->pfrkt_root->pfrkt_anchor);
> + /*
> + * root tables are attached to main ruleset,
> + * because ->pfrkt_anchor[0] == '\0'
> + */
> + KASSERT(q->pfrkt_rs == &pf_main_ruleset);
> + q->pfrkt_rs->tables++;
> + p->pfrkt_root = q;
> + SLIST_INSERT_HEAD(&addq, q, pfrkt_workq);
> }
> -_skip:
> - ;
> - }
> - if (!(flags & PFR_FLAG_DUMMY)) {
> +
> pfr_insert_ktables(&addq);
> pfr_setflags_ktables(&changeq);
> - } else
> - pfr_destroy_ktables(&addq, 0);
> + }
> + PF_UNLOCK();
> + NET_UNLOCK();
> +
> + pfr_destroy_ktables_aux(&auxq);
> + if (flags & PFR_FLAG_DUMMY)
> + pfr_destroy_ktables_aux(&addq);
> +
> if (nadd != NULL)
> *nadd = xadd;
> return (0);
> _bad:
> - pfr_destroy_ktables(&addq, 0);
> + pfr_destroy_ktables_aux(&auxq);
> return (rv);
> }
>
> @@ -2214,6 +2294,7 @@ pfr_create_ktable(struct pfr_table *tbl, time_t tzero,
> int attachruleset,
> kt->pfrkt_t = *tbl;
>
> if (attachruleset) {
> + PF_ASSERT_LOCKED();
> rs = pf_find_or_create_ruleset(tbl->pfrt_anchor);
> if (!rs) {
> pfr_destroy_ktable(kt, 0);
> @@ -2249,6 +2330,31 @@ pfr_destroy_ktables(struct pfr_ktableworkq *workq, int
> flushaddr)
> }
> }
>
> +void
> +pfr_destroy_ktables_aux(struct pfr_ktableworkq *auxq)
> +{
> + struct pfr_ktable *p;
> +
> + while ((p = SLIST_FIRST(auxq)) != NULL) {
> + SLIST_REMOVE_HEAD(auxq, pfrkt_workq);
> + /*
> + * There must be no extra data (rules, shadow tables, ...)
> + * attached, because auxq holds just empty memory to be
> + * initialized. Therefore we can also be called with no lock.
> + */
> + if (p->pfrkt_root != NULL) {
> + KASSERT(p->pfrkt_root->pfrkt_rs == NULL);
> + KASSERT(p->pfrkt_root->pfrkt_shadow == NULL);
> + KASSERT(p->pfrkt_root->pfrkt_root == NULL);
> + pfr_destroy_ktable(p->pfrkt_root, 0);
> + p->pfrkt_root = NULL;
> + }
> + KASSERT(p->pfrkt_rs == NULL);
> + KASSERT(p->pfrkt_shadow == NULL);
> + pfr_destroy_ktable(p, 0);
> + }
> +}
> +
> void
> pfr_destroy_ktable(struct pfr_ktable *kt, int flushaddr)
> {
