Hello,
there was a brief email discussion off-list between bluhm, mbuhl and me
preceding diff below. Let me briefly summarize a context for tech@ here.
Moritz (mbuhl) is working on a fix reported syzkaller by syzkaller [1].
In order to fix it he wants to move both locks (pf_lock and net_lock)
into a for loop, so copyin() operation at line 2479 will happen
outside of them:
2462 case DIOCXBEGIN: {
2463 struct pfioc_trans *io = (struct pfioc_trans *)addr;
2464 struct pfioc_trans_e *ioe;
2465 struct pfr_table *table;
2466 int i;
2467
2468 if (io->esize != sizeof(*ioe)) {
2469 error = ENODEV;
2470 goto fail;
2471 }
2472 ioe = malloc(sizeof(*ioe), M_TEMP, M_WAITOK);
2473 table = malloc(sizeof(*table), M_TEMP, M_WAITOK);
2474 NET_LOCK();
2475 PF_LOCK();
2476 pf_default_rule_new = pf_default_rule;
2477 memset(&pf_trans_set, 0, sizeof(pf_trans_set));
2478 for (i = 0; i < io->size; i++) {
2479 if (copyin(io->array+i, ioe, sizeof(*ioe))) {
2480 PF_UNLOCK();
2481 NET_UNLOCK();
2482 free(table, M_TEMP, sizeof(*table));
2483 free(ioe, M_TEMP, sizeof(*ioe));
2484 error = EFAULT;
2485 goto fail;
2486 }
....
2496 switch (ioe->type) {
2497 case PF_TRANS_TABLE:
2498 memset(table, 0, sizeof(*table));
2499 strlcpy(table->pfrt_anchor, ioe->anchor,
2500 sizeof(table->pfrt_anchor));
2501 if ((error = pfr_ina_begin(table,
2502 &ioe->ticket, NULL, 0))) {
2503 PF_UNLOCK();
2504 NET_UNLOCK();
2505 free(table, M_TEMP,
sizeof(*table));
2506 free(ioe, M_TEMP, sizeof(*ioe));
2507 goto fail;
2508 }
2509 break;
moritz's fix releases and re-acquires both locks with every iteration
of for loop at line 2478. Such change may alter behavior of processes,
which update pf(4) configuration simultaneously. I think this subtle
change is not desired.
in order to keep behavior same I'd like to introduce pfioctl_rw,
which only purpose is to synchronize simultaneous ioctl(2) operations
on pf(4).
bluhm@ poked into cvs history already. There used to be pf_consistency_lock,
which got removed in 1.307 (2017). I think time has come to re-introduce
it under slightly different name/purpose.
as I've said the purpose of pfioctl_rw is to synchronize processes,
which perform operation on /dev/pf.
other locks we have (pf_lock, pf_state_lock), synchronize packets (pf_test())
with timer (pf_purge_thread()) and process (pfioctl()), which alters pf(4)
configuration (the winner of pfioctl_rw).
OK?
thanks and
regards
sashan
[1]
https://syzkaller.appspot.com/bug?id=bcea66b7efc68fee34781f448e106622b35bba6e
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
index dbbc79c0a0e..329284ce6a6 100644
--- a/sys/net/pf_ioctl.c
+++ b/sys/net/pf_ioctl.c
@@ -150,6 +150,7 @@ TAILQ_HEAD(pf_tags, pf_tagname) pf_tags =
TAILQ_HEAD_INITIALIZER(pf_tags),
*/
struct rwlock pf_lock = RWLOCK_INITIALIZER("pf_lock");
struct rwlock pf_state_lock = RWLOCK_INITIALIZER("pf_state_lock");
+struct rwlock pfioctl_rw = RWLOCK_INITIALIZER("pfioctl_rw");
#if (PF_QNAME_SIZE != PF_TAG_NAME_SIZE)
#error PF_QNAME_SIZE must be equal to PF_TAG_NAME_SIZE
@@ -1142,6 +1143,11 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
return (EACCES);
}
+ if (flags & FWRITE)
+ rw_enter_write(&pfioctl_rw);
+ else
+ rw_enter_read(&pfioctl_rw);
+
switch (cmd) {
case DIOCSTART:
@@ -2945,8 +2951,10 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
case DIOCSETIFFLAG: {
struct pfioc_iface *io = (struct pfioc_iface *)addr;
- if (io == NULL)
- return (EINVAL);
+ if (io == NULL) {
+ error = EINVAL;
+ break;
+ }
NET_LOCK();
PF_LOCK();
@@ -2959,8 +2967,10 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
case DIOCCLRIFFLAG: {
struct pfioc_iface *io = (struct pfioc_iface *)addr;
- if (io == NULL)
- return (EINVAL);
+ if (io == NULL) {
+ error = EINVAL;
+ break;
+ }
NET_LOCK();
PF_LOCK();
@@ -3020,6 +3030,11 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
break;
}
fail:
+ if (flags & FWRITE)
+ rw_exit_write(&pfioctl_rw);
+ else
+ rw_exit_read(&pfioctl_rw);
+
return (error);
}
