On Tue, May 03, 2022 at 09:31:51PM +0200, Alexander Bluhm wrote:
...
> The code is too complex to be sure what the reason of the syzkaller
> panic is. Sleep in malloc is correct anyway and may improve the
> situation.
>
> Functions with argument values 0 or 1 are hard to read. It would
> be much nicer to pass M_WAITOK or M_NOWAIT. And the variable name
> "intr" does not make sense anymore. pf does not run in interrupt
> context. Call it "mflags" like in pfi_kif_alloc(). Or "wait" like
> in other functions.
>
> Could you cleanup that also?
I renamed the intr parameter for pfr_create_ktable and pfr_attach_table
and passed it further up the call stack.
In pf_addr_setup I noticed that pf_tbladdr_setup also used to set intr.
I changed it to wait for both, pfi_dynaddr_setup and pf_tbladdr_setup.
Now it should be a little easier to notice sleeping points in pf_ioctl.c.
OK?
mbuhl
Index: sys/net/pf.c
===================================================================
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.1133
diff -u -p -r1.1133 pf.c
--- sys/net/pf.c 13 Jun 2022 12:48:00 -0000 1.1133
+++ sys/net/pf.c 26 Jun 2022 09:21:21 -0000
@@ -1585,11 +1585,11 @@ pf_purge_expired_states(u_int32_t maxche
}
int
-pf_tbladdr_setup(struct pf_ruleset *rs, struct pf_addr_wrap *aw)
+pf_tbladdr_setup(struct pf_ruleset *rs, struct pf_addr_wrap *aw, int wait)
{
if (aw->type != PF_ADDR_TABLE)
return (0);
- if ((aw->p.tbl = pfr_attach_table(rs, aw->v.tblname, 1)) == NULL)
+ if ((aw->p.tbl = pfr_attach_table(rs, aw->v.tblname, wait)) == NULL)
return (1);
return (0);
}
Index: sys/net/pf_if.c
===================================================================
RCS file: /cvs/src/sys/net/pf_if.c,v
retrieving revision 1.105
diff -u -p -r1.105 pf_if.c
--- sys/net/pf_if.c 16 May 2022 13:31:19 -0000 1.105
+++ sys/net/pf_if.c 26 Jun 2022 09:38:43 -0000
@@ -423,7 +423,7 @@ pfi_match_addr(struct pfi_dynaddr *dyn,
}
int
-pfi_dynaddr_setup(struct pf_addr_wrap *aw, sa_family_t af)
+pfi_dynaddr_setup(struct pf_addr_wrap *aw, sa_family_t af, int wait)
{
struct pfi_dynaddr *dyn;
char tblname[PF_TABLE_NAME_SIZE];
@@ -432,8 +432,7 @@ pfi_dynaddr_setup(struct pf_addr_wrap *a
if (aw->type != PF_ADDR_DYNIFTL)
return (0);
- if ((dyn = pool_get(&pfi_addr_pl, PR_WAITOK | PR_LIMITFAIL | PR_ZERO))
- == NULL)
+ if ((dyn = pool_get(&pfi_addr_pl, wait|PR_LIMITFAIL|PR_ZERO)) == NULL)
return (1);
if (!strcmp(aw->v.ifname, "self"))
@@ -466,7 +465,7 @@ pfi_dynaddr_setup(struct pf_addr_wrap *a
goto _bad;
}
- if ((dyn->pfid_kt = pfr_attach_table(ruleset, tblname, 1)) == NULL) {
+ if ((dyn->pfid_kt = pfr_attach_table(ruleset, tblname, wait)) == NULL) {
rv = 1;
goto _bad;
}
Index: sys/net/pf_ioctl.c
===================================================================
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.381
diff -u -p -r1.381 pf_ioctl.c
--- sys/net/pf_ioctl.c 10 May 2022 23:12:25 -0000 1.381
+++ sys/net/pf_ioctl.c 26 Jun 2022 09:35:17 -0000
@@ -891,8 +891,8 @@ int
pf_addr_setup(struct pf_ruleset *ruleset, struct pf_addr_wrap *addr,
sa_family_t af)
{
- if (pfi_dynaddr_setup(addr, af) ||
- pf_tbladdr_setup(ruleset, addr) ||
+ if (pfi_dynaddr_setup(addr, af, PR_WAITOK) ||
+ pf_tbladdr_setup(ruleset, addr, PR_WAITOK) ||
pf_rtlabel_add(addr))
return (EINVAL);
@@ -1413,7 +1413,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
if (rule->overload_tblname[0]) {
if ((rule->overload_tbl = pfr_attach_table(ruleset,
- rule->overload_tblname, 0)) == NULL)
+ rule->overload_tblname, PR_WAITOK)) == NULL)
error = EINVAL;
else
rule->overload_tbl->pfrkt_flags |=
PFR_TFLAG_ACTIVE;
@@ -1636,7 +1636,8 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
if (newrule->overload_tblname[0]) {
newrule->overload_tbl = pfr_attach_table(
- ruleset, newrule->overload_tblname, 0);
+ ruleset, newrule->overload_tblname,
+ PR_WAITOK);
if (newrule->overload_tbl == NULL)
error = EINVAL;
else
Index: sys/net/pf_table.c
===================================================================
RCS file: /cvs/src/sys/net/pf_table.c,v
retrieving revision 1.142
diff -u -p -r1.142 pf_table.c
--- sys/net/pf_table.c 16 Jun 2022 20:47:26 -0000 1.142
+++ sys/net/pf_table.c 26 Jun 2022 08:53:20 -0000
@@ -323,7 +323,7 @@ pfr_add_addrs(struct pfr_table *tbl, str
if (pfr_validate_table(tbl, 0, flags & PFR_FLAG_USERIOCTL))
return (EINVAL);
tmpkt = pfr_create_ktable(&pfr_nulltable, 0, 0,
- !(flags & PFR_FLAG_USERIOCTL));
+ (flags & PFR_FLAG_USERIOCTL? PR_WAITOK : PR_NOWAIT));
if (tmpkt == NULL)
return (ENOMEM);
SLIST_INIT(&workq);
@@ -540,7 +540,7 @@ pfr_set_addrs(struct pfr_table *tbl, str
if (kt->pfrkt_flags & PFR_TFLAG_CONST)
return (EPERM);
tmpkt = pfr_create_ktable(&pfr_nulltable, 0, 0,
- !(flags & PFR_FLAG_USERIOCTL));
+ (flags & PFR_FLAG_USERIOCTL? PR_WAITOK : PR_NOWAIT));
if (tmpkt == NULL)
return (ENOMEM);
pfr_mark_addrs(kt);
@@ -1513,7 +1513,7 @@ pfr_add_tables(struct pfr_table *tbl, in
senderr(EINVAL);
key.pfrkt_flags |= PFR_TFLAG_ACTIVE;
p = pfr_create_ktable(&key.pfrkt_t, tzero, 0,
- !(flags & PFR_FLAG_USERIOCTL));
+ (flags & PFR_FLAG_USERIOCTL? PR_WAITOK : PR_NOWAIT));
if (p == NULL)
senderr(ENOMEM);
@@ -1524,7 +1524,7 @@ pfr_add_tables(struct pfr_table *tbl, in
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));
+ (flags & PFR_FLAG_USERIOCTL? PR_WAITOK : PR_NOWAIT));
if (p->pfrkt_root == NULL) {
pfr_destroy_ktable(p, 0);
senderr(ENOMEM);
@@ -1910,7 +1910,7 @@ pfr_ina_define(struct pfr_table *tbl, st
kt = RB_FIND(pfr_ktablehead, &pfr_ktables, (struct pfr_ktable *)tbl);
if (kt == NULL) {
kt = pfr_create_ktable(tbl, 0, 1,
- !(flags & PFR_FLAG_USERIOCTL));
+ (flags & PFR_FLAG_USERIOCTL? PR_WAITOK : PR_NOWAIT));
if (kt == NULL)
return (ENOMEM);
SLIST_INSERT_HEAD(&tableq, kt, pfrkt_workq);
@@ -1927,7 +1927,7 @@ pfr_ina_define(struct pfr_table *tbl, st
goto _skip;
}
rt = pfr_create_ktable(&key.pfrkt_t, 0, 1,
- !(flags & PFR_FLAG_USERIOCTL));
+ (flags & PFR_FLAG_USERIOCTL? PR_WAITOK : PR_NOWAIT));
if (rt == NULL) {
pfr_destroy_ktables(&tableq, 0);
return (ENOMEM);
@@ -1937,7 +1937,8 @@ pfr_ina_define(struct pfr_table *tbl, st
} else if (!(kt->pfrkt_flags & PFR_TFLAG_INACTIVE))
xadd++;
_skip:
- shadow = pfr_create_ktable(tbl, 0, 0, !(flags & PFR_FLAG_USERIOCTL));
+ shadow = pfr_create_ktable(tbl, 0, 0,
+ (flags & PFR_FLAG_USERIOCTL? PR_WAITOK : PR_NOWAIT));
if (shadow == NULL) {
pfr_destroy_ktables(&tableq, 0);
return (ENOMEM);
@@ -2286,15 +2287,12 @@ pfr_clstats_ktable(struct pfr_ktable *kt
struct pfr_ktable *
pfr_create_ktable(struct pfr_table *tbl, time_t tzero, int attachruleset,
- int intr)
+ int wait)
{
struct pfr_ktable *kt;
struct pf_ruleset *rs;
- if (intr)
- kt = pool_get(&pfr_ktable_pl, PR_NOWAIT|PR_ZERO|PR_LIMITFAIL);
- else
- kt = pool_get(&pfr_ktable_pl, PR_WAITOK|PR_ZERO|PR_LIMITFAIL);
+ kt = pool_get(&pfr_ktable_pl, wait|PR_ZERO|PR_LIMITFAIL);
if (kt == NULL)
return (NULL);
kt->pfrkt_t = *tbl;
@@ -2533,7 +2531,7 @@ pfr_update_stats(struct pfr_ktable *kt,
}
struct pfr_ktable *
-pfr_attach_table(struct pf_ruleset *rs, char *name, int intr)
+pfr_attach_table(struct pf_ruleset *rs, char *name, int wait)
{
struct pfr_ktable *kt, *rt;
struct pfr_table tbl;
@@ -2545,14 +2543,14 @@ pfr_attach_table(struct pf_ruleset *rs,
strlcpy(tbl.pfrt_anchor, ac->path, sizeof(tbl.pfrt_anchor));
kt = pfr_lookup_table(&tbl);
if (kt == NULL) {
- kt = pfr_create_ktable(&tbl, gettime(), 1, intr);
+ kt = pfr_create_ktable(&tbl, gettime(), 1, wait);
if (kt == NULL)
return (NULL);
if (ac != NULL) {
bzero(tbl.pfrt_anchor, sizeof(tbl.pfrt_anchor));
rt = pfr_lookup_table(&tbl);
if (rt == NULL) {
- rt = pfr_create_ktable(&tbl, 0, 1, intr);
+ rt = pfr_create_ktable(&tbl, 0, 1, wait);
if (rt == NULL) {
pfr_destroy_ktable(kt, 0);
return (NULL);
Index: sys/net/pfvar.h
===================================================================
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.507
diff -u -p -r1.507 pfvar.h
--- sys/net/pfvar.h 29 Apr 2022 09:55:43 -0000 1.507
+++ sys/net/pfvar.h 26 Jun 2022 09:21:07 -0000
@@ -1709,7 +1709,7 @@ extern struct ifnet *sync_ifp;
extern struct pf_rule pf_default_rule;
extern int pf_tbladdr_setup(struct pf_ruleset *,
- struct pf_addr_wrap *);
+ struct pf_addr_wrap *, int);
extern void pf_tbladdr_remove(struct pf_addr_wrap *);
extern void pf_tbladdr_copyout(struct pf_addr_wrap *);
extern void pf_calc_skip_steps(struct pf_rulequeue *);
@@ -1875,7 +1875,7 @@ void pfi_group_addmember(const char *)
void pfi_group_delmember(const char *);
int pfi_match_addr(struct pfi_dynaddr *, struct pf_addr *,
sa_family_t);
-int pfi_dynaddr_setup(struct pf_addr_wrap *, sa_family_t);
+int pfi_dynaddr_setup(struct pf_addr_wrap *, sa_family_t, int);
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 *);
