Date: Sat, 11 Jul 2015 16:56:59 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: Konstantin Belousov <kostikbel@gmail.com> 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> In-Reply-To: <20150711124848.GG2080@kib.kiev.ua> References: <1436569684-3939-1-git-send-email-mjguzik@gmail.com> <1436569684-3939-3-git-send-email-mjguzik@gmail.com> <20150711124848.GG2080@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <mjg@freebsd.org> > > > > 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 <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150711145658.GB1433>