On Wed, Jun 28, 2023 at 06:17:46PM +0200, Alexandr Nedvedicky wrote:
> Hello,
>
> the fix below solves the locking issue. however pf_close_all_trans() still
> breaks the test case. it fails to retrieve all rules. it looks like pfctl(8)
> currently opens transaction for every ruleset/anchor it's going to retrieve.
>
> the ruleset in question reads as follows:
>
> netlock# cat /usr/src/regress/sbin/pfctl/pf91.in
> # basic anchor test
> anchor on tun1000000 {
> anchor foo out {
> pass proto tcp to port 1234
> anchor proto tcp to port 2413 user root label "foo" {
> block
> pass from 127.0.0.1
> }
> }
> pass in proto tcp to port 1234
> }
>
> as soon as we loaded we get this output on system which runs diff below:
>
> netlock# /sbin/pfctl -o none -a 'regress/*' -sr
> anchor on tun1000000 all {
> anchor "foo" out all {
> pass proto tcp from any to any port = 1234 flags S/SA
> anchor proto tcp from any to any port = 2413 user = 0 label "foo" {
> block drop all
> pass inet from 127.0.0.1 to any flags S/SA
> }
> pfctl: DIOCGETRULE: Device not configured
> }
> pfctl: DIOCGETRULE: Device not configured
> }
> pfctl: DIOCGETRULE: Device not configured
>
> sigh... things are not that simple. I still want to commit diff
> below because it fixes bug we have in tree.
>
> then I'll have to think on how to make claudio's diff smarter.
Sounds like a plan.
>
> thanks and
> regards
> sashan
>
>
> On Wed, Jun 28, 2023 at 05:46:36PM +0200, Alexandr Nedvedicky wrote:
> > Hello,
> >
> > it looks like we need to use goto fail instead of return.
> > this is the diff I'm testing now.
That early return is clearly a bug holding pfioctl_rw back.
OK kn
> >
> > --------8<---------------8<---------------8<------------------8<--------
> > diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
> > index 36779cfdfd3..a51df9e6089 100644
> > --- a/sys/net/pf_ioctl.c
> > +++ b/sys/net/pf_ioctl.c
> > @@ -1508,11 +1508,15 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int
> > flags, struct proc *p)
> > int i;
> >
> > t = pf_find_trans(minor(dev), pr->ticket);
> > - if (t == NULL)
> > - return (ENXIO);
> > + if (t == NULL) {
> > + error = ENXIO;
> > + goto fail;
> > + }
> > KASSERT(t->pft_unit == minor(dev));
> > - if (t->pft_type != PF_TRANS_GETRULE)
> > - return (EINVAL);
> > + if (t->pft_type != PF_TRANS_GETRULE) {
> > + error = EINVAL;
> > + goto fail;
> > + }
> >
> > NET_LOCK();
>
