I don't know this code well enough to give a meaningful OK, but this should probably get committed.
On 2022/06/01 09:16, Alexandr Nedvedicky wrote: > Hello, > > </snip> > > r420-1# rcctl -f start relayd > > relayd(ok) > > r420-1# uvm_fault(0xfffffd862f82f990, 0x0, 0, 1) -> e > > kernel: page fault trap, code=0 > > Stopped at pf_find_or_create_ruleset+0x1c: movb 0(%rdi),%al > > TID PID UID PRFLAGS PFLAGS CPU COMMAND > > 431388 19003 0 0x2 0 5 relayd > > 174608 32253 89 0x1000012 0 2 relayd > > 395415 12468 0 0x2 0 4 relayd > > 493579 11904 0 0x2 0 3 relayd > > *101082 14967 89 0x1100012 0 0K relayd > > pf_find_or_create_ruleset(0) at pf_find_or_create_ruleset+0x1c > > pfr_add_tables(832d7cca800,1,ffff800000eaf43c,10000000) at > > pfr_add_tables+0x6ae > > > > pfioctl(4900,c450443d,ffff800000eaf000,3,ffff80002272e7f0) at pfioctl+0x1d9f > > VOP_IOCTL(fffffd8551f82dd0,c450443d,ffff800000eaf000,3,fffffd862f7d60c0,ffff800 > > 02272e7f0) at VOP_IOCTL+0x5c > > vn_ioctl(fffffd855ecec1e8,c450443d,ffff800000eaf000,ffff80002272e7f0) at > > vn_ioctl+0x75 > > sys_ioctl(ffff80002272e7f0,ffff8000227d9980,ffff8000227d99d0) at > > sys_ioctl+0x2c4 > > syscall(ffff8000227d9a40) at syscall+0x374 > > Xsyscall() at Xsyscall+0x128 > > end of kernel > > it looks like we are dying here at line 239 due to NULL pointer deference: > > 232 struct pf_ruleset * > 233 pf_find_or_create_ruleset(const char *path) > 234 { > 235 char *p, *aname, *r; > 236 struct pf_ruleset *ruleset; > 237 struct pf_anchor *anchor; > 238 > 239 if (path[0] == 0) > 240 return (&pf_main_ruleset); > 241 > 242 while (*path == '/') > 243 path++; > 244 > > I've followed the same steps to reproduce the issue to check if > diff below resolves the issue. The bug has been introduced by > my recent change to pf_table.c [1] from May 10th: > > Modified files: > sys/net : pf_ioctl.c pf_table.c > > Log message: > move memory allocations in pfr_add_tables() out of > NET_LOCK()/PF_LOCK() scope. bluhm@ helped a lot > to put this diff into shape. > > besides using a regression test I've also did simple testing > using a 'load anchor': > > netlock# cat /tmp/anchor.conf > > load anchor "test" from "/tmp/pf.conf" > netlock# > netlock# cat /tmp/pf.conf > > table <try> { 192.168.1.1 } > pass from <try> > netlock# > netlock# pfctl -sA > test > netlock# pfctl -a test -sT > try > netlock# pfctl -a test -t try -T show > 192.168.1.1 > > OK to commit fix below? > > 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 8315ea5dd3a..dfc49de5efe 100644 > --- a/sys/net/pf_table.c > +++ b/sys/net/pf_table.c > @@ -1628,8 +1628,7 @@ pfr_add_tables(struct pfr_table *tbl, int size, int > *nadd, int flags) > if (r != NULL) > continue; > > - q->pfrkt_rs = pf_find_or_create_ruleset( > - q->pfrkt_root->pfrkt_anchor); > + q->pfrkt_rs = > pf_find_or_create_ruleset(q->pfrkt_anchor); > /* > * root tables are attached to main ruleset, > * because ->pfrkt_anchor[0] == '\0' >