Re: panic: rw_enter: pfioctl_rw locking against myself

[Alexandr Nedvedicky]

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.

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.
> 
> --------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();