From owner-freebsd-hackers@FreeBSD.ORG Wed Apr 9 11:17:00 2014 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id E725548D for ; Wed, 9 Apr 2014 11:17:00 +0000 (UTC) Received: from mail-we0-x22e.google.com (mail-we0-x22e.google.com [IPv6:2a00:1450:400c:c03::22e]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 76001174A for ; Wed, 9 Apr 2014 11:17:00 +0000 (UTC) Received: by mail-we0-f174.google.com with SMTP id t60so2330422wes.33 for ; Wed, 09 Apr 2014 04:16:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=dhgaB9apCVGViUAIWorvbBxAQ5pHOgpVZTF+1mATTwo=; b=Aezza8pXaHGSuLAFWAGqAw/saIEhyZqh9denl+Vp4vXGhKhK/y5Aqu6BVgbR1fB0rL eZQtlkf9yFRBY0NNmFNVeGWrFJ5G8NsYl4kTPF7ZhhMjnjAD8056HLzSxt3tVp7Uk+oJ nsn+Z0kA7rsp8EUDC4jRtNEfmvmrxonm+6JC+zWS0XPfXhMPeZkFTmV6f0f0x6t9l/Lf 5+IyztCiQT6sqU1CrJ0P0cos7KECcXfcnYGjBe8JnlQLJFe9diwGrW2k8bQ1AP7eJJCU 0McI8LbFC7XU5nbRpCwxBo4swincWMVLKa605s8jTXjU/NUN2EH26QcqCeQJTVEM4Je2 PLLQ== X-Received: by 10.180.76.142 with SMTP id k14mr9389952wiw.21.1397042218574; Wed, 09 Apr 2014 04:16:58 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id g13sm1190609wjn.15.2014.04.09.04.16.56 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Wed, 09 Apr 2014 04:16:57 -0700 (PDT) Date: Wed, 9 Apr 2014 13:16:54 +0200 From: Mateusz Guzik To: Konstantin Belousov Subject: Re: pipe() resource exhaustion Message-ID: <20140409111654.GA17650@dft-labs.eu> Mail-Followup-To: Mateusz Guzik , Konstantin Belousov , Eduardo Morras , freebsd-hackers@freebsd.org References: <20140408130206.e75f3bf6c6df28b6e4839e70@yahoo.es> <20140408121222.GB30326@dft-labs.eu> <20140408123827.GW21331@kib.kiev.ua> <20140408130727.GA11363@dft-labs.eu> <20140408132442.GZ21331@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20140408132442.GZ21331@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-hackers@freebsd.org, Eduardo Morras X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Apr 2014 11:17:01 -0000 On Tue, Apr 08, 2014 at 04:24:42PM +0300, Konstantin Belousov wrote: > On Tue, Apr 08, 2014 at 03:07:27PM +0200, Mateusz Guzik wrote: > > On Tue, Apr 08, 2014 at 03:38:27PM +0300, Konstantin Belousov wrote: > > > On Tue, Apr 08, 2014 at 02:12:22PM +0200, Mateusz Guzik wrote: > > > > That said, supporting a reserve for this one sounds like a good idea and > > > > not that hard to implement - one can either play with atomics and double > > > > check or just place a mutex-protected check in pipespace_new (before > > > > vm_map_find). > > > > > > > ... > > > > > > I think more reasonable behaviour there is to just fall back to the > > > buffered pipe if the direct buffer allocation fails. Look at the > > > pipespace_new() calls in the pipe_create(); probably ignoring the error > > > would do the trick. > > > > Yeah, should have checked the caller. > > > > Interesting though how the error was made fatal in thiscase. > > > > Anyhow, the following hack following your suggestion indeed makes the > > issue go away for me: > > > > diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c > > index 6ba52e3..5930cf2 100644 > > --- a/sys/kern/sys_pipe.c > > +++ b/sys/kern/sys_pipe.c > > @@ -647,19 +647,21 @@ pipe_create(pipe, backing) > > struct pipe *pipe; > > int backing; > > { > > - int error; > > > > if (backing) { > > + /* > > + * Note that these functions can fail, but we ignore > > + * the error as it is not fatal and could be provoked > > + * by users. > > + */ > > if (amountpipekva > maxpipekva / 2) > > - error = pipespace_new(pipe, SMALL_PIPE_SIZE); > > + (void)pipespace_new(pipe, SMALL_PIPE_SIZE); > > else > > - error = pipespace_new(pipe, PIPE_SIZE); > > - } else { > > - /* If we're not backing this pipe, no need to do anything. */ > > - error = 0; > > + (void)pipespace_new(pipe, PIPE_SIZE); > > } > > + > > pipe->pipe_ino = -1; > > - return (error); > > + return (0); > > } > > > > Yes, this looks right. I think it does not make sense to continue > returning an error from the pipe_create() after the patch. The change > would become bigger, but the code for pipe_create() and pipe_paircreate > collapse. It seems that pipe_paircreate() can be changed to return void > as well, but the benefits would be smaller. I figured keeping pipe_create is beneficial in case one wants to play with backing and does no harm. That said how about the following: diff --git a/sys/fs/fifofs/fifo_vnops.c b/sys/fs/fifofs/fifo_vnops.c index d3eb281..a3e8179 100644 --- a/sys/fs/fifofs/fifo_vnops.c +++ b/sys/fs/fifofs/fifo_vnops.c @@ -146,9 +146,7 @@ fifo_open(ap) if (fp == NULL || (ap->a_mode & FEXEC) != 0) return (EINVAL); if ((fip = vp->v_fifoinfo) == NULL) { - error = pipe_named_ctor(&fpipe, td); - if (error != 0) - return (error); + pipe_named_ctor(&fpipe, td); fip = malloc(sizeof(*fip), M_VNODE, M_WAITOK); fip->fi_pipe = fpipe; fpipe->pipe_wgen = fip->fi_readers = fip->fi_writers = 0; diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c index 6ba52e3..b0a1f0d 100644 --- a/sys/kern/sys_pipe.c +++ b/sys/kern/sys_pipe.c @@ -221,8 +221,8 @@ SYSCTL_INT(_kern_ipc, OID_AUTO, piperesizeallowed, CTLFLAG_RW, static void pipeinit(void *dummy __unused); static void pipeclose(struct pipe *cpipe); static void pipe_free_kmem(struct pipe *cpipe); -static int pipe_create(struct pipe *pipe, int backing); -static int pipe_paircreate(struct thread *td, struct pipepair **p_pp); +static void pipe_create(struct pipe *pipe, int backing); +static void pipe_paircreate(struct thread *td, struct pipepair **p_pp); static __inline int pipelock(struct pipe *cpipe, int catch); static __inline void pipeunlock(struct pipe *cpipe); #ifndef PIPE_NODIRECT @@ -331,12 +331,11 @@ pipe_zone_fini(void *mem, int size) mtx_destroy(&pp->pp_mtx); } -static int +static void pipe_paircreate(struct thread *td, struct pipepair **p_pp) { struct pipepair *pp; struct pipe *rpipe, *wpipe; - int error; *p_pp = pp = uma_zalloc(pipe_zone, M_WAITOK); #ifdef MAC @@ -355,30 +354,21 @@ pipe_paircreate(struct thread *td, struct pipepair **p_pp) knlist_init_mtx(&wpipe->pipe_sel.si_note, PIPE_MTX(wpipe)); /* Only the forward direction pipe is backed by default */ - if ((error = pipe_create(rpipe, 1)) != 0 || - (error = pipe_create(wpipe, 0)) != 0) { - pipeclose(rpipe); - pipeclose(wpipe); - return (error); - } + pipe_create(rpipe, 1); + pipe_create(wpipe, 0); rpipe->pipe_state |= PIPE_DIRECTOK; wpipe->pipe_state |= PIPE_DIRECTOK; - return (0); } -int +void pipe_named_ctor(struct pipe **ppipe, struct thread *td) { struct pipepair *pp; - int error; - error = pipe_paircreate(td, &pp); - if (error != 0) - return (error); + pipe_paircreate(td, &pp); pp->pp_rpipe.pipe_state |= PIPE_NAMED; *ppipe = &pp->pp_rpipe; - return (0); } void @@ -419,9 +409,7 @@ kern_pipe2(struct thread *td, int fildes[2], int flags) int fd, fflags, error; fdp = td->td_proc->p_fd; - error = pipe_paircreate(td, &pp); - if (error != 0) - return (error); + pipe_paircreate(td, &pp); rpipe = &pp->pp_rpipe; wpipe = &pp->pp_wpipe; error = falloc(td, &rf, &fd, flags); @@ -642,24 +630,25 @@ pipeselwakeup(cpipe) * Initialize and allocate VM and memory for pipe. The structure * will start out zero'd from the ctor, so we just manage the kmem. */ -static int +static void pipe_create(pipe, backing) struct pipe *pipe; int backing; { - int error; if (backing) { + /* + * Note that these functions can fail, but we ignore + * the error as it is not fatal and could be provoked + * by users. + */ if (amountpipekva > maxpipekva / 2) - error = pipespace_new(pipe, SMALL_PIPE_SIZE); + (void)pipespace_new(pipe, SMALL_PIPE_SIZE); else - error = pipespace_new(pipe, PIPE_SIZE); - } else { - /* If we're not backing this pipe, no need to do anything. */ - error = 0; + (void)pipespace_new(pipe, PIPE_SIZE); } + pipe->pipe_ino = -1; - return (error); } /* ARGSUSED */ diff --git a/sys/sys/pipe.h b/sys/sys/pipe.h index dfe7f2f..ee7c128 100644 --- a/sys/sys/pipe.h +++ b/sys/sys/pipe.h @@ -142,6 +142,6 @@ struct pipepair { #define PIPE_LOCK_ASSERT(pipe, type) mtx_assert(PIPE_MTX(pipe), (type)) void pipe_dtor(struct pipe *dpipe); -int pipe_named_ctor(struct pipe **ppipe, struct thread *td); +void pipe_named_ctor(struct pipe **ppipe, struct thread *td); void pipeselwakeup(struct pipe *cpipe); #endif /* !_SYS_PIPE_H_ */