On Sun, Jun 26, 2022 at 12:02:41PM +0200, Moritz Buhl wrote:
> 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?
OK bluhm@
> 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 *);
