Messages in this thread | | | Date | Thu, 30 Jun 2022 16:49:12 -0600 | Subject | Re: [PATCH v2 3/9] mm/mshare: make msharefs writable and support directories | From | Khalid Aziz <> |
| |
On 6/30/22 15:34, Darrick J. Wong wrote: > On Wed, Jun 29, 2022 at 04:53:54PM -0600, Khalid Aziz wrote: >> Make msharefs filesystem writable and allow creating directories >> to support better access control to mshare'd regions defined in >> msharefs. >> >> Signed-off-by: Khalid Aziz <khalid.aziz@oracle.com> >> --- >> mm/mshare.c | 195 +++++++++++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 186 insertions(+), 9 deletions(-) >> >> diff --git a/mm/mshare.c b/mm/mshare.c >> index 3e448e11c742..2d5924d39221 100644 >> --- a/mm/mshare.c >> +++ b/mm/mshare.c >> @@ -21,11 +21,21 @@ >> #include <linux/fileattr.h> >> #include <uapi/linux/magic.h> >> #include <uapi/linux/limits.h> >> +#include <uapi/linux/mman.h> >> >> static struct super_block *msharefs_sb; >> >> +static const struct inode_operations msharefs_dir_inode_ops; >> +static const struct inode_operations msharefs_file_inode_ops; >> + >> +static int >> +msharefs_open(struct inode *inode, struct file *file) >> +{ >> + return simple_open(inode, file); >> +} >> + >> static const struct file_operations msharefs_file_operations = { >> - .open = simple_open, >> + .open = msharefs_open, >> .llseek = no_llseek, >> }; >> >> @@ -42,6 +52,113 @@ msharefs_d_hash(const struct dentry *dentry, struct qstr *qstr) >> return 0; >> } >> >> +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); >> +} >> + >> +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) { > > Not sure why you wouldn't go with the less-indently version: > > if (!inode) > return ERR_PTR(-ENOMEM); > > inode->i_ino = get_next_ino(); > <etc> >
Yeah, good idea. I will change it.
>> + inode->i_ino = get_next_ino(); >> + inode_init_owner(&init_user_ns, inode, dir, mode); >> + >> + inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode); >> + >> + switch (mode & S_IFMT) { > > Shouldn't we set the mode somewhere?
mode is passed in as parameter to msharefs_get_inode() which uses this value to determine its actions.
> >> + case S_IFREG: >> + inode->i_op = &msharefs_file_inode_ops; >> + inode->i_fop = &msharefs_file_operations; >> + break; >> + case S_IFDIR: >> + inode->i_op = &msharefs_dir_inode_ops; >> + inode->i_fop = &simple_dir_operations; >> + inc_nlink(inode); >> + break; >> + case S_IFLNK: >> + inode->i_op = &page_symlink_inode_operations; >> + break; >> + default: >> + discard_new_inode(inode); >> + inode = NULL; >> + break; >> + } >> + } >> + >> + return inode; >> +} >> + >> +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); > > ...and if @inode is NULL?
Oh right, IS_ERR() does not check for NULL value. I will add a check for that and return ENOMEM.
> >> + >> + d_instantiate(dentry, inode); >> + dget(dentry); >> + dir->i_mtime = dir->i_ctime = current_time(dir); >> + >> + return err; >> +} >> + >> +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); > > Changing this already?
Possibly. There is one suggestion to change it to PMD and it might be a better choice.
> >> 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; > > What ends the array? NULL name or empty name? > Do we have to erase all of these when the fs gets unmounted?
This code is very similar to simple_fill_super() and I reused the code from there. inodes and dentries will need to be erased on unmount through evict_inode.
Thanks, Khalid
| |