From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 41385385742A for ; Mon, 10 May 2021 17:45:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 41385385742A Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-596-naoQbi4cMFKZtCX2jtBVZA-1; Mon, 10 May 2021 13:45:47 -0400 X-MC-Unique: naoQbi4cMFKZtCX2jtBVZA-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 78A65801107; Mon, 10 May 2021 17:45:46 +0000 (UTC) Received: from oldenburg.str.redhat.com (ovpn-112-137.ams2.redhat.com [10.36.112.137]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 9EE3719C44; Mon, 10 May 2021 17:45:45 +0000 (UTC) From: Florian Weimer To: Adhemerval Zanella Cc: libc-alpha@sourceware.org Subject: Re: [PATCH v2 4/4] libio: Fix race access to _IO_FLAGS2_NEED_LOCK References: <20210510170426.2533768-1-adhemerval.zanella@linaro.org> <20210510170426.2533768-4-adhemerval.zanella@linaro.org> Date: Mon, 10 May 2021 19:46:11 +0200 In-Reply-To: <20210510170426.2533768-4-adhemerval.zanella@linaro.org> (Adhemerval Zanella's message of "Mon, 10 May 2021 14:04:26 -0300") Message-ID: <875yzq7a8s.fsf@oldenburg.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 May 2021 17:45:50 -0000 * Adhemerval Zanella: > The flag is set on: > > 1. the stream creation (for memstream and wmemstream). > 2. by pthread_create when it detects the process become multi-thread. > 3. by flockfile. > > The 1. and 2. cases do not generate a race condition since for 1. the > stream reference is not accessible by other threads and for 2. the > process is still single-threaded. > > However, for 3. the functions feof, ferror, fputc, getc, getchar, > ungetc, and putc access it without locking the stream to check whether > to use the optimized _unlocked variants. So to avoid a racy condition, > both the check and set are done using atomic operations. Sorry, I don't think this works, purely for performance reasons. Here's why: > diff --git a/libio/feof.c b/libio/feof.c > index 8275321788..823b40637a 100644 > --- a/libio/feof.c > +++ b/libio/feof.c > @@ -32,7 +32,7 @@ _IO_feof (FILE *fp) > { > int result; > CHECK_FILE (fp, EOF); > - if (!_IO_need_lock (fp)) > + if (!io_need_lock (fp)) > return _IO_feof_unlocked (fp); > _IO_flockfile (fp); > result =3D _IO_feof_unlocked (fp); io_need_lock is used on the fast path here. > diff --git a/libio/libio.h b/libio/libio.h > index 511b39457f..6b569511ec 100644 > --- a/libio/libio.h > +++ b/libio/libio.h > +/* The _IO_FLAGS2_NEED_LOCK flag is set on: > + > + 1. the stream creation (for memstream and wmemstream). > + 2. by pthread_create when it detects the process become multi-thread. > + 3. by flockfile. > + > + The 1. and 2. cases do not generate a race condition since for 1. > + the stream reference is not accessible by other threads and for 2. > + the process is still single-threaded. > + > + However, for 3. the functions feof, ferror, fputc, getc, getchar, ung= etc, > + and putc access it without locking the stream to check whether to use= the > + optimized _unlocked variants. So to avoid a racy condition, both the > + check and set are done using atomic operations. */ > + > +static inline bool > +io_need_lock (FILE *fp) > +{ > + return atomic_load_acquire (&fp->_flags2) & _IO_FLAGS2_NEED_LOCK; > +} And this means we have an acquire load on the fast path. And I think that's =E2=80=A6 not good on the architecture for which the optimization wa= s written. > diff --git a/stdio-common/flockfile.c b/stdio-common/flockfile.c > index a66e0a731e..3b42fe891d 100644 > --- a/stdio-common/flockfile.c > +++ b/stdio-common/flockfile.c > @@ -22,7 +22,7 @@ > void > __flockfile (FILE *stream) > { > - stream->_flags2 |=3D _IO_FLAGS2_NEED_LOCK; > + io_set_need_lock (stream); > _IO_lock_lock (*stream->_lock); > } > weak_alias (__flockfile, flockfile); My impression is that _IO_FLAGS2_NEED_LOCK is set here for consistency, not for concurrency control (i.e., so that other internal locking operations produce the desired result). As you pointed out, pthread_create automatically sets _IO_FLAGS2_NEED_LOCK in those cases where it is not set by default. So the _flags2 update only matters in the single-threaded case. But the lock does not matter, so we can do this instead: void __flockfile (FILE *stream) { _IO_lock_lock (*stream->_lock); stream->_flags2 |=3D _IO_FLAGS2_NEED_LOCK; } Since the _flags2 update happens after the lock operation, the data race is gone. Ideally this should have a comment, but I'm not sure what to write there. With more careful review, maybe we could eliminate the _flags2 update altogether. What we cannot do is make the flockfile operation itself conditional on _IO_FLAGS2_NEED_LOCK because of observable impact vis-a-vis funlockfile. If we want to optimize flockfile, I think we'd have to change _IO_lock_lock and add an atomics-free fast path for SINGLE_THREAD_P (that still performs the lock update). With that, the entire _IO_FLAGS2_NEED_LOCK business could be removed again, I think. Thanks, Florian