From nobody Mon Sep 22 08:50:09 2025 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4cVcJ26R5xz68pmy; Mon, 22 Sep 2025 08:50:10 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R13" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4cVcJ25n0Tz3vZG; Mon, 22 Sep 2025 08:50:10 +0000 (UTC) (envelope-from jhb@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1758531010; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=4Rlu9wAz+YriA/eAjJvZDDQGHjH37ZDWcFNTPwdYIg8=; b=MERDp5/8kZyYRXXCscBCgxMyXuFSo/xUIfoS3NC724QrpSol2ah+WLrshC2B/91TLKnjCr 4x1sjDnujuaYZVabqjo29R86cmeEMveOiT2pKCqZdDBNCQXYNQrcNzCoioNQqBua0M0jrZ zgRAHrqsfynfcmjiF7RoIS0PEMCSdJS5NgDEJAP+PKJrJ4/od07A/y5Vt+OCry+HoK6Vgu UBBwWj1Fg9stiAJIbreMebodbH9zfmKgEaEEZ8qIOmBH8i3rZkUfEnthzlKVMMZxD4x0Cd CNuNLDaBB8wpDcY9kyZ+6WlFSJHJ0dVa+q0YfYPqjSdasTTgp29erKclTKwQ7Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1758531010; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=4Rlu9wAz+YriA/eAjJvZDDQGHjH37ZDWcFNTPwdYIg8=; b=J4Ghzd40V6RdUez/bO7NgqPe7hVYfSRKdHk4hBZV53YUs2MHyCyvlUomZNy+BDRvojY5gT H6W5MkD3ORn+g5Sv5NXLBJyDzi1gZM0k7F7aeBpOcG3reUlSw/2CWn0pt4VjyQ3OW9UHFb IFJ5bg+t5tS2+Yx8V3eW86nfqLBrKce3u1BAb7HWFp+VkfHU70ArpBizNS5rZr21HKY+QR TfHL7VN46s/42oWwdAoDqaDiGLEmAIRnJB58ddRTWmb5qhFPoDNvhSybWx1yPYGnz7cys9 eecvEns92vc2lxtOsM9gYlbpCDxqpSUMhMlKMnm6x2J5myaiBadHrsupyYAWKA== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1758531010; a=rsa-sha256; cv=none; b=uQBeaBuYcrDH6AaSTJTGaaIMk+NpNzBHVmGEY1wv+bpzVpZ9yxsEw82IeOAyqOS1lctUVI rwkQtOuvlQN9d+BymNqlqSTryZcv0tMB8wjyk/Mucm/XIKY/0plMeOHvqG7nxDgk2sh5rg 3eKWki6fM0cVc/LXeqTNbvBSuoGe7lU+yBqcrftssfT6gGwwdpZ6lyaxun4sv5pKVWYVAQ iwz35AT3mhMP4VwmhwbZeYIciw3Ma7lKdaDS0SyY2yp/hJcwp1cqDYys51Uwi56NcMs2Dz sakYBXshLkxDeh89PbrTZ/BmzYgPgn0L/ndKcKyjBRBE2UYFVuYIAYRSbYaKRg== ARC-Authentication-Results: i=1; mx1.freebsd.org; none Received: from [10.255.53.30] (nat-184-221.net.cam.ac.uk [131.111.184.221]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 4cVcJ21yl3zl04; Mon, 22 Sep 2025 08:50:10 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: <02323a46-fb47-444b-812a-1ec199a654d1@FreeBSD.org> Date: Mon, 22 Sep 2025 09:50:09 +0100 List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-main@freebsd.org Sender: owner-dev-commits-src-main@FreeBSD.org MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: git: 40a42785dbba - main - fcntl(F_SETFL): only allow one thread to perform F_SETFL Content-Language: en-US From: John Baldwin To: Konstantin Belousov , src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org References: <202509191419.58JEJsvj031867@gitrepo.freebsd.org> <92831372-745d-4612-b38f-aeb235dd8cca@FreeBSD.org> In-Reply-To: <92831372-745d-4612-b38f-aeb235dd8cca@FreeBSD.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 9/22/25 04:41, John Baldwin wrote: > On 9/19/25 10:19, Konstantin Belousov wrote: >> The branch main has been updated by kib: >> >> URL: https://cgit.FreeBSD.org/src/commit/?id=40a42785dbba93cc5196178fc49d340c1a89cabe >> >> commit 40a42785dbba93cc5196178fc49d340c1a89cabe >> Author: Konstantin Belousov >> AuthorDate: 2025-09-11 10:05:04 +0000 >> Commit: Konstantin Belousov >> CommitDate: 2025-09-19 14:19:13 +0000 >> >> fcntl(F_SETFL): only allow one thread to perform F_SETFL >> >> Use f_vflags file locking for this. >> Allowing more than one thread handling F_SETFL might cause de-sync >> between real driver state and flags. >> >> Reviewed by: markj >> Tested by: pho >> Sponsored by: The FreeBSD Foundation >> MFC after: 2 weeks >> Differential revision: https://reviews.freebsd.org/D52487 > > Thanks for fixing this. I still slightly worry that "home-grown" locks > aren't visible to WITNESS and it's checking. > > I was also expecting this to require more changes, but apparently if a > process directly invokes FIONBIO on a file descriptor, f_flags isn't > updated currently. I wonder if that is a bug. (Similarly for FIOASYNC.) > > Oh, we do handle that, but poorly. We don't revert on errors, and this > should be updated to use fsetfl_lock now I think: > > kern_ioctl(...) > { > ... > switch (com) { > ... > case FIONBIO: > if ((tmp = *(int *)data)) > atomic_set_int(&fp->f_flag, FNONBLOCK); > else > atomic_clear_int(&fp->f_flag, FNONBLOCK); > data = (void *)&tmp; > break; > case FIOASYNC: > if ((tmp = *(int *)data)) > atomic_set_int(&fp->f_flag, FASYNC); > else > atomic_clear_int(&fp->f_flag, FASYNC); > data = (void *)&tmp; > break; > } > > error = fo_ioctl(fp, com, data, td->td_ucred, td); > out: > > I think instead we want something like: > > int f_flag; > > switch (com) { > ... > case FIONBIO: > case FIOASYNC: > fsetfl_lock(fp); > tmp = *(int *)data; > f_flag = com == FIONBIO ? FNONBLOCK : FASYNC; > if ((fp->f_flag & f_flag) != 0) { This is wrong, should be: if (((fp->f_flag & f_flag) != 0) == (tmp != 0)) > fsetfl_unlock(fp); > goto out; > } > data = (void *)&tmp; > break; > } > > error = fo_ioctl(fp, com, data, td->td_ucred, td); > switch (com) { > ... > case FIONBIO: > case FIOASYNC: > if (error == 0) { > if (tmp) Probably 'if (tmp != 0)' > atomic_set_int(&fp->f_flag, f_flag); > else > atomic_clear_int(&fp->f_flag, f_flag); > } > fsetfl_unlock(fp); > break; > } > > out: > > This only updates the flag if the underlying ioctl succeeds, and it also > avoids invoking the underlying ioctl if the flag is already in the correct\ > state. > -- John Baldwin