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;
Date:   Thu, 21 Nov 2019 11:09:04 -0800
From:   Kees Cook <keescook@chromium.org>
To:     Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc:     Jonathan Corbet <corbet@lwn.net>, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] docs, parallelism: Do not leak blocking mode to
 writer
Message-ID: <201911211105.E11EEBAC4@keescook>
References: <20191121000304.48829-1-keescook@chromium.org>
 <20191121000304.48829-3-keescook@chromium.org>
 <041953ef-0b6c-4ea8-8734-aa1e6703f9f8@rasmusvillemoes.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <041953ef-0b6c-4ea8-8734-aa1e6703f9f8@rasmusvillemoes.dk>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Nov 21, 2019 at 08:41:01AM +0100, Rasmus Villemoes wrote:
> 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.

Ah! This explains my confusion over what you were trying to tell me
before. I thought you meant the other end of the pipe, which seemed
crazy. You mean the other jobserver readers (i.e. "make" itself) who
have the same shared _reader_ fd. This is exactly what you said, but I
was too dense. :)

I'll fix this up!

> 
> > 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.

Hah. Yes indeed.

-- 
Kees Cook