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: <4bbaa753-f145-4971-2b51-c909b946ae63@oracle.com>
Date:   Thu, 30 Jun 2022 16:27:01 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v2 4/9] mm/mshare: Add a read operation for msharefs files
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>
 <05649b455e2191642e85cc5522ef39ad49fdeca3.1656531090.git.khalid.aziz@oracle.com>
 <Yr4VVuCzCp50cu0O@magnolia>
From:   Khalid Aziz <khalid.aziz@oracle.com>
In-Reply-To: <Yr4VVuCzCp50cu0O@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?TzZoYnozclhXaUYrKzB5ak12aTU1c0ZXejJpYjNCQkt3YU5JUFZoT3hVVDNh?=
 =?utf-8?B?OGk2WExRR1gvVVRaWWlxeUNNZnR0MmY3cEk1S3hScHNoQ0w5UStEL2dQWE54?=
 =?utf-8?B?c01seUpkNTdienZ5N0RNU2RmbTlrZ2RWQlNldE5SSWVpbERoYTdFMTdldVNQ?=
 =?utf-8?B?ZXJpNU9jVnJ4MXBVd1h5OENaSkZEZE5qTnI4YmJQd0xjNDdmZmUzWWRvMzJ0?=
 =?utf-8?B?am1NeDV1KzlSZnFOV1ZtRWt5dlNmMlM4bE1KcnFTczM3MysrTGlUM09haWNm?=
 =?utf-8?B?THlsdkcwdGxaWGJaVm1tTm10dWtZKzNwYzNNOHVUaUpIMkw2ZExEaFBpUW5w?=
 =?utf-8?B?YXFwZjlnT1RrNktJSEppSGhOalZVMGJIaTVESWhTRi8vK0NSVUlPaU15czFH?=
 =?utf-8?B?QlZZaE1STDQxVEJ6aWZ5S255OWNueWhpb0IyaTh5NW9mOTZRME1hY2tVQmh1?=
 =?utf-8?B?Wm9NbjZ6WmhDNW5pS0pheDhESGNiZWlBUGIyNkE5Z1oyN3ZBSzAvQmtpeUx5?=
 =?utf-8?B?cUpNRXFNYjVPVHF0OXQvQmlGamlMSVQxblNkUHkxZnBlQjhmQjZkeVc2Y2hY?=
 =?utf-8?B?ZmR0c0RGRWo5WWFHaDQwd1lFNllsak1UZXdkU0Znb05ub2lSeHhTRndpdHFY?=
 =?utf-8?B?NEllNTBZbmR1dFhXazUybUFVMThaWjR2VlRWR1R4UCtDQVVERUQrWk9NbUJT?=
 =?utf-8?B?VytaT0FIWm5OV0tlbFZFSkdSZXUwK2szZDlNSngrQ3Zhd0dVL1Z4c0xENjZD?=
 =?utf-8?B?Q1g3TGZqQWlHVHBSZ2N4WGF5UnVYU2JaQkJiRVBjdWF6YzZCYVl4aU5LTUgz?=
 =?utf-8?B?Qm9MaE5VdWdLUUFCbER1b0UydVZjWnU5SVJ6K0xGNUVnQjdHWlI5RnhkVmM4?=
 =?utf-8?B?cExpM0FWcVdmQ1pnSjRwZkZ4djJtUDNDazBuNmkyYVNSTGJuVGhkVkNGTmtR?=
 =?utf-8?B?UXowSUdPQzd0QTlNR3RRVlU1eWQrWW8rMjMyS0tDYUFJNWcrYVFtdG9TMjJz?=
 =?utf-8?B?Y1Ivbzh2VUhGSjR6ZUI0VCswamFSMkVxMldOUlVNTm9mK05hYllqR2pZa3Jz?=
 =?utf-8?B?R2kwSkl0dkErMnJzWHdSSFBlWWlBajBJczk3dURiOFM3VlZ6Y0ZSa25XVFJa?=
 =?utf-8?B?S1Mzc3BoVHZIZWhhaEZnYUlhYWpGdlhITWJQYW1rNklSMkJGUDFwUlYyMWdC?=
 =?utf-8?B?WlVCeXRHckxsWGdscDdjYm5Pc0xva1F6VVIzc2FCelVZV0QyTHIxcDE3YmIv?=
 =?utf-8?B?UDJFLzhrR24xWnBYMCtMcS9HZ0NFMTBLMGFmU29nQmRHeXVSTXBJMHdLNWM1?=
 =?utf-8?B?N0d2YU8yOU5XdFh3Wi9vcXdTWkI5cGNQTFY4WTVWc252NnVURURMa3NDeE1F?=
 =?utf-8?B?WEtzSGxZYUJjUzlFYmZmNkJuNmtISUkzTDlucVhIUVhIYVF0V3laOTFmNlE2?=
 =?utf-8?B?TXQzQnVxeUpRYzZlb3kzaU9jUzZaNVFYRmo3Q1hLd0VzZXhtc2xHWDV6Wnlz?=
 =?utf-8?B?bmFEaTlFL2dEQjdRL2dsWlpmZWZTbS9kdm8rWUxUcEZ4Z09Xek44Ym5CRVNR?=
 =?utf-8?B?M0ovNmtGMUVZSGdOVG5oRGhCeW1MM25QOUdFK090M2FTMWg3RUY0ZjNNT3Fs?=
 =?utf-8?B?c0hsaldaRGVqczBZR1ZMUUtnSnpWTEJoLzdTUzRXS0pWTmh2SXdEZXVmYmZk?=
 =?utf-8?B?Y2p2YXc0R3NXMVU5QmxmejdqMTBwNFlnUHUrRUZaSHZQR1dKblpMMlo2Q1Jj?=
 =?utf-8?B?eXVVZGVmYkxZT1BlTWl5TUNFUm9jYWNkNktCNmFQa1NEMXpZSVFwN0dBakxr?=
 =?utf-8?B?bWM5Tzh3dUZtQTE4U2x1NVZZMWFweUcrKzlzN0pEZHAwNGlKTHJ5aVRyZTNn?=
 =?utf-8?B?Z0x0WFl6TDNScjBPOEJ2WCtSOENvZmMvUjR6dmgyOFhJMzJPT1E5eEJiUFZj?=
 =?utf-8?B?WnF4Q0tsODN1L0lyWmRKYXV1bkdnTkFGT3FEQUxuR0tDQ1VKbENBUEdCQ1hX?=
 =?utf-8?B?N1BQeDJCTVBvSHNtQWlXZnEyaEFwTnBNdnpSb1VCSU15QWRycmZja2dNL0lt?=
 =?utf-8?B?K0pwUTUrc2pwdjQwYU1wVUpHbllLb2w1bVYxa3ExZkNVcmhaQlRlbU8yYVRY?=
 =?utf-8?B?U0NtZW9BRTRSbDJpOE5RSGlpWDhIWUdPSFlqN1hhVENFTjJhOEM0YXpoZXdN?=
 =?utf-8?B?ek9uQW91MDhnZ1p2STVYNTBiaG9mZERaK2w4OGZhRjNHdXVvc1pwTkZRdWcx?=
 =?utf-8?B?czNRVFBSZDNjV2I5VkpvQUZBdktBPT0=?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0478f98c-f6cf-43c4-5594-08da5ae7a92f
