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: <536f2392-45d2-2f43-5e9d-01ef50e33126@oracle.com>
Date:   Mon, 21 Mar 2022 12:57:07 +1100
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 7/8] kernfs: Replace per-fs rwsem with hashed
 rwsems.
Content-Language: en-US
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     tj@kernel.org, gregkh@linuxfoundation.org,
        akpm@linux-foundation.org, linux-kernel@vger.kernel.org
References: <20220317072612.163143-1-imran.f.khan@oracle.com>
 <20220317072612.163143-8-imran.f.khan@oracle.com>
 <YjPNOQJf/Wxa4YeV@zeniv-ca.linux.org.uk>
From:   Imran Khan <imran.f.khan@oracle.com>
In-Reply-To: <YjPNOQJf/Wxa4YeV@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?enJiZkIrVkpYaTV3cGs2eWZLWGxqRFhOOWpVSzdrRTJWazl3RUtxeWx6RERi?=
 =?utf-8?B?UlQxTmlFSG0xcmJDT1A2VlBuSmxkbnNFbFRFbzY5NEg5c1JUNUZwVVVoY290?=
 =?utf-8?B?R3diWXVmNXJMOEdMcEZ4Wko5N1lsVkdNS25qOVJjVmU4UFNyMk1PRkYvR3NS?=
 =?utf-8?B?WTYrTzdLclFtVkZNRDEzbUd5cG0xYkFVUVc1OWFyc2NVY0hUcEhZck4wc2JN?=
 =?utf-8?B?RUlMRkN0Z1ppWmFoQitKd1FaZDFUUW4wNmtLZ045TnIxaCtXNmJJdHdTeHlS?=
 =?utf-8?B?a0dDWlNPNEdYeG5hTXM4d2Z2Vm1UcGRpS1kxSWhlVW12K2kxejZWL1BrTy9C?=
 =?utf-8?B?Z1YrZW00dVpUb2RmREhUNm05eDlkdFVVWlFQOGR1ZVoyMG44THZwaHd4eFI4?=
 =?utf-8?B?akxpU1cvbytTVlJWRlVTS0N5bjdUbFQrSmdzQno1OVFNalRSUEdvL3BJZ3Jq?=
 =?utf-8?B?akdJUkczZ21Vd1VNcHYwT0d0SjdYYzlFUEtZMHpYSm9qc3EzUVV3TEd3SlU5?=
 =?utf-8?B?a3hZbWNMQTF4dzdFMS9iUEN1RndtSUY4dTFyNDZjSFFLcFhoTW40TGVVKzlX?=
 =?utf-8?B?YXZCY0JVRExOb2Vwa2lISm51TW9pYVlON2RpRjlTYm1kaHkxc1MzREo4LzRz?=
 =?utf-8?B?UDBSV3ZIMUJ5VkhuWi9uNU43UTJyajBQa2lpN1JaZWpDYndYRkdzOXFXT0ZR?=
 =?utf-8?B?T2Y0Z1VtcUdid2NYTjZKOU0xZkhtaDlKSGJBUVE4NHc5bXVCZjhpbFFsWWp1?=
 =?utf-8?B?eGFqTjhSejd0bHphZktVNWRsM2RIQnlkTjRmWG16MGtwWkluM2pXYis0WERZ?=
 =?utf-8?B?aUJMQldtVE40OThiMERQNTcrSllidThUL0x1ZVlMYXRXQkxFeEJaT1FPb3NU?=
 =?utf-8?B?dGE5YUtPWkdrVitKWVNLd3QwdE5rakpPN0ZoL3phb21RdDlwQk94YzJYcTJw?=
 =?utf-8?B?OEFZdDhQcTFJMDBUaUMwTmtaY01Ic3FCN1ZHbzUzN1liMnlaaFhOQlFBNFNi?=
 =?utf-8?B?NHdmbm5DT25IMStGUXpJUXFKQzJqcEZxTldjZHlDb3djQXFiRi9DMzhVd2Ex?=
 =?utf-8?B?aGdoeUdvTDdxVGZpcWQxRGx5RjhDT3JCS2tjZDAyWFc2Q1E5aDY0V1Qyb0VU?=
 =?utf-8?B?NUtaNWlUTFdZRHRKZS8wNzhJRkxvQ3grQ2dzNjBhdCtITFM5V3ZlWVVCa3lo?=
 =?utf-8?B?SW95WGVCczRmQURndGV0d2c5SEo3NlFzazZuWCs5SWRGeTNEcVVaNFFnb1cz?=
 =?utf-8?B?UHN2OFpsTFJ5cWZqM3k4THJVVkVIT0FsNE0zcGs4Y2JYZjlCdnBiTEV4WnF4?=
 =?utf-8?B?RjdrUDFYbXBzWHJHclVRTWdmZE4yemh5TG9BNGJQL2dma0hxSVdkblZ4Y0di?=
 =?utf-8?B?S1VqMWZmcnF1TlJtUGJaMSs3a3NYbVJyQ3V3V0dKQ28rYXQ2bnhZQU1CbzAr?=
 =?utf-8?B?M3JNd1RDSWpsMHVtU2ZKZy9OMEJrUkt4bFVDdGNvNWZ0QjJqK2JtWWJsQ1NS?=
 =?utf-8?B?djlxNytJbjlTMk1iUGFGWnpGNEo5d2c3NitDVVJBR29za3lGU2FHZFdZekxh?=
 =?utf-8?B?VTZ2Q3JGL2VNc242ajJNTTdMazNLZHhDdWdPdHA2anpVWm52dTlZcTlNQ2sz?=
 =?utf-8?B?UFY0T240S012Tko3bVFVNWszTFlyVmdNOHBWbzc3UXBFUUFLUStoWHdidG92?=
 =?utf-8?B?QWhtYlIrU0loMTFIS0FVUUxrVVpwZmoxYnNiZzQrYkVFZ3BsL2duRDQ3VmEy?=
 =?utf-8?B?d3Nnak9BNDh5Z1JteUwzeWs2NVo0K3EyN1BUS2VsRUhZOVNGc3ZON0pxVVlh?=
 =?utf-8?B?UXFDaVU4M3Y0WkR4d0dhRzJaTTBNVUVVak9CWTFZRTVta2dPZHoyZFBBVmpq?=
 =?utf-8?B?ZzZwZlJkU1ZUS01pVmRhL1pKOFpielJhZXZ4QW5CZXVMQ1VhVW9DRDhLbExu?=
 =?utf-8?B?UXlzWFNOWmJQVG0rNFU1cFR6ZTBIRXhsWkZVVG5jMHNIdXBjNU9FTFVmbll0?=
 =?utf-8?B?eUp4c2c5OTV3PT0=?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c78ace83-4f9c-4e1d-4de1-08da0ade1f8c
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4468.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Mar 2022 01:57:15.5589
 (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: AFLk4Dx3S6R2GlZYNF0NHulFjjUhrcGIlaONaqThj2xHDBuxyHugh/8d35Di+rc04cnLNAUj8PerAh4u6nlwmw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR10MB1598
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10292 signatures=694221
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 phishscore=0
 mlxlogscore=999 adultscore=0 suspectscore=0 malwarescore=0 spamscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2203210009
X-Proofpoint-GUID: oImxH-JB4dUockDjBveyYLnj3Lin4G-7
X-Proofpoint-ORIG-GUID: oImxH-JB4dUockDjBveyYLnj3Lin4G-7
X-Spam-Status: No, score=-3.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 again for reviewing this.

On 18/3/22 11:07 am, Al Viro wrote:
> On Thu, Mar 17, 2022 at 06:26:11PM +1100, Imran Khan wrote:
> 
>> diff --git a/fs/kernfs/symlink.c b/fs/kernfs/symlink.c
>> index 9d4103602554..cbdd1be5f0a8 100644
>> --- a/fs/kernfs/symlink.c
>> +++ b/fs/kernfs/symlink.c
>> @@ -113,12 +113,19 @@ static int kernfs_getlink(struct inode *inode, char *path)
>>  	struct kernfs_node *kn = inode->i_private;
>>  	struct kernfs_node *parent = kn->parent;
>>  	struct kernfs_node *target = kn->symlink.target_kn;
>> -	struct rw_semaphore *rwsem;
>> +	struct kernfs_rwsem_token token;
>>  	int error;
>>  
>> -	rwsem = kernfs_down_read(parent);
>> +	/**
>> +	 * Lock both parent and target, to avoid their movement
>> +	 * or removal in the middle of path construction.
>> +	 * If a competing remove or rename for parent or target
>> +	 * wins, it will be reflected in result returned from
>> +	 * kernfs_get_target_path.
>> +	 */
>> +	kernfs_down_read_double_nodes(target, parent, &token);
>>  	error = kernfs_get_target_path(parent, target, path);
>> -	kernfs_up_read(rwsem);
>> +	kernfs_up_read_double_nodes(target, parent, &token);
>>  
>>  	return error;
>>  }
> 
> No.  Read through the kernfs_get_target_path().  Why would locking these
> two specific nodes be sufficient for anything useful?  That code relies
> upon ->parent of *many* nodes being stable.  Which is not going to be
> guaranteed by anything of that sort.
> 
> And it's not just "we might get garbage if we race" - it's "we might
> walk into kfree'd object and proceed to walk the pointer chain".
> 
> Or have this loop
> 	kn = target;
> 	while (kn->parent && kn != base) {
> 		len += strlen(kn->name) + 1;
> 		kn = kn->parent;
> 	}
> see the names that are not identical to what we see in
> 	kn = target;
> 	while (kn->parent && kn != base) {
> 		int slen = strlen(kn->name);
> 
> 		len -= slen;
> 		memcpy(s + len, kn->name, slen);
> 		if (len)
> 			s[--len] = '/';
> 
> 		kn = kn->parent;
> 	}
> done later in the same function.  With obvious unpleasant effects.
> Or a different set of nodes, for that matter.
> 
> This code really depends upon the tree being stable.  No renames of
> any sort allowed during that thing.

