Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <23ceb6f7-93b8-f633-09bc-76bccfa0fa10@oracle.com>
Date:   Fri, 25 Feb 2022 16:52:57 +1100
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.6.0
Subject: Re: [PATCH v6 6/7] kernfs: Introduce hashed rw-sem to replace per-fs
 kernfs_rwsem.
Content-Language: en-US
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     tj@kernel.org, gregkh@linuxfoundation.org,
        linux-kernel@vger.kernel.org
References: <20220214120322.2402628-1-imran.f.khan@oracle.com>
 <20220214120322.2402628-7-imran.f.khan@oracle.com>
 <YgxXh3clQqpxUPba@zeniv-ca.linux.org.uk>
 <bfdef75d-4343-2734-2723-d8546df37c69@oracle.com>
 <Yg8Rq2H1C1ihFqds@zeniv-ca.linux.org.uk>
From:   Imran Khan <imran.f.khan@oracle.com>
In-Reply-To: <Yg8Rq2H1C1ihFqds@zeniv-ca.linux.org.uk>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NW04ZGpwbks3M0xhK2dYMEFyRkhrQmVTbEtYdmZHa0YzOWk5M2RjWWRpVHpE?=
 =?utf-8?B?WUVLdGZ1NWYxNklaS084cnpPNUs5VTdpSHVFMEUvcEVnVWwzc3ErOHVCMVVu?=
 =?utf-8?B?VmxVa2IweVFJQmJLTTVzQzllRmRmeFllaTFwZUtKRkJwdnJUeDlSakdhc2lz?=
 =?utf-8?B?Ly9tV0RTc3VqUkQ1eVladHd3eVhXcFhkSWFVMENpWGZVYzRIZlhMTDZpK21R?=
 =?utf-8?B?cElTRjhhTFpSR1pjSG1Sem9McnNhU2p1MVl4TXFlRWdMV2phemp5UVJYdFBU?=
 =?utf-8?B?WHRJdTFvck84YTUxUEVpeVA2dldlZDJPZ1RIZmxpbXEybTk4V01ydCt0QTJ3?=
 =?utf-8?B?ZXVvVnl0N0RXdzZZem5qWHFyU3NKZzBIQVZjek01Sno3ZTZYTDhWK1NySXZF?=
 =?utf-8?B?MWY1eW1YZFJZVzYvcjRlWURXMUZkLzB5azljcE52ZXdQK1FHZEZvYXJoaFlp?=
 =?utf-8?B?MHE1Wjg0NG1kaWVNUmxMZ3dLQS9hNnZMbnlCTTc1VjM1L0NqN3cvV1ptalFr?=
 =?utf-8?B?S0Zqb2hCSVIrVU1nYXB1TktQSTFWMU9LdmFxSGVmMXk3amZxRHhrQ2VqZnVT?=
 =?utf-8?B?eVdmMGVmMGRFNEhRL3AyVjNoYXVZR0FoSTVkYlFXSjgyL09nR2g3NFMrcHp3?=
 =?utf-8?B?K0Z2RWxOcm9zalR2RG5HY2xDUnIvVUw1YjFFRThaaVpYbkdTcTZBR2hFakxm?=
 =?utf-8?B?QlJxdVBVQXg0MkE5aXdaZTVmQkdwUTcyRVJ1UGJKcDNVOEN1bnpsVW9CRXR4?=
 =?utf-8?B?NDZEMVJBbEk0K3QraTVpTTJlUXJzN3N2TnE5NDVmMmZTc0JtRTNWeWR4dXBw?=
 =?utf-8?B?b3V6dFIreCtIOWVaN0w2TnBPMzdtRHliTkFDeWsrVE1qYWN1d1BjcXdxRXpS?=
 =?utf-8?B?MnI0MWEvWmp1bkxNdDE5bWlEYU5sV1BFdnlxUTFRNkNvYVhJcHRTUDVNdjRr?=
 =?utf-8?B?ME9ZbGQyM29vZVhFVWpTMnl2UFMzRlMzNmpWcDd0YjZHM2I2SnptZnR0dWoz?=
 =?utf-8?B?Z2lHaG1adTVvZW5NRFBuY2JqdGNKOTMvVE95VHhiZHpWT3VORndBZlNEQVZS?=
 =?utf-8?B?V1RzYjBLb1d0NDdHZVJ4c280T2xYWTljN0xuN1hYdHJQS2g5bEh0SXozLzVW?=
 =?utf-8?B?d1VVaEdXZWxBTk9aMjhTUk8za2lUZjBTS0tiMm1GV0p1a0JRbUVZRFRNNTBB?=
 =?utf-8?B?MUlQUVZwUFMwQXhmY0pqS3k3SHowS3NrYWJ5aUZRZ1pjOFE4VjIyU084Tkpi?=
 =?utf-8?B?eWpuaUVnWEQxTHN5N2lXbXBjV3lyUDdIUzN2bFVDWHRHaFpBNFVKRlZ3R1ZK?=
 =?utf-8?B?MVlFWW9ZM3E0SnRsTS9tZEh6ckJPamVQTVF5emNOQ2VmWFhjM1drRUFxZzRJ?=
 =?utf-8?B?NStQUzVvK2R1bjAvM1E2c3h2dHlGdGpVeTBrT2N6SWxJSk5tY3E0RUZFcjdQ?=
 =?utf-8?B?aUZyaklnWDA5ZXNFZDI2Y3BmMlV0YmhNTE5ya0RSWXpEOWhJRThRN205QVZK?=
 =?utf-8?B?ci80dktiWThmRGxJOUFtK0l2b1RLKzhpNVg4ZTY4OWtXWGtMR3hXZlJaa2U3?=
 =?utf-8?B?WEI4dHVCVmlVRTh0V2NCQzVLeVJYWFRRQm9zVnBESGtNQlEwZzNvQkpGUC95?=
 =?utf-8?B?aWtjMjhFZ05rYzVrWXh4dmg4MHJ2WHFkMjAraGJvalFzOG16Q3J4SEg5NlRK?=
 =?utf-8?B?emllSmVtWFFFTWZJbktJeitEY29IMnJoRTBndGNrK1NyRDdiak1sckdjcmxi?=
 =?utf-8?B?OGZlUjNTbGhUS05DaWlZRjl2aC9oVXA0OS90a1JEN1h1N3pESm5wYXdDL0Nv?=
 =?utf-8?B?TnQ1aFNOYlZhMkltbmtEMVJzbFpFdzVkNld5eEpHT1FsV1BJaERmRE9qaGtl?=
 =?utf-8?B?SVlOWTkwUUswODArWjFtcGwyelFaWmZWanNyMFU3NlNjdFhSNkMxZzUrcWY5?=
 =?utf-8?B?WFpLelBQVEpZSExZY1VoQmNrNHdVQ1E4UWRNQmhLbFZPTk5BU0k1WExWUExt?=
 =?utf-8?B?ZHh5QUtvTmgzSUVjT2xWcmJNeDNkSnpsRTNyMnBqaDZxYVRXL3kzbWhPbGZa?=
 =?utf-8?B?Q3A1dGUvUUE5aTQxaFE4clg5cU1tS2xkYitUdldpblphbDByOWRIOEZuMk5D?=
 =?utf-8?B?NHg3NGhRU21EVnZJR1dYcW1VcEtYZVh2NmxvcEFTRDhxN3BQS1NTWEtxVE9N?=
 =?utf-8?Q?4kMYIYHKR3+BHmj7IXMZj0w=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3c96993b-5534-4500-4316-08d9f8231850
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4468.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Feb 2022 05:53:06.8060
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: QIMKlzITKg2uNd3fHVeK6a83YZmb3euPLxoPigQMar+7hsrB3H/yFZuVlZlUfN9tcF+ejDuZmSvl7+Klw0+JWQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB2460
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10268 signatures=684655
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 suspectscore=0 mlxscore=0
 spamscore=0 mlxlogscore=999 adultscore=0 phishscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2201110000
 definitions=main-2202250028
X-Proofpoint-GUID: clzK0JDAq02yZNxW6SziGFbNLUs7IPXO
X-Proofpoint-ORIG-GUID: clzK0JDAq02yZNxW6SziGFbNLUs7IPXO
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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/