X-MS-Exchange-CrossTenant-AuthSource: BN8PR10MB3220.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Jun 2022 22:27:05.0774
 (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: zuQaMCsmldBejNU6NMtJPLGE5IXbcIEhuryKbLzOKzHaDnSbYjLo1SCcFOgL9sigO7NDYZlSUJRDuQ8G8jU39g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR10MB3152
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.517,18.0.883
 definitions=2022-06-30_14:2022-06-28,2022-06-30 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-2206300086
X-Proofpoint-ORIG-GUID: 1zNyCvbwHxqdaBB13U_zbgN6aVScqtg9
X-Proofpoint-GUID: 1zNyCvbwHxqdaBB13U_zbgN6aVScqtg9
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:27, Darrick J. Wong wrote:
> On Wed, Jun 29, 2022 at 04:53:55PM -0600, Khalid Aziz wrote:
>> When a new file is created under msharefs, allocate a new mm_struct
>> that will hold the VMAs for mshare region. Also allocate structure
>> to defines the mshare region and add a read operation to the file
>> that returns this information about the mshare region. Currently
>> this information is returned as a struct:
>>
>> struct mshare_info {
>> 	unsigned long start;
>> 	unsigned long size;
>> };
>>
>> This gives the start address for mshare region and its size.
>>
>> Signed-off-by: Khalid Aziz <khalid.aziz@oracle.com>
>> ---
>>   include/uapi/linux/mman.h |  5 +++
>>   mm/mshare.c               | 64 ++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 68 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/mman.h b/include/uapi/linux/mman.h
>> index f55bc680b5b0..56fe446e24b1 100644
>> --- a/include/uapi/linux/mman.h
>> +++ b/include/uapi/linux/mman.h
>> @@ -41,4 +41,9 @@
>>   #define MAP_HUGE_2GB	HUGETLB_FLAG_ENCODE_2GB
>>   #define MAP_HUGE_16GB	HUGETLB_FLAG_ENCODE_16GB
>>   
>> +struct mshare_info {
>> +	unsigned long start;
>> +	unsigned long size;
> 
> You might want to make these explicitly u64, since this is userspace
> ABI and you never know when someone will want to do something crazy like
> run 32-bit programs with mshare files.
> 
> Also you might want to add some padding fields for flags, future
> expansion, etc.

That sounds like a good idea. I will queue it up for next version of patch series.

> 
>> +};
>> +
>>   #endif /* _UAPI_LINUX_MMAN_H */
>> diff --git a/mm/mshare.c b/mm/mshare.c
>> index 2d5924d39221..d238b68b0576 100644
>> --- a/mm/mshare.c
>> +++ b/mm/mshare.c
>> @@ -22,8 +22,14 @@
>>   #include <uapi/linux/magic.h>
>>   #include <uapi/linux/limits.h>
>>   #include <uapi/linux/mman.h>
>> +#include <linux/sched/mm.h>
>>   
>>   static struct super_block *msharefs_sb;
>> +struct mshare_data {
>> +	struct mm_struct *mm;
>> +	refcount_t refcnt;
>> +	struct mshare_info *minfo;
>> +};
>>   
>>   static const struct inode_operations msharefs_dir_inode_ops;
>>   static const struct inode_operations msharefs_file_inode_ops;
>> @@ -34,8 +40,29 @@ msharefs_open(struct inode *inode, struct file *file)
>>   	return simple_open(inode, file);
>>   }
>>   
>> +static ssize_t
>> +msharefs_read(struct kiocb *iocb, struct iov_iter *iov)
>> +{
>> +	struct mshare_data *info = iocb->ki_filp->private_data;
>> +	size_t ret;
>> +	struct mshare_info m_info;
>> +
>> +	if (info->minfo != NULL) {
>> +		m_info.start = info->minfo->start;
>> +		m_info.size = info->minfo->size;
>> +	} else {
>> +		m_info.start = 0;
>> +		m_info.size = 0;
> 
> Hmmm, read()ing out the shared mapping information.  Heh.
> 
> When does this case happen?  Is it before anybody mmaps this file into
> an address space?
> 

It can happen before or after the first mmap which will establish the start address and size. Hence I have to account 
for both cases.

>> +	}
>> +	ret = copy_to_iter(&m_info, sizeof(m_info), iov);
>> +	if (!ret)
>> +		return -EFAULT;
>> +	return ret;
>> +}
>> +
>>   static const struct file_operations msharefs_file_operations = {
>>   	.open		= msharefs_open,
>> +	.read_iter	= msharefs_read,
>>   	.llseek		= no_llseek,
>>   };
>>   
>> @@ -73,12 +100,43 @@ static struct dentry
>>   	return ERR_PTR(-ENOMEM);
>>   }
>>   
>> +static int
>> +msharefs_fill_mm(struct inode *inode)
>> +{
>> +	struct mm_struct *mm;
>> +	struct mshare_data *info = NULL;
>> +	int retval = 0;
>> +
>> +	mm = mm_alloc();
>> +	if (!mm) {
>> +		retval = -ENOMEM;
>> +		goto err_free;
>> +	}
>> +
>> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
>> +	if (!info) {
>> +		retval = -ENOMEM;
>> +		goto err_free;
>> +	}
>> +	info->mm = mm;
>> +	info->minfo = NULL;
>> +	refcount_set(&info->refcnt, 1);
>> +	inode->i_private = info;
>> +
>> +	return 0;
>> +
>> +err_free:
>> +	if (mm)
>> +		mmput(mm);
>> +	kfree(info);
>> +	return retval;
>> +}
>> +
>>   static struct inode
>>   *msharefs_get_inode(struct super_block *sb, const struct inode *dir,
>>   			umode_t mode)
>>   {
>>   	struct inode *inode = new_inode(sb);
>> -
>>   	if (inode) {
>>   		inode->i_ino = get_next_ino();
>>   		inode_init_owner(&init_user_ns, inode, dir, mode);
>> @@ -89,6 +147,10 @@ static struct inode
>>   		case S_IFREG:
>>   			inode->i_op = &msharefs_file_inode_ops;
>>   			inode->i_fop = &msharefs_file_operations;
>> +			if (msharefs_fill_mm(inode) != 0) {
>> +				discard_new_inode(inode);
>> +				inode = ERR_PTR(-ENOMEM);
> 
> Is it intentional to clobber the msharefs_fill_mm return value and
> replace it with ENOMEM?

ENOMEM sounded like the right value to return from msharefs_get_inode() in case of failure. On the other hand, there 
isn't much of a reason to not just return the return value from msharefs_fill_mm(). I can change that.

Thanks for the review.

--
Khalid