linux-kernel - Re: [PATCH v6 6/7] kernfs: Introduce hashed rw-sem to replace per-fs kernfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <23ceb6f7-93b8-f633-09bc-76bccfa0fa10@oracle.com>
Date:   Fri, 25 Feb 2022 16:52:57 +1100
From:   Imran Khan <imran.f.khan@...cle.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     tj@...nel.org, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 6/7] kernfs: Introduce hashed rw-sem to replace per-fs
 kernfs_rwsem.

Hello Al,

Thanks for the feedback and suggestions. I have addressed these in v7 of
the patchset at [1].

On 18/2/22 2:25 pm, Al Viro wrote:
> On Wed, Feb 16, 2022 at 03:57:09PM +1100, Imran Khan wrote:
> 
[...]
> 
> Maybe...  TBH, kernfs_remove_ns() calling conventions suck; "move this
> (wherever it is) there under that name", especially combined with
> topology-modifying moves, makes life very unpleasant for callers who
> want to use it safely.  And I'm not at all sure they manage (or care to)
> use it safely...
> 
> There are very few sources of cross-directory moves in the entire system.
> One is cross-directory cgroup rename(2) (already serialized on per-fs basis
> on VFS level), another is device_move().  Which is uncommon (5 callers
> in the entire system, one in drivers/media/usb/pvrusb2/, another in
> drivers/s390/cio, 3 more in net/bluetooth) and AFAICS unsafe wrt e.g.
> kobject_get_path().  Not for kernfs data structures - unsafe use of
> kobject->parent pointers.  I could be wrong - that's just from some grepping
> around, but it looks like a use-after-free race in the best case.
> 
> So changes of kn->parent can be considered a very cold path.  Just make sure
> you pin the damn thing, so it won't disapper away from under you while you
> are grabbing the locks.
> 

Done.

>> I have run my tests with lockdep enabled as well. Could you please let
>> me know if there is something that can be done as proof of correctness.
>> For sanity of patches, my current tests include LTP sysfs tests, CPU
>> hotplugging and cgroup access/creation/removal. I am booting oracle
>> linux as well which involves cgroupfs access(via systemd). If you could
>> suggest some other tests I can execute those as well.
>>
>> Also regarding locking rules, I am not sure where to mention it. If I
>> put accompanying comments, at all the places where I am taking hashed
>> rwsems, to explain why lock corresponding to a node or corresponding to
>> its parent is being taken, will that be enough as description of locking
>> rules.
> 
> A separate text, along the lines of "foo->bar is modified only when we
> are holding this, this and that; any of those is sufficient to stabilize
> it.  Locking order is such-and-such.  Such-and-such predicate is guaranteed
> to be true when you acquire this lock and must be true again by the time
> you drop it.", etc.  Some of that might go into the comments somewhere
> in source (e.g. when it's about data structures dealt with only one
> file, or in the header where the data structures are declared), some -
> in a separate text, perhaps in fs/kernfs/, perhaps in Documentation/filesystems/)
> 
> And, of course, there's the cover letter of series and commit messages.
> 

I have added a document to describe usage and proof of correctness for
hashed locks. This should highlight if I have misunderstood something or
if I have overlooked some scenarios while making these changes. I will
await your feedback regarding this.

Thanks
 -- Imran

[1]:
https://lore.kernel.org/lkml/20220225052116.1243150-1-imran.f.khan@oracle.com/