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: <a9fb9c9c-32d5-1b8a-f4f0-f97394b6f621@oracle.com>
Date:   Fri, 1 Jul 2022 09:58:13 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v2 7/9] mm/mshare: Add unlink and munmap support
Content-Language: en-US
To:     "Darrick J. Wong" <djwong@kernel.org>
Cc:     akpm@linux-foundation.org, willy@infradead.org,
        aneesh.kumar@linux.ibm.com, arnd@arndb.de, 21cnbao@gmail.com,
        corbet@lwn.net, dave.hansen@linux.intel.com, david@redhat.com,
        ebiederm@xmission.com, hagen@jauu.net, jack@suse.cz,
        keescook@chromium.org, kirill@shutemov.name, kucharsk@gmail.com,
        linkinjeon@kernel.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        longpeng2@huawei.com, luto@kernel.org, markhemm@googlemail.com,
        pcc@google.com, rppt@kernel.org, sieberf@amazon.com,
        sjpark@amazon.de, surenb@google.com, tst@schoebel-theuer.de,
        yzaikin@google.com
References: <cover.1656531090.git.khalid.aziz@oracle.com>
 <1b0a0e8c9558766f13a64ae93092eb8c9ea7965d.1656531090.git.khalid.aziz@oracle.com>
 <Yr4amM9d6HpwH5BW@magnolia>
From:   Khalid Aziz <khalid.aziz@oracle.com>
In-Reply-To: <Yr4amM9d6HpwH5BW@magnolia>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NjBDMS9vNHBGQXN3WUJrYlFOOHgrci9UL1pvUXBLS2p4Um41TTRPdXdQeXZG?=
 =?utf-8?B?Y2J0c2VsK2NlcEVjV3JnRTNzV1kxaGlXZXMyMEU3czc5Z1IwY3RuazlWeThP?=
 =?utf-8?B?SFV2aWdNUjJ1THRKbVpZbXNLOHlmNkZyZWljL1BTQzZ0K3V0QmU5UW96SzE2?=
 =?utf-8?B?aWVLczBpZVFxUkFRdVJodWpaTkcreUFuSXNXYmhMK2kyWXplcUpXVmcwbGtD?=
 =?utf-8?B?cXpjd3JHMFlJLzlMblhtS2JUeVo4VDl3bll6RzNxNzF3MTJJN1ZXaHdiR1ho?=
 =?utf-8?B?a0hOUGh3Nm55bnR4SVE1aXlWU1l1MnRLNVA2Tkh2M2NtUjhjWVlid080RDdM?=
 =?utf-8?B?ZUgrNU1oajU1eEJSL3lwWGpJMFVHNVVmTURkS0xZTENqemd1cXZOV2twV3h4?=
 =?utf-8?B?VzhQK2tWQUk3OWJvNHNRSmlyeGJrT2RpTE1URnhLOXdNejFIL1VrNWtuOXRG?=
 =?utf-8?B?MkYxeUpFTGViUWx1c3puSXMvdUVTajBHS3BZT0ZjSzJCcmROZWQ0cEp3MnB1?=
 =?utf-8?B?bUlTUFFRK3pnSTQzTk4zQXBZbzVYTU1UOTdaTVBCQ3ZHWXE3YWFpak82ajRV?=
 =?utf-8?B?WHZXTE5QS2x1RHhHZlhlVUc5cGRyb1hzSmI1WTBaZS83a01GVFlHVXFhNW5u?=
 =?utf-8?B?UWU3ODQwdlZ6bkNqMVN1VmxuMHNkWWJoekMyYjlQenpFSys5NXJiSzdBYkpZ?=
 =?utf-8?B?ZDloa0VLQjJWQTNhL25JbHdMMy8zc1BlaHc4dkR3TEQzRWc4QllTZ2w0NUdR?=
 =?utf-8?B?RjRHV3FEaUdQdTdaK0Y3eVE4ellVNHpzUFVhOEs4TXdxUWtzL3VVWUNsZE9N?=
 =?utf-8?B?UVRETk5xT3lzUlJsU2pZSlEyd2Zva29JdkI0RW4xMGtCMEFmaElSZFRtN1RL?=
 =?utf-8?B?R3M2SjFxS1FHNUo5TnJuNzl5ZWxPRkh4UWZqYXdoQ1ZzWVFyZWdkZHRsR1Mr?=
 =?utf-8?B?MFBuYytYcW9aNDErbGlGelJ5ZFQ2SGU2bEVNaWZkd2NyZ2ozVldJQ245VlBk?=
 =?utf-8?B?M0hhdFJEaW1HNTVUNi9XOEZyTjBYMVUxeUdlb3QwYUs2UWVPM2krZkxoRmNK?=
 =?utf-8?B?eTFyRWxXaTBTcUpydjlMcFRmYXpTMWZuMk93ZnEwWEFYMkEvS2ZieWVpaWd4?=
 =?utf-8?B?K3I2RXZENUEvanVUSVlod21KVUVFcHE1SDcwZWZ3RWtkRW9HZldnMU1KblFh?=
 =?utf-8?B?K1FiYll1YnhHVVNvVzlJVnltVEJiTnF3TTM5My9PTG5ZMFdESmd2ZEVzc0hL?=
 =?utf-8?B?dFZrcGtUczA1SGxCV0NKdDlQcUJyNDV4bmlhT0FYYjgvaWs5Zk5hRytnMlMx?=
 =?utf-8?B?dmV4bThQTDVoQTVYSDhZTmszZWcvcHZISUozRlN2RzlTb0p4bGE1TndOQWIv?=
 =?utf-8?B?Q2lpOHdqZTg3bTdrdWNLMk9YUCs0N2NLZjRUM1E4a2ZVbDFudHZxam5MQUpu?=
 =?utf-8?B?R3hQaldLbk5JS0R1akcrY0Mya0xwWHNBbHN1U2kyU1FxRXpWT3lHVVRQTFZx?=
 =?utf-8?B?cDF2QzhndTFLK2grUWlVQXZYNU8xbVRHT05QTk9GU1ZPa250VmV1bi9RcUxR?=
 =?utf-8?B?akFGZUMyRmlucnhuQmxtZDV5TTJCNW9RSFFUOExkVUEya2FrUTZueWpseWE0?=
 =?utf-8?B?WEZ1and3NTFWbEZRSUVEMmd0L1ZzOVgvQzZHOXZobXlBTGlqa1dmTVExZ0tD?=
 =?utf-8?B?M0VQSmc3ZXlYRXJXd3prNEdSNHpXaHZVNzI5YVduNGdlWDVzTVAzQVYzWlBl?=
 =?utf-8?B?QnlqVGVITmVqOFhMVCtFdTdSck4vWUdtVlVUOTB2UUZqNkdIa3p3MUwyR3pj?=
 =?utf-8?B?N2cwRGpmbU53ZmR1eGIwd3NLcFFoSXcya3g0U05BR3BZWGkwTVlBdE5IS2F0?=
 =?utf-8?B?ZnJTRGl6R01BeWZkSmtKZTdMUk54Zmc4c0g2dFNZbnlwcGh6aWhMTnZLWFkz?=
 =?utf-8?B?YUdmL1NqdGxQeDRyaFIwMVRSMGppOVFQVUdCSjgyTWtRNEVpMWRnajdEdjZT?=
 =?utf-8?B?K0V2K2I5VTVlb3J6c2hZRHUxeHB6VitOczZza21PZHB0Mk5aYmZiclUybGlU?=
 =?utf-8?B?R3FYTG1uMGhYK29lNXZKNXFWNWVGYUVnWWFYSjE3RVFwaTJxVUhuOHAybTNC?=
 =?utf-8?Q?+VVbVyl5XbO49sX2gcG+zF4Q1?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c4c5a156-ad7b-4e6d-5960-08da5b7a84fb
