NetBSD Problem Report #57552

From www@netbsd.org  Sun Jul 30 15:38:33 2023
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 3E8E61A9238
	for <gnats-bugs@gnats.NetBSD.org>; Sun, 30 Jul 2023 15:38:33 +0000 (UTC)
Message-Id: <20230730153831.89E6D1A923A@mollari.NetBSD.org>
Date: Sun, 30 Jul 2023 15:38:31 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: fileassoc(9) causes rm to take kernel lock even when never used
X-Send-Pr-Version: www-1.0

>Number:         57552
>Category:       kern
>Synopsis:       fileassoc(9) causes rm to take kernel lock even when never used
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Jul 30 15:40:00 +0000 2023
>Last-Modified:  Wed Aug 02 07:15:02 +0000 2023
>Originator:     Taylor R Campbell
>Release:        current, 10, 9, 8, &c.
>Organization:
The NetBSD Fileassociation
>Environment:
burning just a tiny bit faster with the energy wasted on the kernel lock here
>Description:
If veriexec is compiled into the kernel, then fileassoc_domain is initialized via main -> veriexec_init -> fileassoc_register -> RUN_ONCE(..., fileassoc_init) -> fileassoc_init.

https://nxr.netbsd.org/xref/src/sys/kern/init_main.c?r=1.542#594
https://nxr.netbsd.org/xref/src/sys/kern/kern_veriexec.c?r=1.27#334
https://nxr.netbsd.org/xref/src/sys/kern/kern_fileassoc.c?r=1.36#187
https://nxr.netbsd.org/xref/src/sys/kern/kern_fileassoc.c?r=1.36#171

In this case, every rm-like syscall goes through do_sys_unlinkat which calls fileassoc_file_delete which finds fileassoc_domain nonnull and therefore takes a pass through the kernel lock, even if nothing else in the system has ever used fileassoc:

https://nxr.netbsd.org/xref/src/sys/kern/vfs_syscalls.c?r=1.560#2911
https://nxr.netbsd.org/xref/src/sys/kern/kern_fileassoc.c?r=1.36#513

(fileassoc and veriexec locking -- and probably vfs semantics -- is also broken to the point that it is hard to imagine these can actually provide meaningful security; e.g., https://gnats.netbsd.org/35351 and https://gnats.netbsd.org/41251, and what happens if VOP_REMOVE fails, say because of a permissions error?)

(It's also not clear vp->v_mount is safe inside fileassoc_file_lookup.)
>How-To-Repeat:
dtrace use of the kernel lock in ordinary system usage without veriexec.
>Fix:
Yes, please!

Maybe just use a global thmap (lazily initialized) for these associations instead of fussing around with specificdata keys and wotsits.  Won't fix any fundamental locking problems of the API but it might help avoid touching the kernel lock.

>Audit-Trail:
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/57552: fileassoc(9) causes rm to take kernel lock even when
 never used
Date: Mon, 31 Jul 2023 00:55:55 +0000

 On Sun, Jul 30, 2023 at 03:40:00PM +0000, campbell+netbsd@mumble.net wrote:
  > If veriexec is compiled into the kernel, then fileassoc_domain is
  > initialized via main -> veriexec_init -> fileassoc_register ->
  > RUN_ONCE(..., fileassoc_init) -> fileassoc_init.

 None of the rest of fileassoc uses the kernel lock so there's maybe no
 reason to bother with this part either? :-)

 It should be sufficient to set a global "I've been used" flag in
 fileassoc_add() and test it in fileassoc_file_delete().

 The whole thing should be ripped out though. In addition to there being no
 locking, the call of fileassoc_file_delete is a stray piece of
 veriexec_removechk that escaped somehow.

  > (It's also not clear vp->v_mount is safe inside fileassoc_file_lookup.)

 That one I don't follow. The mount shouldn't be able to go away while
 the caller is holding a vnode.

  > Maybe just use a global thmap (lazily initialized) for these
  > associations instead of fussing around with specificdata keys and
  > wotsits.  Won't fix any fundamental locking problems of the API but
  > it might help avoid touching the kernel lock.

 From what I can tell, fileassoc is a halfassed and broken specificdata
 interface for vnodes. Given that there's a native specificdata
 interface for mounts in the vfs code, if we want to be able to do what
 veriexec does with it we should probably create a native specificdata
 interface for vnodes. There are reasons this is not completely
 straightforward (especially if, unlike fileassoc, it's expected to
 work) but there shouldn't be any fundamental barriers...

 -- 
 David A. Holland
 dholland@netbsd.org

From: Taylor R Campbell <riastradh@NetBSD.org>
To: gnats-bugs@netbsd.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
	netbsd-bugs@netbsd.org, campbell+netbsd@mumble.net
Subject: Re: kern/57552: fileassoc(9) causes rm to take kernel lock even when
	never used
Date: Mon, 31 Jul 2023 13:16:19 +0000

 > Date: Mon, 31 Jul 2023 01:00:02 +0000 (UTC)
 > From: David Holland <dholland-bugs@netbsd.org>
 >  
 >  > (It's also not clear vp->v_mount is safe inside fileassoc_file_lookup.)
 > 
 > That one I don't follow. The mount shouldn't be able to go away while
 > the caller is holding a vnode.

 The vnode can change identity while the caller holds a reference, if
 it is concurrently revoked.  Unless there's some non-obvious context
 guaranteeing concurrent revoke can't happen, access to vp->v_mount
 would race with that.

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57552 CVS commit: src/sys/kern
Date: Wed, 2 Aug 2023 07:11:32 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Wed Aug  2 07:11:31 UTC 2023

 Modified Files:
 	src/sys/kern: kern_fileassoc.c

 Log Message:
 fileassoc(9): Fast paths to skip global locks when not in use.

 PR kern/57552


 To generate a diff of this commit:
 cvs rdiff -u -r1.36 -r1.37 src/sys/kern/kern_fileassoc.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

Home	PR Database Search