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: <bd424825-5d19-a569-d267-0f9dcf382358@oracle.com>
Date:   Fri, 1 Jul 2022 18:22:09 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v2 3/9] mm/mshare: make msharefs writable and support
 directories
Content-Language: en-US
To:     Al Viro <viro@zeniv.linux.org.uk>
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>
 <397ad80630444b90877625a1e94dd81392fc678e.1656531090.git.khalid.aziz@oracle.com>
 <Yr4tM2oOF9rlwWdV@ZenIV>
From:   Khalid Aziz <khalid.aziz@oracle.com>
In-Reply-To: <Yr4tM2oOF9rlwWdV@ZenIV>
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?RFcyZldEWFhiY1NqL2F4QTVXTVFOT1l1M0RMUWtuMGVCVy94eW5sYXA2V3Yx?=
 =?utf-8?B?NzN6clg1SzJ5Vzh2dUt1UDNrTWJoaVkxbC9RNEhZb2xkNjBwL1ZZemdRQlZB?=
 =?utf-8?B?SnRuUFJCQnUrOE5CUnAvd1oyekN0THNIbG1oVUZjdTIwaVgrblZicWhWZmds?=
 =?utf-8?B?RzRpRVlXckh2dElDSE93bTNxdW1QR1gwaGFRWnZIbWh2T0x2TS9hMUl3KzFp?=
 =?utf-8?B?SmNNeGxabFZEV2VCcUNYdGtGT1MxQ1VEWnNNazFOM0NNbnZDM2hiVENDV3hZ?=
 =?utf-8?B?V0c5WDd2Qkd5V09oSXpIbEI0MSttMUVHc1ZXb1kzU1ducjdCV3ErdGV6UEZM?=
 =?utf-8?B?TFFvRHhva28yNEtBbnpBNG1XME8wdUZ2MU1QNzBNbUI2cFZXaWhrMmtqYzNY?=
 =?utf-8?B?WnljV0hVSGlIdEVzYW1RNmVDR2dGK1dsN21UYVlmb3JqM1JDSzJ2cEZCUCsr?=
 =?utf-8?B?VmF3WXUxSUNvQWY4UmorL3FzNmNHRXFsQytnc09rMFZqUXZZY0dDaXBpazNM?=
 =?utf-8?B?UEdrZWFiT01zWVl6ZzFmKzhOVFB5QTZYMytXWmhTREV2Mmp3ZlBJMHlneFpC?=
 =?utf-8?B?R2NhUGxaNy8rYmJRWHJsOFg4MEdOL1Qvdm9qa00wZktDNmtZZ1kxdWF2QUxN?=
 =?utf-8?B?YXVCd1d1VmxyVnIzMy9OaXZTT09aZW8reHhzVzRTTzNhUy9nNkdDS3lvU1pl?=
 =?utf-8?B?MjhoblBZRi82LzdhbHlBUkJZN01CVlh2MmR1YzNqRjZkV1ZUbFZEemxBV1Qv?=
 =?utf-8?B?UjFDNGkwSHBKbTZPUEYvaWR0M3RBVDBFSEJ6ZllBZXkwVnd6MTIrQ3c2a20w?=
 =?utf-8?B?RjhIUkdyK3FsaFczQlMxc3VaTXB4R1I2M2EvTDdCR3RNQ1VMRFdRTEM5NXRK?=
 =?utf-8?B?WUJMZGU3K0EzZGRDWjVFSktqU2hWYVd5ekNYMVRWdGdyV21SaVY0Si9BZnVX?=
 =?utf-8?B?Z0FBeXR3MHk5VzNhUk9pZXdRbzdwR1kyMTFxWFpZaFJSUFNRb3RDR3pkQjN1?=
 =?utf-8?B?LzlhNVlLTXQ2M29yZSs0RVpNczRJL1ExUFFEUmZnUXFMOVZWc1hSZ0VrTXhP?=
 =?utf-8?B?K1FSODZ2cVN5NDRXYXVXbU1JTnJwQWpPUm10UlpQNHhiN2JZSGtDcXovVXFu?=
 =?utf-8?B?M0hRU3BXSUdhNnlraDJGU2grRnR4VlFtSFliRDdCRUkraHRjUkFiZmUyYkFu?=
 =?utf-8?B?VkRXK0pjSWdGNk00WXZUa0ZQWlZWMzRoNEYyR3FqNkV3MStsVS85SnFSenls?=
 =?utf-8?B?cmQ5ZVRjTllPWWRMTVBOdmI5TVB5UlJRMFFpSXo0Y0d5OCtLV3BJUVlDa2ZI?=
 =?utf-8?B?N0RvWkx0WDRWZ1Y2UTRLZlI4MmFsOVg3NE04cjVIYTdEbm01L2JNZ0txaFBo?=
 =?utf-8?B?S0RUSllSU0ZkdThUZmRuOFhpZlUyZDZybmQyRmcwYkM0R0Z4dGlNcDdNWUVX?=
 =?utf-8?B?VmtiTTJKaTV2LytqWEE2bzJEY2VuaVpDdjNWUW9nTlZoVGx5akdwRWhWVHV0?=
 =?utf-8?B?dDBITzlxcEZnMkxmWTM3YUswWmF2WW1FcWUwSlR5MFdTVVNTUExyd0VWbzI2?=
 =?utf-8?B?endHako3TkpjRWp4ZFBRYUVPNGhPamhFdmxiNXFoNnNETGloSXM5a3dZaGpW?=
 =?utf-8?B?RXpYS1FNNURoZHF0cWt6MjZRZmRNY1dHaHdhRlZxcXYxeUx5cXh6Wi8xUGFw?=
 =?utf-8?B?a2lWcmczMHlYZmdXWXE2T0psR1hWVndpWGt6RHVUdkpXd1g3V1dKK25INThL?=
 =?utf-8?B?Y1JtbUwwZUhMMmZ0d1JQczhYNXhwbTZuTytxYzB0Snd2MjAwYm05cmdmRUl1?=
 =?utf-8?B?MWdDWUdpWGVJcWp2MkFRWjRGcldrUTI5NXdTNjR6ZVZHcS9hbHR3cktFcEdT?=
 =?utf-8?B?dTNsQ2diNTNIcmZIMTcxcHp0Z0F6L3RiNFU0WnRoYXFaMG5YV2FXbnIwQTRh?=
 =?utf-8?B?Y0cydzYxOG9IeFcxNnB4dy9mNTJpRjhCVjBPRkkrK3JYdUg4YWRURnhKZHZ2?=
 =?utf-8?B?aEtUZWEwWVVMZ2VzeUZxb0cyU2NTL0V1RGNLNjdCR0c0My9tWGVIbkZ2QlN4?=
 =?utf-8?B?R1BnY2F4WmZnalBEN3ZQMVhlU1pzdUJwTUh6UDd4RnIzbmNOSXY0SmEyR0Jp?=
 =?utf-8?Q?OrjZovESs0D1ezzhJTLhJaYIv?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f3a0ef8e-b95c-44b4-ff0d-08da5bc0e177