Yes. My earlier approach is wrong.

This patch set has also introduced a per-fs mutex (kernfs_rm_mutex)
which should fix the problem of inconsistent tree view as far as
kernfs_get_path is concerned.
Acquiring kernfs_rm_mutex before invoking kernfs_get_path in
kernfs_getlink will ensure that kernfs_get_path will get a consistent
view of ->parent of nodes from root to target. This is because acquiring
kernfs_rm_mutex will ensure that __kernfs_remove does not remove any
kernfs_node(or parent of kernfs_node). Further it ensures that
kernfs_rename_ns does not move any kernfs_node. So far I have not used
per-fs mutex in kernfs_rename_ns but I can make this change in next
version. So following change on top of current patch set should fix
this issue of ->parent change in the middle of kernfs_get_path.


diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 1b28d99ff1c3..8095dcdd437c 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -1672,11 +1672,13 @@ int kernfs_rename_ns(struct kernfs_node *kn,
struct kernfs_node *new_parent,
        const char *old_name = NULL;
        struct kernfs_rwsem_token token;
        int error;
+       struct kernfs_root *root = kernfs_root(kn);

        /* can't move or rename root */
        if (!kn->parent)
                return -EINVAL;

+       mutex_lock(&root->kernfs_rm_mutex);
        old_parent = kn->parent;
        kernfs_get(old_parent);
        kernfs_down_write_triple_nodes(kn, old_parent, new_parent, &token);
@@ -1741,6 +1743,7 @@ int kernfs_rename_ns(struct kernfs_node *kn,
struct kernfs_node *new_parent,
        error = 0;
  out:
        kernfs_up_write_triple_nodes(kn, new_parent, old_parent, &token);
+       mutex_unlock(&root->kernfs_rm_mutex);
        return error;
 }

diff --git a/fs/kernfs/symlink.c b/fs/kernfs/symlink.c
index cbdd1be5f0a8..805543d7a1f2 100644
--- a/fs/kernfs/symlink.c
+++ b/fs/kernfs/symlink.c
@@ -113,19 +113,22 @@ static int kernfs_getlink(struct inode *inode,
char *path)
        struct kernfs_node *kn = inode->i_private;
        struct kernfs_node *parent = kn->parent;
        struct kernfs_node *target = kn->symlink.target_kn;
-       struct kernfs_rwsem_token token;
+       struct kernfs_root *root;
        int error;

+       root = kernfs_root(kn);
+
        /**
-        * Lock both parent and target, to avoid their movement
-        * or removal in the middle of path construction.
-        * If a competing remove or rename for parent or target
-        * wins, it will be reflected in result returned from
-        * kernfs_get_target_path.
+        * Acquire kernfs_rm_mutex to ensure that kernfs_get_path
+        * sees correct ->parent for all nodes.
+        * We need to make sure that during kernfs_get_path parent
+        * of any node from target to root does not change. Acquiring
+        * kernfs_rm_mutex ensure that there are no concurrent remove
+        * or rename operations.
         */
-       kernfs_down_read_double_nodes(target, parent, &token);
+       mutex_lock(&root->kernfs_rm_mutex);
        error = kernfs_get_target_path(parent, target, path);
-       kernfs_up_read_double_nodes(target, parent, &token);
+       mutex_unlock(&root->kernfs_rm_mutex);

        return error;
 }

Could you please let me know if you see some issues with this approach ?

Thanks
-- Imran