From nobody Fri Aug 27 18:40:01 2021 X-Original-To: freebsd-fs@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 824921773B5D for ; Fri, 27 Aug 2021 18:40:11 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (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 did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4Gx7n708LWz3vvS for ; Fri, 27 Aug 2021 18:40:10 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.16.1/8.16.1) with ESMTPS id 17RIe23Z018668 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Fri, 27 Aug 2021 21:40:05 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua 17RIe23Z018668 Received: (from kostik@localhost) by tom.home (8.16.1/8.16.1/Submit) id 17RIe1h9018658; Fri, 27 Aug 2021 21:40:01 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Fri, 27 Aug 2021 21:40:01 +0300 From: Konstantin Belousov To: Alexander Lochmann Cc: freebsd-fs , Horst Schirmeier Subject: Re: Various unprotected accesses to buf and vnode Message-ID: References: <55f3661e-2173-793e-4834-bbcd79d3d99e@tu-dortmund.de> List-Id: Filesystems List-Archive: https://lists.freebsd.org/archives/freebsd-fs List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-fs@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55f3661e-2173-793e-4834-bbcd79d3d99e@tu-dortmund.de> X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FORGED_GMAIL_RCVD,FREEMAIL_FROM, NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.5 X-Spam-Checker-Version: SpamAssassin 3.4.5 (2021-03-20) on tom.home X-Rspamd-Queue-Id: 4Gx7n708LWz3vvS X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-ThisMailContainsUnwantedMimeParts: N On Fri, Aug 27, 2021 at 03:48:38PM +0200, Alexander Lochmann wrote: > Hi folks, > > I'm still analyzing our LockDoc (lock analysis) data for FreeBSD. I came > across accesses that do not adhere to the locking documentation. I cannot > tell whether these accesses are made deliberately without locks or not. > I listed them below. > > Can you please shed some light on those cases? > > Thx and regards, > Alex > > - Write to buf.b_error without buf.b_lock: > https://github.com/freebsd/freebsd-src/blob/main/sys/kern/vfs_vnops.c#L2846 I believe this is fine. If the buffer is still on the delayed write queue, then nobody can start the write, and buffer cannot leave the queue because we locked the bo lock. It might be slightly formally better to move clearing of b_error after we obtained the buffer lock, but I do not want to change this code just for formality. It might be that the clearing can be avoided at all, in fact. > > - Read of buf.b_blkno in cluster_write(): > According to the documentation b_lock is needed. > Is b_blkno maybe a read-only element of struct buf? No, b_blkno is not read-only, it is the disk block number for the block, as opposed to b_lbklno which is logical file block number. The buffer is instantiated with b_blkno == b_lblkno, and when the buffer is mapped to the real disk block, b_blkno is updated. Could you show me the backtrace of the situation where cluster_write() is called with unlocked buffer? > > - Read of buf.b_flags, buf.b_xflags and buf.b_vp: > https://github.com/freebsd/freebsd-src/blob/main/sys/kern/vfs_subr.c#L2351 > Are those reads innocent races? > According to our data, buf.b_lock is not acquired. These are fine. We check that nbp buffer is still on the clean/dirty queue of our vnode, and we own the buffer object lock. Since buffers are moved from the queues under the bo lock, the operation is safe. You may think about it in the following way: - the update of the flags word require owning corresponding lock to not loose other updates - but changes of the flags that are tested in the referenced lines are also protected by the buffer object lock > > - Write to vnode.v_bufobj.bo_object: > https://github.com/freebsd/freebsd-src/blob/main/sys/vm/vnode_pager.c#L291 > According to the documentation, '[...] the vnode lock which embeds the > bufobj' is needed. However, interlock is taken in line 276. > Is the interlock equivalent to the vnode lock? > (I assume 'the vnode lock' refers to vnode.v_lock.) vnode_pager_alloc() must be called with the vnode exclusively locked. This is asserted at the beginning of the function. > > - Is buf.b_bufobj a read-only element? You should scope the question. While buffer is owned by a vnode, b_bufobj is constant. Since buffers are type-stable, they migrate between vnodes as cache finds it required to reclaim and reuse. There, b_bufobj is changed.