| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 30 Jun 2022 16:49:12 -0600
From: Khalid Aziz <khalid.aziz@...cle.com>
To: "Darrick J. Wong" <djwong@...nel.org>
Cc: akpm@...ux-foundation.org, willy@...radead.org,
aneesh.kumar@...ux.ibm.com, arnd@...db.de, 21cnbao@...il.com,
corbet@....net, dave.hansen@...ux.intel.com, david@...hat.com,
ebiederm@...ssion.com, hagen@...u.net, jack@...e.cz,
keescook@...omium.org, kirill@...temov.name, kucharsk@...il.com,
linkinjeon@...nel.org, linux-fsdevel@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-mm@...ck.org,
longpeng2@...wei.com, luto@...nel.org, markhemm@...glemail.com,
pcc@...gle.com, rppt@...nel.org, sieberf@...zon.com,
sjpark@...zon.de, surenb@...gle.com, tst@...oebel-theuer.de,
yzaikin@...gle.com
Subject: Re: [PATCH v2 3/9] mm/mshare: make msharefs writable and support
directories
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@...cle.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
Powered by blists - more mailing lists