From nobody Tue Sep 23 15:34:24 2025 X-Original-To: dev-commits-src-all@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 4cWPD809y8z68n1r; Tue, 23 Sep 2025 15:34:32 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (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 4cWPD76Xv2z3rTw; Tue, 23 Sep 2025 15:34:31 +0000 (UTC) (envelope-from jhb@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1758641672; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Cc8pxXthp2bZidOu7lQ+sYZ5JQdPjmC4Qc+gqT7MdA8=; b=jT5PfEL9g4WNQtcFM+JDRmkW0qbcWn35p4C7044CEyFTZd6c58NqPs+RnfZdev//l2OBGX P+EXfRVVyRVGJ1P5FjcNg0/A+RUNo+gqNwnzlhUM6BfnC9WiSeSzl6WUX/iZbTfc/bokIY 55Jms2VrDuyy0ghHDP5fLo4vpwQsv2zziAIXgeE0M3X73xMBa2GuPOnRES9V8/dfv+Qdsw +dMEudgtfoTf3j+tf1tI2jDM8a7M7yEIdT6vTa3NySnVkJLdB6Atq4SEbHtlOoPpozg80z kBBLFMp7FMV6/HD/Cb9XOwaxllu3vO3fMqvdJml40NuV7CJDEkLPEZw4NSidng== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1758641671; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Cc8pxXthp2bZidOu7lQ+sYZ5JQdPjmC4Qc+gqT7MdA8=; b=pHtvGIruXaHQIz9K+vc0xnLW62FJDFXioghCwAn+CUqL1W1Z5aucfq6P5YpMTf1Y5ZQDNS IqmsoMQOZKhlhuMqPycoB4i9UWGSEr2uQuMn0GUU1SYfS+vNTlGysJitCc2ryBcvpb1Fa8 AixeuqkBuLL++zE33qKMbbWxbWIyWsElQyLMrwOPTgzlNeYnt4riIO/O8RsX9Xd9IGTtyz 8W+FAN4zOQ9LkYxmBaYphahln/PYSfRGlHAy1oRoIzTmHzKZ/G6rmq3xGfKhwLrTqziupr NgMihwo4v9H0kYWFdte7cS9pCifI12/TDkus0oM4RrlyKWHYt8mIHXZQHi2+8Q== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1758641671; a=rsa-sha256; cv=none; b=AjUw7Ta0oLKCX21wkTXxsbb2ESDw0p4wf8OmfL1D9EE/HirICK1RzGwIPqA0/44emcWGpy S5oQVvmrfzR9ig5zaAvPzsk4aAvbGCaQaKAHZRUpCj9g/NFC+JGGAQqupXD6/oPxt0SBOj sAHKH6HLFoWg1FBL4P4nOWEHAU0Kxl6hJtgwe/8MpwfbZvprmSzHRs892Bjr+SE3saJUmx irybiGMpLbDeNSWl6vpfh8i6eV1583CyKBY4wvoV+PZr3BmTrQXMsMHCWOwdYLkD5+nMXG pohDfr7WAobcen9heM9LDVHF0gYTmWOBA3RRu4FrkAjiyU07Tf4kuX9sBIQo1Q== ARC-Authentication-Results: i=1; mx1.freebsd.org; none Received: from [10.20.22.243] (unknown [195.29.209.28]) (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 4cWPD71SGFz1R0l; Tue, 23 Sep 2025 15:34:31 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: Date: Tue, 23 Sep 2025 17:34:24 +0200 List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@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 Cc: 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> <02323a46-fb47-444b-812a-1ec199a654d1@FreeBSD.org> From: John Baldwin In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 9/22/25 17:05, Konstantin Belousov wrote: > On Mon, Sep 22, 2025 at 09:50:09AM +0100, John Baldwin wrote: >> 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. > > So will you handle this? I can. Do you think this is a good idea? :) -- John Baldwin