pipe() resource exhaustion

[Konstantin Belousov]

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.

> 
> 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.
> +		 */
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 = 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_ */
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20140409/acbf938a/attachment.sig>