Hello,
diff below fixes my a tiny glitch I've introduced with
commit [1] back in May. Fortunately the impact is hardly
noticeable (I think).
The current code reads as follows:
1495 pfr_add_tables(struct pfr_table *tbl, int size, int *nadd, int flags)
1496 {
....
1507 for (i = 0; i < size; i++) {
1508 YIELD(flags & PFR_FLAG_USERIOCTL);
1509 if (COPYIN(tbl+i, &key.pfrkt_t, sizeof(key.pfrkt_t),
flags))
1510 senderr(EFAULT);
1511 if (pfr_validate_table(&key.pfrkt_t, PFR_TFLAG_USRMASK,
1512 flags & PFR_FLAG_USERIOCTL))
1513 senderr(EINVAL);
1514 key.pfrkt_flags |= PFR_TFLAG_ACTIVE;
1515 p = pfr_create_ktable(&key.pfrkt_t, tzero, 0,
1516 (flags & PFR_FLAG_USERIOCTL? PR_WAITOK : PR_NOWAIT));
1517 if (p == NULL)
1518 senderr(ENOMEM);
1519
1520 /*
1521 * Note: we also pre-allocate a root table here. We keep it
1522 * at ->pfrkt_root, which we must not forget about.
1523 */
1524 key.pfrkt_flags = 0;
....
1551 }
1552
1553 /*
1554 * auxq contains freshly allocated tables with no dups.
1555 * also note there are no rulesets attached, because
1556 * the attach operation requires PF_LOCK().
1557 */
1558 NET_LOCK();
1559 PF_LOCK();
1560 SLIST_FOREACH_SAFE(n, &auxq, pfrkt_workq, w) {
1561 p = RB_FIND(pfr_ktablehead, &pfr_ktables, n);
1562 if (p == NULL) {
1563 SLIST_REMOVE(&auxq, n, pfr_ktable, pfrkt_workq);
1564 SLIST_INSERT_HEAD(&addq, n, pfrkt_workq);
1565 xadd++;
1566 } else if (!(flags & PFR_FLAG_DUMMY) &&
1567 !(p->pfrkt_flags & PFR_TFLAG_ACTIVE)) {
1568 p->pfrkt_nflags = (p->pfrkt_flags &
1569 ~PFR_TFLAG_USRMASK) | key.pfrkt_flags;
1570 SLIST_INSERT_HEAD(&changeq, p, pfrkt_workq);
1571 }
1572 }
at line 1569 local variable 'key.pfrkt_flags' is always 0, because
it's set to 0 at line 1524. Thus the code has no effect. We should
be using 'n->pfrkt_flags' instead at line 1569.
It took me a while to figure out when else branch at line 1568 gets
executed. If I understand code right the branch handles kind of
edge case which deals with situation the table exists already but
transaction which inserts/creates table is not yet committed.
For example consider situation of pfctl which creates a table 'test'
via pfr_ina_define() (on behalf of transaction when it loads pf.conf).
The pfctl process gets terminated for whatever reason before transaction
is committed. In this case we might be left with inactive table hanging
in pf(4) driver. Such table is never reported by 'pfctl -sT' but is
still present in pfr_ktables tree.
There is pf_tbltrans.c test program which emulates a theoretical failure
of pfctl. It creates a table via transaction (like parse.y does) but
it never commits the transaction. This is what happens when we run
commands below on current pf(4):
netlock# ./pf_tbltrans test
netlock# pfctl -t test -T add 192.168.1.0/24
pfctl: Table does not exist
netlock# pfctl -FT
0 tables deleted.
netlock# pfctl -t test -T add 192.168.1.0/24
pfctl: Table does not exist
There 'pfctl -t test -T add 192.168.1.0/24' fails because it is not able
to add address to inactive (not committed) table 'test'. As soon as
we replay the same scenario on system with diff applied the
story looks as follows:
netlock# ./pf_tbltrans test
netlock# pfctl -t test -T add 192.168.1.0/24
1/1 addresses added.
The reason why it works is that fixed line 1569 kicks in and
active flag gets set which makes table active ('visible') for
all other operations.
OK to commit?
thanks and
regards
sashan
[1] https://marc.info/?l=openbsd-cvs&m=165222430111103&w=2
--------8<---------------8<-----------------8<---------------8<---------
diff --git a/sys/net/pf_table.c b/sys/net/pf_table.c
index f537aac2387..f6d5c355b1f 100644
--- a/sys/net/pf_table.c
+++ b/sys/net/pf_table.c
@@ -1566,7 +1566,7 @@ pfr_add_tables(struct pfr_table *tbl, int size, int
*nadd, int flags)
} else if (!(flags & PFR_FLAG_DUMMY) &&
!(p->pfrkt_flags & PFR_TFLAG_ACTIVE)) {
p->pfrkt_nflags = (p->pfrkt_flags &
- ~PFR_TFLAG_USRMASK) | key.pfrkt_flags;
+ ~PFR_TFLAG_USRMASK) | n->pfrkt_flags;
SLIST_INSERT_HEAD(&changeq, p, pfrkt_workq);
}
}
#include <err.h>
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <net/if.h>
#include <netinet/in.h>
#include <net/pfvar.h>
#include <sys/ioctl.h>
int
main(int argc, const char *argv[])
{
int pf;
struct pfioc_table io;
struct pfioc_trans pfioc_t;
struct pfioc_trans_e pfioc_e;
if (argc != 2) {
fprintf(stderr, "run as '%s tblname'\n", argv[0]);
return (1);
}
pf = open("/dev/pf", O_RDWR);
if (pf == -1)
err(1, "Ubale to open /dev/pf: %s\n", strerror(errno));
memset(&pfioc_t, 0, sizeof (pfioc_t));
memset(&pfioc_e, 0, sizeof (pfioc_e));
pfioc_e.type = PF_TRANS_TABLE;
pfioc_t.size = 1;
pfioc_t.esize = sizeof (struct pfioc_trans_e);
pfioc_t.array = &pfioc_e;
if (ioctl(pf, DIOCXBEGIN, &pfioc_t) == -1) {
err(1, "DIOCXBEGIN: ");
}
memset(&io, 0, sizeof (struct pfioc_table));
strlcpy(io.pfrio_table.pfrt_name, argv[1], PF_TABLE_NAME_SIZE);
io.pfrio_table.pfrt_flags = PFR_TFLAG_PERSIST|PFR_TFLAG_CONST;
io.pfrio_ticket = pfioc_e.ticket;
io.pfrio_esize = sizeof (struct pfr_addr);
if (ioctl(pf, DIOCRINADEFINE, &io) == -1) {
err(1, "DIOCRINADEFINE: ");
}
close(pf);
return (0);
}
