Date: Wed, 9 Apr 2014 14:26:27 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Mateusz Guzik <mjguzik@gmail.com> Cc: freebsd-hackers@freebsd.org, Eduardo Morras <emorrasg@yahoo.es> Subject: Re: pipe() resource exhaustion Message-ID: <20140409112627.GI21331@kib.kiev.ua> In-Reply-To: <20140409111654.GA17650@dft-labs.eu> 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> <20140409111654.GA17650@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
--R7RSAOS08SfIpCMC
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Wed, Apr 09, 2014 at 01:16:54PM +0200, Mateusz Guzik wrote:
> That said how about the following:
Looks fine to me, with one note about the comment. See below.
>=20
> 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 =3D=3D NULL || (ap->a_mode & FEXEC) !=3D 0)
> return (EINVAL);
> if ((fip =3D vp->v_fifoinfo) =3D=3D NULL) {
> - error =3D pipe_named_ctor(&fpipe, td);
> - if (error !=3D 0)
> - return (error);
> + pipe_named_ctor(&fpipe, td);
> fip =3D malloc(sizeof(*fip), M_VNODE, M_WAITOK);
> fip->fi_pipe =3D fpipe;
> fpipe->pipe_wgen =3D fip->fi_readers =3D fip->fi_writers =3D 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, CT=
LFLAG_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);
> }
> =20
> -static int
> +static void
> pipe_paircreate(struct thread *td, struct pipepair **p_pp)
> {
> struct pipepair *pp;
> struct pipe *rpipe, *wpipe;
> - int error;
> =20
> *p_pp =3D pp =3D 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));
> =20
> /* Only the forward direction pipe is backed by default */
> - if ((error =3D pipe_create(rpipe, 1)) !=3D 0 ||
> - (error =3D pipe_create(wpipe, 0)) !=3D 0) {
> - pipeclose(rpipe);
> - pipeclose(wpipe);
> - return (error);
> - }
> + pipe_create(rpipe, 1);
> + pipe_create(wpipe, 0);
> =20
> rpipe->pipe_state |=3D PIPE_DIRECTOK;
> wpipe->pipe_state |=3D PIPE_DIRECTOK;
> - return (0);
> }
> =20
> -int
> +void
> pipe_named_ctor(struct pipe **ppipe, struct thread *td)
> {
> struct pipepair *pp;
> - int error;
> =20
> - error =3D pipe_paircreate(td, &pp);
> - if (error !=3D 0)
> - return (error);
> + pipe_paircreate(td, &pp);
> pp->pp_rpipe.pipe_state |=3D PIPE_NAMED;
> *ppipe =3D &pp->pp_rpipe;
> - return (0);
> }
> =20
> void
> @@ -419,9 +409,7 @@ kern_pipe2(struct thread *td, int fildes[2], int flag=
s)
> int fd, fflags, error;
> =20
> fdp =3D td->td_proc->p_fd;
> - error =3D pipe_paircreate(td, &pp);
> - if (error !=3D 0)
> - return (error);
> + pipe_paircreate(td, &pp);
> rpipe =3D &pp->pp_rpipe;
> wpipe =3D &pp->pp_wpipe;
> error =3D 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;
> =20
> if (backing) {
> + /*
> + * Note that these functions can fail, but we ignore
> + * the error as it is not fatal and could be provoked
> + * by users.
> + */
It would be benefitial to add some more details on the way to provoke the
failure. Note in the comment that creating too much pipes would exhaust
pipe map and we fall back to the buffer pipe creation there, which still
work correctly, albeit slow.
> if (amountpipekva > maxpipekva / 2)
> - error =3D pipespace_new(pipe, SMALL_PIPE_SIZE);
> + (void)pipespace_new(pipe, SMALL_PIPE_SIZE);
> else
> - error =3D pipespace_new(pipe, PIPE_SIZE);
> - } else {
> - /* If we're not backing this pipe, no need to do anything. */
> - error =3D 0;
> + (void)pipespace_new(pipe, PIPE_SIZE);
> }
> +
> pipe->pipe_ino =3D -1;
> - return (error);
> }
> =20
> /* 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))
> =20
> 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_ */
--R7RSAOS08SfIpCMC
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (FreeBSD)
iQIcBAEBAgAGBQJTRS5iAAoJEJDCuSvBvK1BQAYP/3o3nngqzNSaQGCvEcYc5DDr
TXXy2d7dRWDYaBZM1q3SmyMSPxiDY9ZNpitbAwoy2a8VSARclS2mf4m6K+F01pxy
1Lbh7apAe2XBHRxb5jw4qsls0HQe8Y7bD3i528A0U/hJOKbc93IPfqwOrV9ij9FY
d0ey6Z0OWbIPJhWwfC2u3fez7xhJBzPf+V1h0qdfHffxqbNBYySA8b6TL8qCynyD
zWf+W5hk1kwd22vnOkcuxCflwHmz3yU5BgAxjhH++gwyyzA1CV3PVP8ajZSea4Zt
T2gFfu9ghNDGeJYh3+nKi5x45ypMw6HcE13ExsvYzMo0mK3V1PtI2pxtd7l7sKKQ
vRMYyL0oJ003ublkXaB7ViDlR4cyszTux3rdJqcwL+6tmFtrQDE/VUBMW+yxPV++
A7pjIoE70dlY5UqNDViF5BgjZ53/Ve6Ej4C0NQqNzy5XacdG9/gEvf2gK1IwmAl/
x/WUM9gKevSXYDhX7rDZiMaoZX9lNk0M03aAZ+QTc1weVEJkJQgktwo3vEZ7qs5y
6gcdZ0MLq7+w8EMz47E2jadetB/2yFhyN7PU6kmB5uNsP1aF5Q2GxdQznuGXQZzK
g5PzL+IpX3SLcumnsL3OtOQyV/Vw3dEaet34htUkol4zl29CwHUz9zHnBtrhK5+G
i2vjRvfqZ2P/mj91mh6S
=F3VI
-----END PGP SIGNATURE-----
--R7RSAOS08SfIpCMC--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140409112627.GI21331>