X-MS-Exchange-CrossTenant-AuthSource: MN2PR10MB3231.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Jul 2022 00:22:00.3119
 (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: AlqUhntdPOFUzwJ1Q70dfI+n9bf32trwfpMJGnfw4OsG0uoHo+1YaCEsL9MjKQw2rkdFWp9oQ2beRvjzf6MKPA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR10MB1256
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.517,18.0.883
 definitions=2022-07-01_16: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=900 adultscore=0 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2204290000
 definitions=main-2207010097
X-Proofpoint-ORIG-GUID: E9ewm6ewQgjosepNgORjy82PQUhPqX20
X-Proofpoint-GUID: E9ewm6ewQgjosepNgORjy82PQUhPqX20
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 17:09, Al Viro wrote:
> On Wed, Jun 29, 2022 at 04:53:54PM -0600, Khalid Aziz wrote:
> 
>> +static int
>> +msharefs_open(struct inode *inode, struct file *file)
>> +{
>> +	return simple_open(inode, file);
>> +}
> 
> Again, whatever for? >
>> +static struct dentry
>> +*msharefs_alloc_dentry(struct dentry *parent, const char *name)
>> +{
>> +	struct dentry *d;
>> +	struct qstr q;
>> +	int err;
>> +
>> +	q.name = name;
>> +	q.len = strlen(name);
>> +
>> +	err = msharefs_d_hash(parent, &q);
>> +	if (err)
>> +		return ERR_PTR(err);
>> +
>> +	d = d_alloc(parent, &q);
>> +	if (d)
>> +		return d;
>> +
>> +	return ERR_PTR(-ENOMEM);
>> +}
> 
> And it's different from d_alloc_name() how, exactly?

By making minor changes to my other code, I was able to use all of the standard functions you pointed out. That 
simplified my patch quite a bit. Thank you!

> 
>> +		case S_IFLNK:
>> +			inode->i_op = &page_symlink_inode_operations;
>> +			break;
> 
> Really?  You've got symlinks here?

I intended to support symlinks on msharefs but I am not sure if I see a use case at this time. I can drop support for 
symlinks and add it in future if there is a use case.

> 
>> +		default:
>> +			discard_new_inode(inode);
>> +			inode = NULL;
> 
> That's an odd way to spell BUG()...

I think what you are saying is this default case represents a bug and I should report it as such. Is that right, or 
should I not have a default case at all (which is what I am seeing in some of the other places)?

> 
>> +static int
>> +msharefs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
>> +		struct dentry *dentry, umode_t mode, dev_t dev)
>> +{
>> +	struct inode *inode;
>> +	int err = 0;
>> +
>> +	inode = msharefs_get_inode(dir->i_sb, dir, mode);
>> +	if (IS_ERR(inode))
>> +		return PTR_ERR(inode);
>> +
>> +	d_instantiate(dentry, inode);
>> +	dget(dentry);
>> +	dir->i_mtime = dir->i_ctime = current_time(dir);
>> +
>> +	return err;
>> +}
> 
> BTW, what's the point of having device nodes on that thing?

