Date: Wed, 17 Apr 2019 23:05:26 +0100 From: "Kristof Provost" <kp@FreeBSD.org> To: "Gleb Smirnoff" <glebius@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r346319 - head/sys/netpfil/pf Message-ID: <8B606261-B614-474F-AA73-217FE09E28BA@FreeBSD.org> In-Reply-To: <20190417211718.GX1243@FreeBSD.org> References: <201904171642.x3HGgslA075253@repo.freebsd.org> <20190417211718.GX1243@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 17 Apr 2019, at 22:17, Gleb Smirnoff wrote:
> Kristof,
>
> On Wed, Apr 17, 2019 at 04:42:54PM +0000, Kristof Provost wrote:
> K> Modified: head/sys/netpfil/pf/pf_ioctl.c
> K>
> ==============================================================================
> K> --- head/sys/netpfil/pf/pf_ioctl.c Wed Apr 17 16:31:30
> 2019 (r346318)
> K> +++ head/sys/netpfil/pf/pf_ioctl.c Wed Apr 17 16:42:54
> 2019 (r346319)
> K> @@ -3103,24 +3103,24 @@ DIOCCHANGEADDR_error:
> K> break;
> K> }
> K>
> K> - PF_RULES_WLOCK();
> K> + PF_RULES_RLOCK();
> K> n = pfr_table_count(&io->pfrio_table, io->pfrio_flags);
> K> io->pfrio_size = min(io->pfrio_size, n);
> K> + PF_RULES_RUNLOCK();
> K>
> K> totlen = io->pfrio_size * sizeof(struct pfr_table);
> K> pfrts = mallocarray(io->pfrio_size, sizeof(struct pfr_table),
> K> M_TEMP, M_NOWAIT);
> K> if (pfrts == NULL) {
> K> error = ENOMEM;
> K> - PF_RULES_WUNLOCK();
> K> break;
> K> }
> K> error = copyin(io->pfrio_buffer, pfrts, totlen);
> K> if (error) {
> K> free(pfrts, M_TEMP);
> K> - PF_RULES_WUNLOCK();
> K> break;
> K> }
> K> + PF_RULES_WLOCK();
> K> error = pfr_set_tflags(pfrts, io->pfrio_size,
> K> io->pfrio_setflag, io->pfrio_clrflag, &io->pfrio_nchange,
> K> &io->pfrio_ndel, io->pfrio_flags | PFR_FLAG_USERIOCTL);
>
> Couple comments:
>
> 1) Now we can malloc with M_WAITOK.
>
That’s a good point. I’ll see about changing that tomorrow.
> 2) Are we sure that table count won't change while we dropped the
> lock?
>
No, the table count can indeed change while we’re unlocked. It
doesn’t really matter though. The initial count only serves to limit
the memory allocation to something sane. pfr_set_tflags() still does
appropriate checks.
It’s always been possible for the table count to change between user
space preparing its request and it being handled in the kernel, so that
was always a possibility.
Regards,
Kristof
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?8B606261-B614-474F-AA73-217FE09E28BA>