svn commit: r234096 - projects/pf/head/sys/contrib/pf/net
Gleb Smirnoff
glebius at FreeBSD.org
Tue Apr 10 13:31:39 UTC 2012
Author: glebius
Date: Tue Apr 10 13:31:38 2012
New Revision: 234096
URL: http://svn.freebsd.org/changeset/base/234096
Log:
Get rid of unsafe PF_COPYIN() and PF_COPYOUT, that drop locks. Achieve
this mostly by allocating enough temporary memory before obtaining
locks.
Modified:
projects/pf/head/sys/contrib/pf/net/pf_if.c
projects/pf/head/sys/contrib/pf/net/pf_ioctl.c
projects/pf/head/sys/contrib/pf/net/pfvar.h
Modified: projects/pf/head/sys/contrib/pf/net/pf_if.c
==============================================================================
--- projects/pf/head/sys/contrib/pf/net/pf_if.c Tue Apr 10 10:50:55 2012 (r234095)
+++ projects/pf/head/sys/contrib/pf/net/pf_if.c Tue Apr 10 13:31:38 2012 (r234096)
@@ -719,32 +719,24 @@ pfi_update_status(const char *name, stru
}
}
-int
+void
pfi_get_ifaces(const char *name, struct pfi_kif *buf, int *size)
{
struct pfi_kif *p, *nextp;
int n = 0;
- int error;
for (p = RB_MIN(pfi_ifhead, &V_pfi_ifs); p; p = nextp) {
nextp = RB_NEXT(pfi_ifhead, &V_pfi_ifs, p);
if (pfi_skip_if(name, p))
continue;
- if (*size > n++) {
- if (!p->pfik_tzero)
- p->pfik_tzero = time_second;
- pfi_kif_ref(p, PFI_KIF_REF_RULE);
- PF_COPYOUT(p, buf++, sizeof(*buf), error);
- if (error) {
- pfi_kif_unref(p, PFI_KIF_REF_RULE);
- return (EFAULT);
- }
- nextp = RB_NEXT(pfi_ifhead, &V_pfi_ifs, p);
- pfi_kif_unref(p, PFI_KIF_REF_RULE);
- }
+ if (*size <= n++)
+ break;
+ if (!p->pfik_tzero)
+ p->pfik_tzero = time_second;
+ bcopy(p, buf++, sizeof(*buf));
+ nextp = RB_NEXT(pfi_ifhead, &V_pfi_ifs, p);
}
*size = n;
- return (0);
}
static int
Modified: projects/pf/head/sys/contrib/pf/net/pf_ioctl.c
==============================================================================
--- projects/pf/head/sys/contrib/pf/net/pf_ioctl.c Tue Apr 10 10:50:55 2012 (r234095)
+++ projects/pf/head/sys/contrib/pf/net/pf_ioctl.c Tue Apr 10 13:31:38 2012 (r234096)
@@ -1846,7 +1846,7 @@ DIOCGETSTATES_full:
error = copyout(pstore, ps->ps_states,
sizeof(struct pfsync_state) * nr);
if (error) {
- free(p, M_TEMP);
+ free(pstore, M_TEMP);
goto fail;
}
ps->ps_len = sizeof(struct pfsync_state) * nr;
@@ -2778,155 +2778,143 @@ DIOCGETSTATES_full:
case DIOCXBEGIN: {
struct pfioc_trans *io = (struct pfioc_trans *)addr;
- struct pfioc_trans_e *ioe;
- struct pfr_table *table;
+ struct pfioc_trans_e *ioes, *ioe;
int i;
if (io->esize != sizeof(*ioe)) {
error = ENODEV;
goto fail;
}
- ioe = malloc(sizeof(*ioe), M_TEMP, M_WAITOK);
- table = malloc(sizeof(*table), M_TEMP, M_WAITOK);
- PF_LOCK();
- for (i = 0; i < io->size; i++) {
- PF_COPYIN(io->array+i, ioe, sizeof(*ioe), error);
+ ioes = malloc(sizeof(*ioe) * io->size, M_TEMP, M_WAITOK);
+ for (i = 0, ioe = ioes; i < io->size; i++, ioe++) {
+ error = copyin(io->array + i, ioe, sizeof(*ioe));
if (error) {
- PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
- error = EFAULT;
+ free(ioes, M_TEMP);
goto fail;
}
+ }
+ PF_LOCK();
+ for (i = 0, ioe = ioes; i < io->size; i++, ioe++) {
switch (ioe->rs_num) {
#ifdef ALTQ
case PF_RULESET_ALTQ:
if (ioe->anchor[0]) {
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
error = EINVAL;
goto fail;
}
if ((error = pf_begin_altq(&ioe->ticket))) {
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
goto fail;
}
break;
#endif /* ALTQ */
case PF_RULESET_TABLE:
- bzero(table, sizeof(*table));
- strlcpy(table->pfrt_anchor, ioe->anchor,
- sizeof(table->pfrt_anchor));
- if ((error = pfr_ina_begin(table,
+ {
+ struct pfr_table table;
+
+ bzero(&table, sizeof(table));
+ strlcpy(table.pfrt_anchor, ioe->anchor,
+ sizeof(table.pfrt_anchor));
+ if ((error = pfr_ina_begin(&table,
&ioe->ticket, NULL, 0))) {
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
goto fail;
}
break;
+ }
default:
if ((error = pf_begin_rules(&ioe->ticket,
ioe->rs_num, ioe->anchor))) {
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
goto fail;
}
break;
}
- PF_COPYOUT(ioe, io->array+i, sizeof(io->array[i]),
- error);
- if (error) {
- PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
- error = EFAULT;
- goto fail;
- }
}
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ for (i = 0, ioe = ioes; i < io->size; i++, ioe++) {
+ error = copyout(ioe, io->array + i,
+ sizeof(io->array[i]));
+ if (error)
+ break;
+ }
+ free(ioes, M_TEMP);
break;
}
case DIOCXROLLBACK: {
struct pfioc_trans *io = (struct pfioc_trans *)addr;
- struct pfioc_trans_e *ioe;
- struct pfr_table *table;
+ struct pfioc_trans_e *ioe, *ioes;
int i;
if (io->esize != sizeof(*ioe)) {
error = ENODEV;
goto fail;
}
- ioe = malloc(sizeof(*ioe), M_TEMP, M_WAITOK);
- table = malloc(sizeof(*table), M_TEMP, M_WAITOK);
- PF_LOCK();
- for (i = 0; i < io->size; i++) {
- PF_COPYIN(io->array+i, ioe, sizeof(*ioe), error);
+ ioes = malloc(sizeof(*ioe) * io->size, M_TEMP, M_WAITOK);
+ for (i = 0, ioe = ioes; i < io->size; i++, ioe++) {
+ error = copyin(io->array + i, ioe, sizeof(*ioe));
if (error) {
- PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
- error = EFAULT;
+ free(ioes, M_TEMP);
goto fail;
}
+ }
+ PF_LOCK();
+ for (i = 0, ioe = ioes; i < io->size; i++, ioe++) {
switch (ioe->rs_num) {
#ifdef ALTQ
case PF_RULESET_ALTQ:
if (ioe->anchor[0]) {
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
error = EINVAL;
goto fail;
}
if ((error = pf_rollback_altq(ioe->ticket))) {
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
goto fail; /* really bad */
}
break;
#endif /* ALTQ */
case PF_RULESET_TABLE:
- bzero(table, sizeof(*table));
- strlcpy(table->pfrt_anchor, ioe->anchor,
- sizeof(table->pfrt_anchor));
- if ((error = pfr_ina_rollback(table,
+ {
+ struct pfr_table table;
+
+ bzero(&table, sizeof(table));
+ strlcpy(table.pfrt_anchor, ioe->anchor,
+ sizeof(table.pfrt_anchor));
+ if ((error = pfr_ina_rollback(&table,
ioe->ticket, NULL, 0))) {
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
goto fail; /* really bad */
}
break;
+ }
default:
if ((error = pf_rollback_rules(ioe->ticket,
ioe->rs_num, ioe->anchor))) {
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
goto fail; /* really bad */
}
break;
}
}
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
break;
}
case DIOCXCOMMIT: {
struct pfioc_trans *io = (struct pfioc_trans *)addr;
- struct pfioc_trans_e *ioe;
- struct pfr_table *table;
+ struct pfioc_trans_e *ioe, *ioes;
struct pf_ruleset *rs;
int i;
@@ -2934,34 +2922,30 @@ DIOCGETSTATES_full:
error = ENODEV;
goto fail;
}
- ioe = malloc(sizeof(*ioe), M_TEMP, M_WAITOK);
- table = malloc(sizeof(*table), M_TEMP, M_WAITOK);
- PF_LOCK();
- /* first makes sure everything will succeed */
- for (i = 0; i < io->size; i++) {
- PF_COPYIN(io->array+i, ioe, sizeof(*ioe), error);
+ ioes = malloc(sizeof(*ioe) * io->size, M_TEMP, M_WAITOK);
+ for (i = 0, ioe = ioes; i < io->size; i++, ioe++) {
+ error = copyin(io->array + i, ioe, sizeof(*ioe));
if (error) {
- PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
- error = EFAULT;
+ free(ioes, M_TEMP);
goto fail;
}
+ }
+ PF_LOCK();
+ /* First makes sure everything will succeed. */
+ for (i = 0, ioe = ioes; i < io->size; i++, ioe++) {
switch (ioe->rs_num) {
#ifdef ALTQ
case PF_RULESET_ALTQ:
if (ioe->anchor[0]) {
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
error = EINVAL;
goto fail;
}
if (!V_altqs_inactive_open || ioe->ticket !=
V_ticket_altqs_inactive) {
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
error = EBUSY;
goto fail;
}
@@ -2972,8 +2956,7 @@ DIOCGETSTATES_full:
if (rs == NULL || !rs->topen || ioe->ticket !=
rs->tticket) {
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
error = EBUSY;
goto fail;
}
@@ -2982,8 +2965,7 @@ DIOCGETSTATES_full:
if (ioe->rs_num < 0 || ioe->rs_num >=
PF_RULESET_MAX) {
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
error = EINVAL;
goto fail;
}
@@ -2993,61 +2975,52 @@ DIOCGETSTATES_full:
rs->rules[ioe->rs_num].inactive.ticket !=
ioe->ticket) {
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
error = EBUSY;
goto fail;
}
break;
}
}
- /* now do the commit - no errors should happen here */
- for (i = 0; i < io->size; i++) {
- PF_COPYIN(io->array+i, ioe, sizeof(*ioe), error);
- if (error) {
- PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
- error = EFAULT;
- goto fail;
- }
+ /* Now do the commit - no errors should happen here. */
+ for (i = 0, ioe = ioes; i < io->size; i++, ioe++) {
switch (ioe->rs_num) {
#ifdef ALTQ
case PF_RULESET_ALTQ:
if ((error = pf_commit_altq(ioe->ticket))) {
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
goto fail; /* really bad */
}
break;
#endif /* ALTQ */
case PF_RULESET_TABLE:
- bzero(table, sizeof(*table));
- strlcpy(table->pfrt_anchor, ioe->anchor,
- sizeof(table->pfrt_anchor));
- if ((error = pfr_ina_commit(table, ioe->ticket,
- NULL, NULL, 0))) {
+ {
+ struct pfr_table table;
+
+ bzero(&table, sizeof(table));
+ strlcpy(table.pfrt_anchor, ioe->anchor,
+ sizeof(table.pfrt_anchor));
+ if ((error = pfr_ina_commit(&table,
+ ioe->ticket, NULL, NULL, 0))) {
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
goto fail; /* really bad */
}
break;
+ }
default:
if ((error = pf_commit_rules(ioe->ticket,
ioe->rs_num, ioe->anchor))) {
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
goto fail; /* really bad */
}
break;
}
}
PF_UNLOCK();
- free(table, M_TEMP);
- free(ioe, M_TEMP);
+ free(ioes, M_TEMP);
break;
}
@@ -3055,9 +3028,8 @@ DIOCGETSTATES_full:
struct pfioc_src_nodes *psn = (struct pfioc_src_nodes *)addr;
struct pf_src_node *n, *p, *pstore;
u_int32_t nr = 0;
- int space = psn->psn_len;
- if (space == 0) {
+ if (psn->psn_len == 0) {
PF_LOCK();
RB_FOREACH(n, pf_src_tree, &V_tree_src_tracking)
nr++;
@@ -3066,43 +3038,41 @@ DIOCGETSTATES_full:
break;
}
- pstore = malloc(sizeof(*pstore), M_TEMP, M_WAITOK);
+ p = pstore = malloc(psn->psn_len, M_TEMP, M_WAITOK);
PF_LOCK();
- p = psn->psn_src_nodes;
RB_FOREACH(n, pf_src_tree, &V_tree_src_tracking) {
int secs = time_second, diff;
if ((nr + 1) * sizeof(*p) > (unsigned)psn->psn_len)
break;
- bcopy(n, pstore, sizeof(*pstore));
+ bcopy(n, p, sizeof(struct pf_src_node));
if (n->rule.ptr != NULL)
- pstore->rule.nr = n->rule.ptr->nr;
- pstore->creation = secs - pstore->creation;
- if (pstore->expire > secs)
- pstore->expire -= secs;
+ p->rule.nr = n->rule.ptr->nr;
+ p->creation = secs - p->creation;
+ if (p->expire > secs)
+ p->expire -= secs;
else
- pstore->expire = 0;
+ p->expire = 0;
- /* adjust the connection rate estimate */
+ /* Adjust the connection rate estimate. */
diff = secs - n->conn_rate.last;
if (diff >= n->conn_rate.seconds)
- pstore->conn_rate.count = 0;
+ p->conn_rate.count = 0;
else
- pstore->conn_rate.count -=
+ p->conn_rate.count -=
n->conn_rate.count * diff /
n->conn_rate.seconds;
-
- PF_COPYOUT(pstore, p, sizeof(*p), error);
- if (error) {
- PF_UNLOCK();
- free(pstore, M_TEMP);
- goto fail;
- }
p++;
nr++;
}
PF_UNLOCK();
+ error = copyout(pstore, psn->psn_src_nodes,
+ sizeof(struct pf_src_node) * nr);
+ if (error) {
+ free(pstore, M_TEMP);
+ goto fail;
+ }
psn->psn_len = sizeof(struct pf_src_node) * nr;
free(pstore, M_TEMP);
break;
@@ -3170,15 +3140,21 @@ DIOCGETSTATES_full:
case DIOCIGETIFACES: {
struct pfioc_iface *io = (struct pfioc_iface *)addr;
+ struct pfi_kif *ifstore;
if (io->pfiio_esize != sizeof(struct pfi_kif)) {
error = ENODEV;
break;
}
+
+ ifstore = malloc(io->pfiio_size * sizeof(struct pfi_kif),
+ M_TEMP, M_WAITOK);
PF_LOCK();
- error = pfi_get_ifaces(io->pfiio_name, io->pfiio_buffer,
- &io->pfiio_size);
+ pfi_get_ifaces(io->pfiio_name, ifstore, &io->pfiio_size);
PF_UNLOCK();
+ error = copyout(ifstore, io->pfiio_buffer, io->pfiio_size *
+ sizeof(struct pfi_kif));
+ free(ifstore, M_TEMP);
break;
}
Modified: projects/pf/head/sys/contrib/pf/net/pfvar.h
==============================================================================
--- projects/pf/head/sys/contrib/pf/net/pfvar.h Tue Apr 10 10:50:55 2012 (r234095)
+++ projects/pf/head/sys/contrib/pf/net/pfvar.h Tue Apr 10 13:31:38 2012 (r234096)
@@ -242,18 +242,6 @@ extern struct rwlock pf_rules_lock;
#define PF_RULES_WUNLOCK() rw_wunlock(&pf_rules_lock)
#define PF_RULES_WASSERT() rw_assert(&pf_rules_lock, RA_WLOCKED)
-#define PF_COPYIN(uaddr, kaddr, len, r) do { \
- PF_UNLOCK(); \
- r = copyin((uaddr), (kaddr), (len)); \
- PF_LOCK(); \
-} while(0)
-
-#define PF_COPYOUT(kaddr, uaddr, len, r) do { \
- PF_UNLOCK(); \
- r = copyout((kaddr), (uaddr), (len)); \
- PF_LOCK(); \
-} while(0)
-
#define PF_MODVER 1
#define PFLOG_MODVER 1
#define PFSYNC_MODVER 1
@@ -1928,7 +1916,7 @@ int pfi_dynaddr_setup(struct pf_addr_w
void pfi_dynaddr_remove(struct pf_addr_wrap *);
void pfi_dynaddr_copyout(struct pf_addr_wrap *);
void pfi_update_status(const char *, struct pf_status *);
-int pfi_get_ifaces(const char *, struct pfi_kif *, int *);
+void pfi_get_ifaces(const char *, struct pfi_kif *, int *);
int pfi_set_flags(const char *, int);
int pfi_clear_flags(const char *, int);
More information about the svn-src-projects
mailing list