Date: Thu, 25 Feb 2016 11:26:18 +0100 From: Kristof Provost <kp@FreeBSD.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Conrad Meyer <cem@FreeBSD.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org> Subject: Re: svn commit: r296025 - head/sys/netpfil/pf Message-ID: <20160225102617.GG3003@vega.codepro.be> In-Reply-To: <20160225100757.GA67250@kib.kiev.ua> References: <201602250733.u1P7Xxoh041746@repo.freebsd.org> <CAJ-Vmok_-SzGnUdYi%2BnnDYdGhcKXOUthC1nnPyxHrnWJCKA%2Bcw@mail.gmail.com> <CAG6CVpWwidL6S90fnThc7mXp9sj3PbJB6rvkBN=MmbvFS%2B_vtw@mail.gmail.com> <20160225091741.GF3003@vega.codepro.be> <20160225100757.GA67250@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2016-02-25 12:07:57 (+0200), Konstantin Belousov <kostikbel@gmail.com> wrote: > On Thu, Feb 25, 2016 at 10:17:41AM +0100, Kristof Provost wrote: > > On 2016-02-24 23:47:55 (-0800), Conrad Meyer <cem@FreeBSD.org> wrote: > > > On Wed, Feb 24, 2016 at 11:41 PM, Adrian Chadd <adrian.chadd@gmail.com> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160225102617.GG3003>