There will be no device nodes on msharefs. Are you referring to the dev_t parameter in msharefs_mknod() declaration? If 
so, I am following the prototype declaration for that function from fs.h:

         int (*mknod) (struct user_namespace *, struct inode *,struct dentry *,
                       umode_t,dev_t);

If I am misunderstanding, please correct me.

> 
>> +static int
>> +msharefs_create(struct user_namespace *mnt_userns, struct inode *dir,
>> +		struct dentry *dentry, umode_t mode, bool excl)
>> +{
>> +	return msharefs_mknod(&init_user_ns, dir, dentry, mode | S_IFREG, 0);
>> +}
>> +
>> +static int
>> +msharefs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>> +		struct dentry *dentry, umode_t mode)
>> +{
>> +	int ret = msharefs_mknod(&init_user_ns, dir, dentry, mode | S_IFDIR, 0);
>> +
>> +	if (!ret)
>> +		inc_nlink(dir);
>> +	return ret;
>> +}
>> +
>> +static const struct inode_operations msharefs_file_inode_ops = {
>> +	.setattr	= simple_setattr,
>> +	.getattr	= simple_getattr,
>> +};
>> +static const struct inode_operations msharefs_dir_inode_ops = {
>> +	.create		= msharefs_create,
>> +	.lookup		= simple_lookup,
>> +	.link		= simple_link,
>> +	.unlink		= simple_unlink,
>> +	.mkdir		= msharefs_mkdir,
>> +	.rmdir		= simple_rmdir,
>> +	.mknod		= msharefs_mknod,
>> +	.rename		= simple_rename,
>> +};
>> +
>>   static void
>>   mshare_evict_inode(struct inode *inode)
>>   {
>> @@ -58,7 +175,7 @@ mshare_info_read(struct file *file, char __user *buf, size_t nbytes,
>>   {
>>   	char s[80];
>>   
>> -	sprintf(s, "%ld", PGDIR_SIZE);
>> +	sprintf(s, "%ld\n", PGDIR_SIZE);
>>   	return simple_read_from_buffer(buf, nbytes, ppos, s, strlen(s));
>>   }
>>   
>> @@ -72,6 +189,38 @@ static const struct super_operations mshare_s_ops = {
>>   	.evict_inode = mshare_evict_inode,
>>   };
>>   
>> +static int
>> +prepopulate_files(struct super_block *s, struct inode *dir,
>> +			struct dentry *root, const struct tree_descr *files)
>> +{
>> +	int i;
>> +	struct inode *inode;
>> +	struct dentry *dentry;
>> +
>> +	for (i = 0; !files->name || files->name[0]; i++, files++) {
>> +		if (!files->name)
>> +			continue;
>> +
>> +		dentry = msharefs_alloc_dentry(root, files->name);
>> +		if (!dentry)
>> +			return -ENOMEM;
>> +
>> +		inode = msharefs_get_inode(s, dir, S_IFREG | files->mode);
>> +		if (!inode) {
>> +			dput(dentry);
>> +			return -ENOMEM;
>> +		}
>> +		inode->i_mode = S_IFREG | files->mode;
>> +		inode->i_atime = inode->i_mtime = inode->i_ctime
>> +			= current_time(inode);
>> +		inode->i_fop = files->ops;
>> +		inode->i_ino = i;
>> +		d_add(dentry, inode);
>> +	}
>> +
>> +	return 0;
>> +}
> 
> Looks remarkably similar to something I've seen somewhere... fs/libfs.c,
> if I'm not mistaken...
> 
> Sarcasm aside, what's wrong with using simple_fill_super()?
I started out using simple_fill_super() in patch 1. I found that when I use simple_fill_super(), I end up with a 
filesystem that userspace can not create a file in. I looked at other code like shmfs and efivarfs and wrote similar 
code which got me a writable filesystem. I might be missing something basic and if there is a way to use 
simple_fill_super() and be able to support file creation from userspace, I would much rather use simple_fill_super().

Thanks,
Khalid