From owner-svn-src-all@freebsd.org Thu Feb 25 10:26:22 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 9479BAB365B; Thu, 25 Feb 2016 10:26:22 +0000 (UTC) (envelope-from kp@vega.codepro.be) Received: from venus.codepro.be (venus.codepro.be [IPv6:2a01:4f8:162:1127::2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.codepro.be", Issuer "Gandi Standard SSL CA 2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 5BD281F40; Thu, 25 Feb 2016 10:26:22 +0000 (UTC) (envelope-from kp@vega.codepro.be) Received: from vega.codepro.be (unknown [172.16.1.3]) by venus.codepro.be (Postfix) with ESMTP id 1B6BE1996E; Thu, 25 Feb 2016 11:26:18 +0100 (CET) Received: by vega.codepro.be (Postfix, from userid 1001) id 142B37F591; Thu, 25 Feb 2016 11:26:18 +0100 (CET) Date: Thu, 25 Feb 2016 11:26:18 +0100 From: Kristof Provost To: Konstantin Belousov Cc: Conrad Meyer , "src-committers@freebsd.org" , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" Subject: Re: svn commit: r296025 - head/sys/netpfil/pf Message-ID: <20160225102617.GG3003@vega.codepro.be> References: <201602250733.u1P7Xxoh041746@repo.freebsd.org> <20160225091741.GF3003@vega.codepro.be> <20160225100757.GA67250@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160225100757.GA67250@kib.kiev.ua> X-Checked-By-NSA: Probably User-Agent: Mutt/1.5.24 (2015-08-30) X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.20 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: Thu, 25 Feb 2016 10:26:22 -0000 On 2016-02-25 12:07:57 (+0200), Konstantin Belousov wrote: > On Thu, Feb 25, 2016 at 10:17:41AM +0100, Kristof Provost wrote: > > On 2016-02-24 23:47:55 (-0800), Conrad Meyer wrote: > > > On Wed, Feb 24, 2016 at 11:41 PM, Adrian Chadd wrote: > > > > .. what's capping totlen so one doesn't run out of memory? > > > > > > There was a DoS vector before (user controlled io->pfrio_size) and > > > basically the same DoS vector now (either of io->pfrio_size or > > > io->pfrio_size2). This change isn't a regression. Still, it should > > > be fixed. > > > > > It's an M_WAITOK allocation, so if the user asks for more memory than is > > available the thread will sleep. I'd assumed that if the user terminates > > the thread the sleep will wake, the allocation will fail and the ioctl() > > will return an error. > M_WAITOK allocations still panic when requested amount of KVA is > unreasonable. > > I am curious what do you mean by 'user terminating the thread'. The > sleep in malloc() is uninterruptible by signal, and user does not have > any other way to disturb the execution. > Ah, so my assumptions about malloc() are incorrect. That's good to know. > > Perhaps we should do what OpenBSD do, and not allocate the temporary > > buffer at all. They copy in/out the individual entries one by one. On > > the other hand, one could still exhaust memory by inserting large > > numbers of addresses in the table. > But note that accesses to the user memory may fault, which puts whole > VM and VFS subsystems (and possibly the network as well, if user > supplied address is backed by mapped NFS file) after the locks owned > at the moment of copyin() call. > That's a good point. We could handle those accesses failing, but sleeping with the PF_RULES_WLOCK held would be a bad idea. I suppose we could just change M_WAITOK into M_NOWAIT and return ENOMEM if it fails. The only downside I can see is that it's more likely for a call to fail if there's not a lot of free memory. There's actually a decent number of cases where that'd have to be done, but it doesn't look particularly hard to do. Regards, Kristof