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

[Khalid Aziz]

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