From owner-svn-src-all@freebsd.org Wed Apr 17 22:05:33 2019 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 43625157AE9F; Wed, 17 Apr 2019 22:05:33 +0000 (UTC) (envelope-from kp@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id D22D295BE8; Wed, 17 Apr 2019 22:05:32 +0000 (UTC) (envelope-from kp@FreeBSD.org) Received: from venus.codepro.be (venus.codepro.be [5.9.86.228]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "smtp.codepro.be", Issuer "Let's Encrypt Authority X3" (verified OK)) (Authenticated sender: kp) by smtp.freebsd.org (Postfix) with ESMTPSA id 6C28F10F0; Wed, 17 Apr 2019 22:05:32 +0000 (UTC) (envelope-from kp@FreeBSD.org) Received: from [192.168.228.1] (host1.test.ravensoft.telecomplete.net [213.160.118.146]) (Authenticated sender: kp) by venus.codepro.be (Postfix) with ESMTPSA id 4E0E51A8BE; Thu, 18 Apr 2019 00:05:29 +0200 (CEST) From: "Kristof Provost" To: "Gleb Smirnoff" Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r346319 - head/sys/netpfil/pf Date: Wed, 17 Apr 2019 23:05:26 +0100 X-Mailer: MailMate (2.0BETAr6135) 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> MIME-Version: 1.0 X-Rspamd-Queue-Id: D22D295BE8 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.98 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.98)[-0.977,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[] Content-Type: text/plain; charset=utf-8; format=flowed; markup=markdown Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Apr 2019 22:05:33 -0000 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