From owner-freebsd-fs@freebsd.org Sat Jul 11 14:57:05 2015 Return-Path: Delivered-To: freebsd-fs@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id E9ABE998F64 for ; Sat, 11 Jul 2015 14:57:04 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wi0-x22a.google.com (mail-wi0-x22a.google.com [IPv6:2a00:1450:400c:c05::22a]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 7D58A107E for ; Sat, 11 Jul 2015 14:57:04 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by widjy10 with SMTP id jy10so36197752wid.1 for ; Sat, 11 Jul 2015 07:57:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=ySmpa49XauztUdX1z+Yo8Bnw7jFmzOE7LYYpvIbWUPE=; b=JfuGVgqJ6W2KG7lu8M1lQ76uYMz+LETiFK/2ZIxRoLdQpyt6s3bTzGZ4xP4WW1pNWk m0Ih40C4TzH+L++M926ujjwVQqP4EjskUSI908IlAfACngZR/ffXzqdfe+FFncbbhukV x3Dj3FVWXNA6GkVj6wUcEk1AniF36KX9Ud4H7Fz9VzusgyouYhe1AZ7FFuT1nBI3GJbZ zB0IYCk9uJQVI3d3oVkACmix58pwFQuQfvQ+2CIXDKzTXD6d3A/bAWEeZW9bnN+RRBsj +ofuouT7TTo8LrOZWba1dZiMEXsIEC9+Mu4S83DFeNMe5j3CpGqNBCfMBfUlPRzPK09L ozEA== X-Received: by 10.194.175.65 with SMTP id by1mr54712027wjc.152.1436626622935; Sat, 11 Jul 2015 07:57:02 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by smtp.gmail.com with ESMTPSA id j6sm3912040wix.5.2015.07.11.07.57.01 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sat, 11 Jul 2015 07:57:01 -0700 (PDT) Date: Sat, 11 Jul 2015 16:56:59 +0200 From: Mateusz Guzik To: Konstantin Belousov Cc: freebsd-fs@freebsd.org Subject: Re: [PATCH 2/2] Create a dedicated function for ensuring that cdir and rdir are populated. Message-ID: <20150711145658.GB1433@dft-labs.eu> References: <1436569684-3939-1-git-send-email-mjguzik@gmail.com> <1436569684-3939-3-git-send-email-mjguzik@gmail.com> <20150711124848.GG2080@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150711124848.GG2080@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 11 Jul 2015 14:57:05 -0000 On Sat, Jul 11, 2015 at 03:48:48PM +0300, Konstantin Belousov wrote: > On Sat, Jul 11, 2015 at 01:08:04AM +0200, Mateusz Guzik wrote: > > From: Mateusz Guzik > > > > Previously several places were doing it on its own, partially > > incorrectly (e.g. without the filedesc locked) or even actively harmful > > by assigning rootvnode without vreling it or populating jdir. > > > > This functionality should not exist and will be garbage collected after > > all callers are properly reviewed. > Why do you think that this code 'should not exist' ? The code comes due > to the need of some kernel processes to do file i/o, but also from the > desire to avoid having all kernel processes reference some vnode, even > if not needed. How do you propose to make it possible to use lookups > etc from a kernel process ? > Currently random threads will get the reference at random times which imposes imposes the need to iterate over them in mountcheckdirs if rootvnode has changed, which is a rare event. If the vnode was already set for all kernel procs it would be cleaner with no serious downside that I can see. There is a an additional minor problem of rootvnode update not being synchronised with code filling cdir+rdir, i.e. there is a hypothetical potential for a use-after-free. Primarily though it is unclear if there are correctness/security issues here. Some of original offenders can be reached in both kernel and userspace process context, which may or may not be problematic for them. Preferably the code would assert the context, if any requirements are there. But that's not something I'm interested in playing with right now. > Regardless of the note above, the patch looks fine, it is the good cleanup. Ok, thanks for review. -- Mateusz Guzik