On Mon 02-11-26 15:21:49, Christian Brauner wrote: > proc_sys_make_inode() checks under sysctl_lock that @head isn't being > unregistered, lets go of the lock, and then reads the mode of @table > and calls ->set_ownership() with @head. That's fine as long as the > caller holds @head in use, which keeps unregistering from getting past > that check. The next patch has a caller that only pins the memory of > @head. For that one the table, and whatever the owner of the header > keeps behind it, is only there for as long as nobody unregisters the > header, which happens under sysctl_lock. So read the mode and set the > ownership before letting go of the lock. The ->set_ownership() > callbacks only look up ids in a user namespace. > > No functional changes. > > Signed-off-by: Christian Brauner (Amutable) <[email protected]>
Looks good to me. Feel free to add: Reviewed-by: Jan Kara <[email protected]> Honza > --- > fs/proc/proc_sysctl.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > index 2150cf842919..a1242247f492 100644 > --- a/fs/proc/proc_sysctl.c > +++ b/fs/proc/proc_sysctl.c > @@ -500,6 +500,7 @@ static struct inode *proc_sys_make_inode(struct > super_block *sb, > struct ctl_table_root *root = head->root; > struct inode *inode; > struct proc_inode *ei; > + umode_t mode; > > inode = new_inode(sb); > if (!inode) > @@ -509,12 +510,22 @@ static struct inode *proc_sys_make_inode(struct > super_block *sb, > > ei = PROC_I(inode); > > + inode->i_uid = GLOBAL_ROOT_UID; > + inode->i_gid = GLOBAL_ROOT_GID; > spin_lock(&sysctl_lock); > if (unlikely(head->unregistering)) { > spin_unlock(&sysctl_lock); > iput(inode); > return ERR_PTR(-ENOENT); > } > + /* > + * The table, and whatever the owner of the header keeps behind it, > + * is only there for as long as nobody unregisters the header, which > + * happens under sysctl_lock. So look at them before letting go of it. > + */ > + mode = table->mode; > + if (root->set_ownership) > + root->set_ownership(head, &inode->i_uid, &inode->i_gid); > ei->sysctl = head; > ei->sysctl_entry = table; > hlist_add_head_rcu(&ei->sibling_inodes, &head->inodes); > @@ -522,8 +533,8 @@ static struct inode *proc_sys_make_inode(struct > super_block *sb, > spin_unlock(&sysctl_lock); > > simple_inode_init_ts(inode); > - inode->i_mode = table->mode; > - if (!S_ISDIR(table->mode)) { > + inode->i_mode = mode; > + if (!S_ISDIR(mode)) { > inode->i_mode |= S_IFREG; > inode->i_op = &proc_sys_inode_operations; > inode->i_fop = &proc_sys_file_operations; > @@ -535,11 +546,6 @@ static struct inode *proc_sys_make_inode(struct > super_block *sb, > make_empty_dir_inode(inode); > } > > - inode->i_uid = GLOBAL_ROOT_UID; > - inode->i_gid = GLOBAL_ROOT_GID; > - if (root->set_ownership) > - root->set_ownership(head, &inode->i_uid, &inode->i_gid); > - > return inode; > } > > > -- > 2.53.0 > -- Jan Kara <[email protected]> SUSE Labs, CR
