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

Al Viro
2022-06-30T23:10:41Z

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?

> + case S_IFLNK:
> + inode->i_op = &page_symlink_inode_operations;
> + break;

Really? You've got symlinks here?

> + default:
> + discard_new_inode(inode);
> + inode = NULL;

That's an odd way to spell BUG()...

> +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?

> +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()?