From nobody Mon Sep 22 08:41:18 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 4cVc5r4sqrz68nwB; Mon, 22 Sep 2025 08:41:20 +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 4cVc5r48HNz3tdh; Mon, 22 Sep 2025 08:41:20 +0000 (UTC) (envelope-from jhb@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1758530480; 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=8m2BGL/L3jrh1QfOvtfW1KXFfxnFUVSdOocHbtijljM=; b=ni9ISaZXoRcCp36FX/5zAbGqw7ZRI1pPpqYybYRA55ZWGK3mPu4UV9JyDklPFbzLH/YVhL mdW+KA+viDes5OEyQkjtJYygt1Ep1AFr9m9ENEscsTTPKIafSsEhnoZrJRl3WSvj5lp9g+ WDT8p+rdJX0KXBzsp/IRaT0uRDlyq20DIz/d8tqOcT6A+MoFcwI1ygePgh/g5mj6GVp6a/ GRxcQBG/sk0YlXhlEcGHi/vnv93lP+efLasRS2i/QAZb5+h51r6n0TPG9o6yeeS0ZOCvsw KxcmwJxfOnEs6w5mmFk1/0k4IOgi+6DFPQVYSejznoyWzRcGO0kl7Nwa1D7PhQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1758530480; 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=8m2BGL/L3jrh1QfOvtfW1KXFfxnFUVSdOocHbtijljM=; b=pdksjgwia7tQWSygkhMmzsR8eRSEx7nYsPy27XmOAnsJESM4A8qO/F7Rb1MIwrckTUReuS AUxk6ukjsyFU8m3Vu858tkWq+Fx42esbU4bE6D2K2kDMXy3FSAiuzBLZLiip05rc2iKAbx ubDQxehe+iee48xT1WmZL5yieybyftlvol8A9HwqRmbzLeSY4nCqY7wfMgAv2HRA61Wjoz s+mg0fEfojp3ZoOZ+sx3t2NnuEaAj8WbSziK6PF/AywhgCRRodab6aVunulESrdzRM/eJ5 7he9i9NZabfl7QIIlGlnsceOjD3blPhA5x0wgrGl9RnQG0CwJbMgxfrXZPHTAA== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1758530480; a=rsa-sha256; cv=none; b=mkgOooPtQ8GszRVb9P7Qnn1nKExcShIwTEAczR9FqWJexSQnfNbDS+FRhLebqXXjPeqXGC JHybgyVlmbrYVg2J2ZkDAQf8uSU7Lx7kHxfsNEGyzs0+eXA09HykE7Esl6U9wX9jgCcDRn qxTYyJ4c+4v/pPjDzm96zrSV/hnyI2ipySZgNFwF+rf7ydvi8ky7qYv9qclvlpuA+0GKsL NICFd/SG7kHkqpH8yKllX8+srWVLCU3pYId5tkPUPiQI+a4DyXc8NHMJqm5S+VwYBMIjC4 +1eoJa1+jIiUcqv4JfxoCvvybsRFWFPClNuBdMd7M1W3WTo4f04gral0N8SphQ== 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 4cVc5r0PxJzlmQ; Mon, 22 Sep 2025 08:41:19 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: <92831372-745d-4612-b38f-aeb235dd8cca@FreeBSD.org> Date: Mon, 22 Sep 2025 09:41:18 +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 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> From: John Baldwin In-Reply-To: <202509191419.58JEJsvj031867@gitrepo.freebsd.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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) { 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) 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