Re: pfioctl: drop net lock from DIOCGETIFACES, DIOC{SET,CLR}IFFLAG
On Fri, May 26, 2023 at 04:18:45PM +, Klemens Nanni wrote: > On Fri, May 26, 2023 at 05:28:01PM +0300, Vitaliy Makkoveev wrote: > > On Fri, May 26, 2023 at 01:03:13PM +, Klemens Nanni wrote: > > > snmpd(8) and 'pfctl -s Interfaces' dump pf's internal list of interfaces. > > > > > > pf.conf's 'set skip on ifN' and 'pfctl -F all|Reset' set and clear flags, > > > PFI_IFLAG_SKIP being the only flag. > > > > > > (There's no other usage of these ioctls in base.) > > > > > > pf's internal interface list is completely protected by the pf lock, > > > pf lock assertions since pf_if.c r1.110 from over a week ago support this. > > > > > > OK? > > > > > > > pfi_skip_if() called by pfi_get_ifaces() performs `ifgl_next' list > > walkthrough. This list is netlock protected, so the netlock around > > pfi_get_ifaces() should be kept, but relaxed to shared netlock. > > Indeed, thanks. Ping. Feedback? Objection? OK? Index: pf_ioctl.c === RCS file: /cvs/src/sys/net/pf_ioctl.c,v retrieving revision 1.405 diff -u -p -r1.405 pf_ioctl.c --- pf_ioctl.c 26 May 2023 12:13:26 - 1.405 +++ pf_ioctl.c 30 May 2023 23:58:25 - @@ -2942,11 +2942,11 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a goto fail; } - NET_LOCK(); + NET_LOCK_SHARED(); PF_LOCK(); pfi_get_ifaces(io->pfiio_name, kif_buf, >pfiio_size); PF_UNLOCK(); - NET_UNLOCK(); + NET_UNLOCK_SHARED(); if (copyout(kif_buf, io->pfiio_buffer, sizeof(*kif_buf) * io->pfiio_size)) error = EFAULT; @@ -2962,11 +2962,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a goto fail; } - NET_LOCK(); PF_LOCK(); error = pfi_set_flags(io->pfiio_name, io->pfiio_flags); PF_UNLOCK(); - NET_UNLOCK(); break; } @@ -2978,11 +2976,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a goto fail; } - NET_LOCK(); PF_LOCK(); error = pfi_clear_flags(io->pfiio_name, io->pfiio_flags); PF_UNLOCK(); - NET_UNLOCK(); break; }
Re: pfioctl: drop net lock from DIOCGETIFACES, DIOC{SET,CLR}IFFLAG
On Fri, May 26, 2023 at 05:28:01PM +0300, Vitaliy Makkoveev wrote: > On Fri, May 26, 2023 at 01:03:13PM +, Klemens Nanni wrote: > > snmpd(8) and 'pfctl -s Interfaces' dump pf's internal list of interfaces. > > > > pf.conf's 'set skip on ifN' and 'pfctl -F all|Reset' set and clear flags, > > PFI_IFLAG_SKIP being the only flag. > > > > (There's no other usage of these ioctls in base.) > > > > pf's internal interface list is completely protected by the pf lock, > > pf lock assertions since pf_if.c r1.110 from over a week ago support this. > > > > OK? > > > > pfi_skip_if() called by pfi_get_ifaces() performs `ifgl_next' list > walkthrough. This list is netlock protected, so the netlock around > pfi_get_ifaces() should be kept, but relaxed to shared netlock. Indeed, thanks. Index: pf_ioctl.c === RCS file: /cvs/src/sys/net/pf_ioctl.c,v retrieving revision 1.405 diff -u -p -r1.405 pf_ioctl.c --- pf_ioctl.c 26 May 2023 12:13:26 - 1.405 +++ pf_ioctl.c 26 May 2023 16:10:26 - @@ -2942,11 +2942,11 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a goto fail; } - NET_LOCK(); + NET_LOCK_SHARED(); PF_LOCK(); pfi_get_ifaces(io->pfiio_name, kif_buf, >pfiio_size); PF_UNLOCK(); - NET_UNLOCK(); + NET_UNLOCK_SHARED(); if (copyout(kif_buf, io->pfiio_buffer, sizeof(*kif_buf) * io->pfiio_size)) error = EFAULT; @@ -2962,11 +2962,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a goto fail; } - NET_LOCK(); PF_LOCK(); error = pfi_set_flags(io->pfiio_name, io->pfiio_flags); PF_UNLOCK(); - NET_UNLOCK(); break; } @@ -2978,11 +2976,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a goto fail; } - NET_LOCK(); PF_LOCK(); error = pfi_clear_flags(io->pfiio_name, io->pfiio_flags); PF_UNLOCK(); - NET_UNLOCK(); break; }
Re: pfioctl: drop net lock from DIOCGETIFACES, DIOC{SET,CLR}IFFLAG
On Fri, May 26, 2023 at 01:03:13PM +, Klemens Nanni wrote: > snmpd(8) and 'pfctl -s Interfaces' dump pf's internal list of interfaces. > > pf.conf's 'set skip on ifN' and 'pfctl -F all|Reset' set and clear flags, > PFI_IFLAG_SKIP being the only flag. > > (There's no other usage of these ioctls in base.) > > pf's internal interface list is completely protected by the pf lock, > pf lock assertions since pf_if.c r1.110 from over a week ago support this. > > OK? > pfi_skip_if() called by pfi_get_ifaces() performs `ifgl_next' list walkthrough. This list is netlock protected, so the netlock around pfi_get_ifaces() should be kept, but relaxed to shared netlock. > Index: pf_ioctl.c > === > RCS file: /cvs/src/sys/net/pf_ioctl.c,v > retrieving revision 1.405 > diff -u -p -r1.405 pf_ioctl.c > --- pf_ioctl.c26 May 2023 12:13:26 - 1.405 > +++ pf_ioctl.c26 May 2023 12:46:37 - > @@ -2942,11 +2942,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a > goto fail; > } > > - NET_LOCK(); > PF_LOCK(); > pfi_get_ifaces(io->pfiio_name, kif_buf, >pfiio_size); > PF_UNLOCK(); > - NET_UNLOCK(); > if (copyout(kif_buf, io->pfiio_buffer, sizeof(*kif_buf) * > io->pfiio_size)) > error = EFAULT; > @@ -2962,11 +2960,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a > goto fail; > } > > - NET_LOCK(); > PF_LOCK(); > error = pfi_set_flags(io->pfiio_name, io->pfiio_flags); > PF_UNLOCK(); > - NET_UNLOCK(); > break; > } > > @@ -2978,11 +2974,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a > goto fail; > } > > - NET_LOCK(); > PF_LOCK(); > error = pfi_clear_flags(io->pfiio_name, io->pfiio_flags); > PF_UNLOCK(); > - NET_UNLOCK(); > break; > } > >
pfioctl: drop net lock from DIOCGETIFACES, DIOC{SET,CLR}IFFLAG
snmpd(8) and 'pfctl -s Interfaces' dump pf's internal list of interfaces. pf.conf's 'set skip on ifN' and 'pfctl -F all|Reset' set and clear flags, PFI_IFLAG_SKIP being the only flag. (There's no other usage of these ioctls in base.) pf's internal interface list is completely protected by the pf lock, pf lock assertions since pf_if.c r1.110 from over a week ago support this. OK? Index: pf_ioctl.c === RCS file: /cvs/src/sys/net/pf_ioctl.c,v retrieving revision 1.405 diff -u -p -r1.405 pf_ioctl.c --- pf_ioctl.c 26 May 2023 12:13:26 - 1.405 +++ pf_ioctl.c 26 May 2023 12:46:37 - @@ -2942,11 +2942,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a goto fail; } - NET_LOCK(); PF_LOCK(); pfi_get_ifaces(io->pfiio_name, kif_buf, >pfiio_size); PF_UNLOCK(); - NET_UNLOCK(); if (copyout(kif_buf, io->pfiio_buffer, sizeof(*kif_buf) * io->pfiio_size)) error = EFAULT; @@ -2962,11 +2960,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a goto fail; } - NET_LOCK(); PF_LOCK(); error = pfi_set_flags(io->pfiio_name, io->pfiio_flags); PF_UNLOCK(); - NET_UNLOCK(); break; } @@ -2978,11 +2974,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a goto fail; } - NET_LOCK(); PF_LOCK(); error = pfi_clear_flags(io->pfiio_name, io->pfiio_flags); PF_UNLOCK(); - NET_UNLOCK(); break; }
