Re: [PATCH v2 3/9] mm/mshare: make msharefs writable and support directories

Khalid Aziz
2022-07-02T00:23:11Z

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