pipe() resource exhaustion

[Konstantin Belousov]

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.
-------------- 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/20140408/bb74591a/attachment.sig>