Date: Wed, 9 Apr 2014 13:16:54 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: freebsd-hackers@freebsd.org, Eduardo Morras <emorrasg@yahoo.es> Subject: Re: pipe() resource exhaustion Message-ID: <20140409111654.GA17650@dft-labs.eu> In-Reply-To: <20140408132442.GZ21331@kib.kiev.ua> References: <lhu0jv$r6n$1@ger.gmane.org> <ab57e60fcc1c1438fcca500e3c594d35@mail.feld.me> <20140408130206.e75f3bf6c6df28b6e4839e70@yahoo.es> <20140408121222.GB30326@dft-labs.eu> <20140408123827.GW21331@kib.kiev.ua> <20140408130727.GA11363@dft-labs.eu> <20140408132442.GZ21331@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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_ */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140409111654.GA17650>