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: <61000bfd-7778-1d59-4aef-678eb0583b0c@oracle.com>
Date:   Thu, 14 Apr 2022 10:01:35 +1000
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.7.0
Subject: Re: [RESEND PATCH v7 1/8] kernfs: Introduce interface to access
 global kernfs_open_file_mutex.
Content-Language: en-US
From:   Imran Khan <imran.f.khan@oracle.com>
To:     Al Viro <viro@zeniv.linux.org.uk>, tj@kernel.org
Cc:     gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
        "Eric W. Biederman" <ebiederm@xmission.com>
References: <20220317072612.163143-1-imran.f.khan@oracle.com>
 <20220317072612.163143-2-imran.f.khan@oracle.com>
 <YjOpedPDj+3KCJjk@zeniv-ca.linux.org.uk>
 <10b5d071-7f69-da59-6395-064550c6c6cb@oracle.com>
 <YkxRDJ2ynEHGdjeT@zeniv-ca.linux.org.uk>
 <0dfe1056-3dc5-4d31-698e-e2c075ffd6ee@oracle.com>
In-Reply-To: <0dfe1056-3dc5-4d31-698e-e2c075ffd6ee@oracle.com>
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?VHpXODVpak1QejZ1MmRXdzNFRmhndE9MODZoZXJtdVBxRUhSK004bGdvaG9U?=
 =?utf-8?B?aU84SVU4bUNwUzVPRUUrdG1UY2JFaXJ3anZtSmoySXgyeWhQOEV6U0JseXQ1?=
 =?utf-8?B?UEllL0R5UzU1ZklOZFprSWdqUlRCdE5SVmVsL05OcFMyOHJMb1UyVGgxb3c5?=
 =?utf-8?B?b2ZrYm9GU21kSy9BRVA0SjBhY3NCdzd4b3lZdWl4WDNES3dCYXI5a3ROUDJR?=
 =?utf-8?B?QkxDTGl3WmxPK2FDVE9zSlNSMm1XbFZRWHYrM2lHeUd3Sy9FYXFkM2szTFRa?=
 =?utf-8?B?OFk1Ri9Ra0xpZk9McTB0Sys3VkpIeVpkV2JmWjRlK2xvcGlOM0M1UmlQSEhI?=
 =?utf-8?B?cEl5elpibFluTm5IUDBFRXBDNHROMlY2d0lTcXhESCtvZ2Z4bEJsZEJpajF5?=
 =?utf-8?B?NHRmY1R5UWJiRkNzN1V6OGNhUk8velovSTF3ZXNVVVdobXJhaENKSXlYb0dO?=
 =?utf-8?B?VWNjR3JiNGFFSnlTWC9odjZITGt1dkl4aXIwcjRBd0FBTDA1RGVqcWRoMzRD?=
 =?utf-8?B?cjZLaHhwTmFPVFE0WklpWlk2UXNtL1JKZU5haHIvOG9qdjZsOHJocUZLSEdK?=
 =?utf-8?B?WC9MT0NIbTJNVkhTYzNsb1lINnVSZkREWG1WMklRTHh6YUsxZlJpeGxkSlk2?=
 =?utf-8?B?c0VjY29qSmRTaGcwajE4eVM0YVkxWXFEaTRlQzdFa3l0N3F4VUk1andUeUp4?=
 =?utf-8?B?TWxSSnhpdmN3eDkrcFU3Qnl2M1RmdE9TcWcrQVl1UEdBMWJ4NTV6QjgwRFBE?=
 =?utf-8?B?N3U0dU4xR01mU3RBQVlIRFc2bWY2eFlSWDFvMncvSW9ncnNacEFsanhZVGIv?=
 =?utf-8?B?TUlTSTBvS0tNdzZVTkdNWUtrUkVhczZuS1hLaDRSN0VCclpiVnZzdEVPQzVJ?=
 =?utf-8?B?bzhwZFk0UnZuT25BSm1ST292TG1NRGJWMmp0d3ZCQXZPc3FwMmc2Qnp4Ym0y?=
 =?utf-8?B?THR0by9xcCtPM2w5azFGWmZKTVY0RFFlaUZCTWFZcG5CanJoWDA0WEs3dk55?=
 =?utf-8?B?YnV1NlVJbEJXLytPQWNQS2VPTkZkT3lJZHB4WjJJRVdSVmprSGtHa3lNeVhl?=
 =?utf-8?B?ZnBaWHpYUndWT1Ewa2VrMW1YWUZsT1o2bXFWSzhNOVhrdkJSUlVBWDRSNnVm?=
 =?utf-8?B?aVBwUXZRVTVwTkNHNkhxK2F3bXUzTTVLbEFuMjhxMjd0Mlh3YTkrOWh2RzJv?=
 =?utf-8?B?YlNHdm55cHhTVzcwNVRDQnV5N3daSEM0U1RleDd2c094czUyclVEODdaaUc3?=
 =?utf-8?B?T2VFd1kwVVNYcDByZDRHbU1lTy9ka2xFTEdSNEtsYTlJMEpUNzAyMDZZWis0?=
 =?utf-8?B?cXVyeUg5ZERSNFpDK0NsZVFjWjEvZ3hzVVp4S09Edms5NnRrTGxEZU5GRXkx?=
 =?utf-8?B?MVpOU2xxMW1SbytGLzJqeG9DOUZzaG1iak5UL3pmdmxESkFoejYyVWlzempT?=
 =?utf-8?B?cS9xRjdkUG1Wc3ZQZ2hCMThxQWg3L2xpekJGVWJpZHNYOFl2RmhHVkFiU3Aw?=
 =?utf-8?B?MUkvb3Rta2VPMHNQUm1kV1JwVzdBdGFpTDNBSS9KYlBTeDhWSDdPN0lWQjIx?=
 =?utf-8?B?UXJQbVFwK1F1YTV3YzV4NEhYY2V0c0dyVDFUQkREdmdTNWk5azMrRmtDVDBt?=
 =?utf-8?B?U0Y1aHNmY01TSDJPRVNlM0U0MzNGa1A3L3RjampyLzBmSld2UUdKemRyYmJs?=
 =?utf-8?B?SlJ0UWdKdEtyaTQ4Q3dHQmJOSHUxTTU1UkRCenA3VXhQb1hBU1QrRWhUSW1u?=
 =?utf-8?B?SFFXUEl0SzBQZlY5RWd3SHIzVzBNOTNwNGhoYzUyYWV6T1YxbkJzcVdFMGtl?=
 =?utf-8?B?aVR6UG9vVlVUQzY0Q0NRaVlTRkRrWXViajBGVUppYW1kMk1wVHBrNjFhS0ZK?=
 =?utf-8?B?RTM2bXFYU2hldTdtdUpxaVNvV0paZUZCTzgvSEZFMlVVeEo0NllyQ3hZMmh1?=
 =?utf-8?B?MXovK2NmNE10UThYeXhuelhGaTZGMnlFRXBaNGszV2hOa1ZtWUU5MzdWa1Vu?=
 =?utf-8?B?d1ArUWg5Z0c3V3VOK1grVkk0NzUyVklNb09IQlVWeUZRckVNVkxxQU4rNGNR?=
 =?utf-8?B?WmNDWlZmT1FmNDB2VDhkNHh3RWhzOCt4UUhWNFdmZThOVkdvUFB3cGlva0kw?=
 =?utf-8?B?WURFdkRrZytyMDlQY245YVIzd2pkTm41cno2QmN6REkvb1owU1Zrc25kb01N?=
 =?utf-8?B?QjgrcGpnM1ViSlZiaE13WVJVaC9mYzhiSUVraytHWDFoYVBCNW9lUTBKQ2FE?=
 =?utf-8?B?bEVsYVdNYXhpZ0IwLzlQL1RYNHk0alZVbHAyckQ4d1J6dFRSM3JSS2tGTlZZ?=
 =?utf-8?B?VEtOUU04bHRibEN3QmJvVHVWYjVxemN1NFk2dkJzQWpvcnhiK2VHTkp2bGJs?=
 =?utf-8?Q?NH3My5VThr2IJpJQ=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d82ec50d-b0cf-41ea-f7c0-08da1da9f6b6
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4468.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Apr 2022 00:01:45.5491
 (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: J1VnYhZMUNTPxvPd6NkgaiI23NLVJ19ur+oA5+YL6ALL9dXPBTBzb8+cOqHB9fv+Bnyk/EndQQ7zDceUAzPITw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR10MB4441
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-04-13_04:2022-04-13,2022-04-13 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 suspectscore=0
 malwarescore=0 mlxlogscore=999 spamscore=0 adultscore=0 phishscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2204130114
X-Proofpoint-ORIG-GUID: OUy23_8Jy2Jjj70f7NM2LPyaaELbzZvL
X-Proofpoint-GUID: OUy23_8Jy2Jjj70f7NM2LPyaaELbzZvL
X-Spam-Status: No, score=-4.1 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, Hello Tejun,

I have sent v8 of the patchset at [1]. I have incorporated your
suggestions and have also addressed the issue of not locking correct
nodes during kernfs_walk_ns.
I have not yet make the changes to make kernfs_find_ns use qstr because
this part is not clear to me. My understanding is that kernfs_find_ns
is looking for node of given name under a parent, so we need a buffer
in kernfs_walk_ns to hold the full path and then use strsep to take each
path component and look for it under parent (the node obtained during
previous iteration). For sure I am missing something from your
suggestion, about using qstr and removing strsep, but not sure what.

Could you please have a look at current version and let me know your
feedback?

Thanks
-- Imran

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

On 6/4/22 2:54 pm, Imran Khan wrote:
> Hello Al,
> 
> On 6/4/22 12:24 am, Al Viro wrote:
> [...]
>>
>> What for?  Again, have kernfs_drain_open_files() do this:
>> {
>>         struct kernfs_open_node *on;
>> 	struct kernfs_open_file *of;
>>
>> 	if (!(kn->flags & (KERNFS_HAS_MMAP | KERNFS_HAS_RELEASE)))
>> 		return;
>> 	if (rcu_dereference(kn->attr.open) == NULL)
>> 		return;
>> 	mutex_lock(&kernfs_open_file_mutex);
>> 	// now ->attr.open is stable (all stores are under kernfs_open_file_mutex)
>> 	on = rcu_dereference(kn->attr.open);
>> 	if (!on) {
>> 		mutex_unlock(&kernfs_open_file_mutex);
>> 		return;
>> 	}
>> 	// on->files contents is stable
>> 	list_for_each_entry(of, &on->files, list) {
>> 		struct inode *inode = file_inode(of->file);
>>
>> 		if (kn->flags & KERNFS_HAS_MMAP)
>> 			unmap_mapping_range(inode->i_mapping, 0, 0, 1);
>>
>> 		if (kn->flags & KERNFS_HAS_RELEASE)
>> 			kernfs_release_file(kn, of);
>> 	}
>> 	mutex_unlock(&kernfs_open_file_mutex);
>> }
>>
> 
> I did something similar in in [1], except that I was traversing
> on->files under rcu_read_lock and this was a source of confusion.
> 
>> What's the problem?  The caller has already guaranteed that no additions will
>> happen.  Once we'd grabbed kernfs_open_file_mutex, we know that
>> 	* kn->attr.open value won't change until we drop the mutex
>> 	* nothing gets removed from kn->attr.open->files until we drop the mutex
>> so we can bloody well walk that list, blocking as much as we want.
>>
>> We don't need rcu_read_lock() there - we are already holding the mutex used
>> by writers for exclusion among themselves.  RCU *allows* lockless readers,
>> it doesn't require all readers to be such.  kernfs_notify() can be made
>> lockless, this one can't and that's fine.
>>
> 
> Thanks for explaining this. I missed the exclusiveness being provided by
> kernfs_open_file_mutex in this case.
> 
>> BTW, speaking of kernfs_notify() - can calls of that come from NMI handlers?
>> If not, I'd consider using llist for kernfs_notify_list...
> 
> I see it gets invoked from 3 places only: cgroup_file_notify,
> sysfs_notify and sysfs_notify_dirent. So kernfs_notify should not be
> getting invoked in NMI context. I will make the llist transition in next
> version.
> 
> Thanks,
> -- Imran
> 
> [1]
> https://lore.kernel.org/lkml/20220324103040.584491-3-imran.f.khan@oracle.com/