Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 2/3] docs, parallelism: Do not leak blocking mode to
 writer
To:     Kees Cook <keescook@chromium.org>, Jonathan Corbet <corbet@lwn.net>
Cc:     linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20191121000304.48829-1-keescook@chromium.org>
 <20191121000304.48829-3-keescook@chromium.org>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
Message-ID: <041953ef-0b6c-4ea8-8734-aa1e6703f9f8@rasmusvillemoes.dk>
Date:   Thu, 21 Nov 2019 08:41:01 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.0
MIME-Version: 1.0
In-Reply-To: <20191121000304.48829-3-keescook@chromium.org>
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 21/11/2019 01.03, Kees Cook wrote:
> Setting non-blocking via a local copy of the jobserver file descriptor
> is safer than just assuming the writer on the original fd is prepared
> for it to be non-blocking.

This is a bit inaccurate. The fd referring to the write side of the pipe
is always blocking - it has to be, due to the protocol requiring you to
write back the tokens you've read, so you can't just drop a token on the
floor. But it's also rather moot, since the pipe will never hold
anywhere near 4096 bytes, let alone a (linux) pipe's default capacity of
64K.

But what we cannot do is change the mode of the open file description to
non-blocking for the read side, in case the parent make (or some sibling
process that has also inherited the same "struct file") expects it to be
blocking.

> Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Link: https://lore.kernel.org/lkml/44c01043-ab24-b4de-6544-e8efd153e27a@rasmusvillemoes.dk
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  scripts/jobserver-count | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/scripts/jobserver-count b/scripts/jobserver-count
> index 6e15b38df3d0..a68a04ad304f 100755
> --- a/scripts/jobserver-count
> +++ b/scripts/jobserver-count
> @@ -12,12 +12,6 @@ default="1"
>  if len(sys.argv) > 1:
>  	default=sys.argv[1]
>  
> -# Set non-blocking for a given file descriptor.
> -def nonblock(fd):
> -	flags = fcntl.fcntl(fd, fcntl.F_GETFL)
> -	fcntl.fcntl(fd, fcntl.F_SETFL, flags | os.O_NONBLOCK)
> -	return fd
> -
>  # Extract and prepare jobserver file descriptors from envirnoment.
>  try:
>  	# Fetch the make environment options.
> @@ -31,8 +25,13 @@ try:
>  	# Parse out R,W file descriptor numbers and set them nonblocking.
>  	fds = opts[0].split("=", 1)[1]
>  	reader, writer = [int(x) for x in fds.split(",", 1)]
> -	reader = nonblock(reader)
> -except (KeyError, IndexError, ValueError, IOError):
> +	# Open a private copy of reader to avoid setting nonblocking
> +	# on an unexpecting writer.

s/writer/reader/

> +	reader = os.open("/proc/self/fd/%d" % (reader), os.O_RDONLY)
> +	flags = fcntl.fcntl(reader, fcntl.F_GETFL)
> +	fcntl.fcntl(reader, fcntl.F_SETFL, flags | os.O_NONBLOCK)

I think you can just specify O_NONBLOCK in the open() call so you avoid
those two fcntls.

Rasmus