X-MS-Exchange-CrossTenant-AuthSource: BN8PR10MB3220.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Jul 2022 15:58:20.4986
 (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: oaEUW1lv7dWV0b1W5R8L/iQJ5yJDtglGfiUFaozjupoxAPVkp69ZgNYDfz1ED2HuxdSmm4iDkZikMSHNaNB8cw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB4103
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.517,18.0.883
 definitions=2022-07-01_08:2022-06-28,2022-07-01 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 spamscore=0
 malwarescore=0 suspectscore=0 mlxlogscore=999 adultscore=0 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2204290000
 definitions=main-2207010063
X-Proofpoint-ORIG-GUID: grHj0HpwV9WKz1KhU3NC7gC8SGAtkSaS
X-Proofpoint-GUID: grHj0HpwV9WKz1KhU3NC7gC8SGAtkSaS
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_H2,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

On 6/30/22 15:50, Darrick J. Wong wrote:
> On Wed, Jun 29, 2022 at 04:53:58PM -0600, Khalid Aziz wrote:
>> Number of mappings of an mshare region should be tracked so it can
>> be removed when there are no more references to it and associated
>> file has been deleted. This add code to support the unlink operation
>> for associated file, remove the mshare region on file deletion if
>> refcount goes to zero, add munmap operation to maintain refcount
>> to mshare region and remove it on last munmap if file has been
>> deleted.
>>
>> Signed-off-by: Khalid Aziz <khalid.aziz@oracle.com>
>> ---
>>   mm/mshare.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/mshare.c b/mm/mshare.c
>> index 088a6cab1e93..90ce0564a138 100644
>> --- a/mm/mshare.c
>> +++ b/mm/mshare.c
>> @@ -29,6 +29,7 @@ static struct super_block *msharefs_sb;
>>   struct mshare_data {
>>   	struct mm_struct *mm;
>>   	refcount_t refcnt;
>> +	int deleted;
>>   	struct mshare_info *minfo;
>>   };
>>   
>> @@ -48,6 +49,7 @@ msharefs_read(struct kiocb *iocb, struct iov_iter *iov)
>>   	size_t ret;
>>   	struct mshare_info m_info;
>>   
>> +	mmap_read_lock(info->mm);
>>   	if (info->minfo != NULL) {
>>   		m_info.start = info->minfo->start;
>>   		m_info.size = info->minfo->size;
>> @@ -55,18 +57,42 @@ msharefs_read(struct kiocb *iocb, struct iov_iter *iov)
>>   		m_info.start = 0;
>>   		m_info.size = 0;
>>   	}
>> +	mmap_read_unlock(info->mm);
>>   	ret = copy_to_iter(&m_info, sizeof(m_info), iov);
>>   	if (!ret)
>>   		return -EFAULT;
>>   	return ret;
>>   }
>>   
>> +static void
>> +msharefs_close(struct vm_area_struct *vma)
>> +{
>> +	struct mshare_data *info = vma->vm_private_data;
>> +
>> +	if (refcount_dec_and_test(&info->refcnt)) {
>> +		mmap_read_lock(info->mm);
>> +		if (info->deleted) {
>> +			mmap_read_unlock(info->mm);
>> +			mmput(info->mm);
>> +			kfree(info->minfo);
>> +			kfree(info);
> 
> Aren't filesystems supposed to take care of disposing of the file data
> in destroy_inode?  IIRC struct inode doesn't go away until all fds are
> closed, mappings are torn down, and there are no more references from
> dentries.  I could be misremembering since it's been a few months since
> I went looking at the (VFS) inode lifecycle.

Documentation (vfs.rst) says - "this method is called by destroy_inode() to release resources allocated for struct 
inode. It is only required if ->alloc_inode was defined and simply undoes anything done by ->alloc_inode.". I am not 
defining alloc_inode, so I assumed I do not need to define destroy_inode and the standard destroy_inode will do the 
right thing since standard alloc_inode is being used.

Are you suggesting per-region mshare_data should be freed in destroy_inode instead of in close?

> 
>> +		} else {
>> +			mmap_read_unlock(info->mm);
>> +		}
>> +	}
>> +}
>> +
>> +static const struct vm_operations_struct msharefs_vm_ops = {
>> +	.close	= msharefs_close,
>> +};
>> +
>>   static int
>>   msharefs_mmap(struct file *file, struct vm_area_struct *vma)
>>   {
>>   	struct mshare_data *info = file->private_data;
>>   	struct mm_struct *mm = info->mm;
>>   
>> +	mmap_write_lock(mm);
>>   	/*
>>   	 * If this mshare region has been set up once already, bail out
>>   	 */
>> @@ -80,10 +106,14 @@ msharefs_mmap(struct file *file, struct vm_area_struct *vma)
>>   	mm->task_size = vma->vm_end - vma->vm_start;
>>   	if (!mm->task_size)
>>   		mm->task_size--;
>> +	mmap_write_unlock(mm);
>>   	info->minfo->start = mm->mmap_base;
>>   	info->minfo->size = mm->task_size;
>> +	info->deleted = 0;
>> +	refcount_inc(&info->refcnt);
>>   	vma->vm_flags |= VM_SHARED_PT;
>>   	vma->vm_private_data = info;
>> +	vma->vm_ops = &msharefs_vm_ops;
>>   	return 0;
>>   }
>>   
>> @@ -240,6 +270,38 @@ msharefs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>   	return ret;
>>   }
>>   
>> +static int
>> +msharefs_unlink(struct inode *dir, struct dentry *dentry)
>> +{
>> +	struct inode *inode = d_inode(dentry);
>> +	struct mshare_data *info = inode->i_private;
>> +
>> +	/*
>> +	 * Unmap the mshare region if it is still mapped in
>> +	 */
>> +	vm_munmap(info->minfo->start, info->minfo->size);
>> +
>> +	/*
>> +	 * Mark msharefs file for deletion so it can not be opened
>> +	 * and used for mshare mappings any more
>> +	 */
>> +	simple_unlink(dir, dentry);
>> +	mmap_write_lock(info->mm);
>> +	info->deleted = 1;
>> +	mmap_write_unlock(info->mm);
> 
> What if the file is hardlinked?

It looks like that is a bug currently. I need to account for that. Thanks!

--
Khalid