| Message ID | 20220516164718.2419891-7-shr@fb.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io-uring/xfs: support async buffered writes | expand |
On Mon, May 16, 2022 at 09:47:08AM -0700, Stefan Roesch wrote: > This splits off the function need_remove_file_privs() from the function > do_remove_file_privs() from the function file_remove_privs(). > > No intended functional changes in this patch. > > Signed-off-by: Stefan Roesch <shr@fb.com> > --- Just a few nits... > fs/inode.c | 57 ++++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 38 insertions(+), 19 deletions(-) > > diff --git a/fs/inode.c b/fs/inode.c > index 9d9b422504d1..a6d70a1983f8 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -2010,17 +2010,8 @@ static int __remove_privs(struct user_namespace *mnt_userns, > return notify_change(mnt_userns, dentry, &newattrs, NULL); > } > > -/* > - * Remove special file priviledges (suid, capabilities) when file is written > - * to or truncated. > - */ > -int file_remove_privs(struct file *file) > +static int need_file_remove_privs(struct inode *inode, struct dentry *dentry) I'd rather call this file_needs_remove_privs()? > { > - struct dentry *dentry = file_dentry(file); > - struct inode *inode = file_inode(file); > - int kill; > - int error = 0; > - > /* > * Fast path for nothing security related. > * As well for non-regular files, e.g. blkdev inodes. > @@ -2030,16 +2021,37 @@ int file_remove_privs(struct file *file) > if (IS_NOSEC(inode) || !S_ISREG(inode->i_mode)) > return 0; > > - kill = dentry_needs_remove_privs(dentry); > - if (kill < 0) > - return kill; > - if (kill) > - error = __remove_privs(file_mnt_user_ns(file), dentry, kill); > + return dentry_needs_remove_privs(dentry); > +} > + > +static int do_file_remove_privs(struct file *file, struct inode *inode, > + struct dentry *dentry, int kill) and that __file_remove_privs() which matches the rest of the file since here we don't have a lot of do_* but rather __* convention afaict. > +{ > + int error = 0; > + > + error = __remove_privs(file_mnt_user_ns(file), dentry, kill); > if (!error) > inode_has_no_xattr(inode); > > return error; > } > + > +/* > + * Remove special file privileges (suid, capabilities) when file is written > + * to or truncated. > + */ > +int file_remove_privs(struct file *file) This is a generic comment, not aimed specifically at your change but we really need to get better at kernel-doc... Since you're already touching this code could you at least to the exported function you're modifying add sm like: /** * file_remove_privs - remove special file privileges (suid, capabilities) * @file: file to remove privileges from * * When file is modified by a write or truncation ensure that special * file privileges are removed. * * Return: 0 on success, negative errno on failure. */ int file_remove_privs(struct file *file) This will then render on kernel.org/doc see e.g. lookup_one(): https://www.kernel.org/doc/html/latest/filesystems/api-summary.html?highlight=lookup_one#c.lookup_one > +{ > + struct dentry *dentry = file_dentry(file); > + struct inode *inode = file_inode(file); > + int kill; > + > + kill = need_file_remove_privs(inode, dentry); > + if (kill <= 0) > + return kill; > + > + return do_file_remove_privs(file, inode, dentry, kill); > +} > EXPORT_SYMBOL(file_remove_privs); > > /** > @@ -2093,15 +2105,22 @@ EXPORT_SYMBOL(file_update_time); > /* Caller must hold the file's inode lock */ > int file_modified(struct file *file) Similar I'd add sm like: /** * file_modified - handle mandated vfs changes when modifying a file * @file: file that was was modified * * When file has been modified ensure that special * file privileges are removed and time settings are updated. * * Context: Caller must hold the file's inode lock. * * Return: 0 on success, negative errno on failure. */ int file_remove_privs(struct file *file) > { > - int err; > + int ret; > + struct dentry *dentry = file_dentry(file); > + struct inode *inode = file_inode(file); > > /* > * Clear the security bits if the process is not being run by root. > * This keeps people from modifying setuid and setgid binaries. > */ > - err = file_remove_privs(file); > - if (err) > - return err; > + ret = need_file_remove_privs(inode, dentry); > + if (ret < 0) { > + return ret; > + } else if (ret > 0) { > + ret = do_file_remove_privs(file, inode, dentry, ret); > + if (ret) > + return ret; > + } The else-if branch looks a bit unorthodox to me. I'd probably rather make this: ret = need_file_remove_privs(inode, dentry); if (ret < 0) return ret; if (ret > 0) { ret = do_file_remove_privs(file, inode, dentry, ret); if (ret) return ret; } > > if (unlikely(file->f_mode & FMODE_NOCMTIME)) > return 0; > -- > 2.30.2 >
On 5/17/22 6:18 AM, Christian Brauner wrote: > On Mon, May 16, 2022 at 09:47:08AM -0700, Stefan Roesch wrote: >> This splits off the function need_remove_file_privs() from the function >> do_remove_file_privs() from the function file_remove_privs(). >> >> No intended functional changes in this patch. >> >> Signed-off-by: Stefan Roesch <shr@fb.com> >> --- > > Just a few nits... > >> fs/inode.c | 57 ++++++++++++++++++++++++++++++++++++------------------ >> 1 file changed, 38 insertions(+), 19 deletions(-) >> >> diff --git a/fs/inode.c b/fs/inode.c >> index 9d9b422504d1..a6d70a1983f8 100644 >> --- a/fs/inode.c >> +++ b/fs/inode.c >> @@ -2010,17 +2010,8 @@ static int __remove_privs(struct user_namespace *mnt_userns, >> return notify_change(mnt_userns, dentry, &newattrs, NULL); >> } >> >> -/* >> - * Remove special file priviledges (suid, capabilities) when file is written >> - * to or truncated. >> - */ >> -int file_remove_privs(struct file *file) >> +static int need_file_remove_privs(struct inode *inode, struct dentry *dentry) > > I'd rather call this file_needs_remove_privs()? > Renamed to file_needs_remove_privs() >> { >> - struct dentry *dentry = file_dentry(file); >> - struct inode *inode = file_inode(file); >> - int kill; >> - int error = 0; >> - >> /* >> * Fast path for nothing security related. >> * As well for non-regular files, e.g. blkdev inodes. >> @@ -2030,16 +2021,37 @@ int file_remove_privs(struct file *file) >> if (IS_NOSEC(inode) || !S_ISREG(inode->i_mode)) >> return 0; >> >> - kill = dentry_needs_remove_privs(dentry); >> - if (kill < 0) >> - return kill; >> - if (kill) >> - error = __remove_privs(file_mnt_user_ns(file), dentry, kill); >> + return dentry_needs_remove_privs(dentry); >> +} >> + >> +static int do_file_remove_privs(struct file *file, struct inode *inode, >> + struct dentry *dentry, int kill) > > and that __file_remove_privs() which matches the rest of the file since > here we don't have a lot of do_* but rather __* convention afaict. > Renamed the function to __file_remove_privs(). >> +{ >> + int error = 0; >> + >> + error = __remove_privs(file_mnt_user_ns(file), dentry, kill); >> if (!error) >> inode_has_no_xattr(inode); >> >> return error; >> } >> + >> +/* >> + * Remove special file privileges (suid, capabilities) when file is written >> + * to or truncated. >> + */ >> +int file_remove_privs(struct file *file) > > This is a generic comment, not aimed specifically at your change but we > really need to get better at kernel-doc... > > Since you're already touching this code could you at least to the > exported function you're modifying add sm like: > I added the kernel documentation. > /** > * file_remove_privs - remove special file privileges (suid, capabilities) > * @file: file to remove privileges from > * > * When file is modified by a write or truncation ensure that special > * file privileges are removed. > * > * Return: 0 on success, negative errno on failure. > */ > int file_remove_privs(struct file *file) > > This will then render on kernel.org/doc see e.g. lookup_one(): > https://www.kernel.org/doc/html/latest/filesystems/api-summary.html?highlight=lookup_one#c.lookup_one > >> +{ >> + struct dentry *dentry = file_dentry(file); >> + struct inode *inode = file_inode(file); >> + int kill; >> + >> + kill = need_file_remove_privs(inode, dentry); >> + if (kill <= 0) >> + return kill; >> + >> + return do_file_remove_privs(file, inode, dentry, kill); >> +} >> EXPORT_SYMBOL(file_remove_privs); >> >> /** >> @@ -2093,15 +2105,22 @@ EXPORT_SYMBOL(file_update_time); >> /* Caller must hold the file's inode lock */ >> int file_modified(struct file *file) > > Similar I'd add sm like: > I added the kernel documentation. > /** > * file_modified - handle mandated vfs changes when modifying a file > * @file: file that was was modified > * > * When file has been modified ensure that special > * file privileges are removed and time settings are updated. > * > * Context: Caller must hold the file's inode lock. > * > * Return: 0 on success, negative errno on failure. > */ > int file_remove_privs(struct file *file) > >> { >> - int err; >> + int ret; >> + struct dentry *dentry = file_dentry(file); >> + struct inode *inode = file_inode(file); >> >> /* >> * Clear the security bits if the process is not being run by root. >> * This keeps people from modifying setuid and setgid binaries. >> */ >> - err = file_remove_privs(file); >> - if (err) >> - return err; >> + ret = need_file_remove_privs(inode, dentry); >> + if (ret < 0) { >> + return ret; >> + } else if (ret > 0) { >> + ret = do_file_remove_privs(file, inode, dentry, ret); >> + if (ret) >> + return ret; >> + } > > The else-if branch looks a bit unorthodox to me. I'd probably rather > make this: > > ret = need_file_remove_privs(inode, dentry); > if (ret < 0) > return ret; > > if (ret > 0) { > ret = do_file_remove_privs(file, inode, dentry, ret); > if (ret) > return ret; > } >> >> if (unlikely(file->f_mode & FMODE_NOCMTIME)) >> return 0; I replaced the else if. >> -- >> 2.30.2 >>
diff --git a/fs/inode.c b/fs/inode.c index 9d9b422504d1..a6d70a1983f8 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -2010,17 +2010,8 @@ static int __remove_privs(struct user_namespace *mnt_userns, return notify_change(mnt_userns, dentry, &newattrs, NULL); } -/* - * Remove special file priviledges (suid, capabilities) when file is written - * to or truncated. - */ -int file_remove_privs(struct file *file) +static int need_file_remove_privs(struct inode *inode, struct dentry *dentry) { - struct dentry *dentry = file_dentry(file); - struct inode *inode = file_inode(file); - int kill; - int error = 0; - /* * Fast path for nothing security related. * As well for non-regular files, e.g. blkdev inodes. @@ -2030,16 +2021,37 @@ int file_remove_privs(struct file *file) if (IS_NOSEC(inode) || !S_ISREG(inode->i_mode)) return 0; - kill = dentry_needs_remove_privs(dentry); - if (kill < 0) - return kill; - if (kill) - error = __remove_privs(file_mnt_user_ns(file), dentry, kill); + return dentry_needs_remove_privs(dentry); +} + +static int do_file_remove_privs(struct file *file, struct inode *inode, + struct dentry *dentry, int kill) +{ + int error = 0; + + error = __remove_privs(file_mnt_user_ns(file), dentry, kill); if (!error) inode_has_no_xattr(inode); return error; } + +/* + * Remove special file privileges (suid, capabilities) when file is written + * to or truncated. + */ +int file_remove_privs(struct file *file) +{ + struct dentry *dentry = file_dentry(file); + struct inode *inode = file_inode(file); + int kill; + + kill = need_file_remove_privs(inode, dentry); + if (kill <= 0) + return kill; + + return do_file_remove_privs(file, inode, dentry, kill); +} EXPORT_SYMBOL(file_remove_privs); /** @@ -2093,15 +2105,22 @@ EXPORT_SYMBOL(file_update_time); /* Caller must hold the file's inode lock */ int file_modified(struct file *file) { - int err; + int ret; + struct dentry *dentry = file_dentry(file); + struct inode *inode = file_inode(file); /* * Clear the security bits if the process is not being run by root. * This keeps people from modifying setuid and setgid binaries. */ - err = file_remove_privs(file); - if (err) - return err; + ret = need_file_remove_privs(inode, dentry); + if (ret < 0) { + return ret; + } else if (ret > 0) { + ret = do_file_remove_privs(file, inode, dentry, ret); + if (ret) + return ret; + } if (unlikely(file->f_mode & FMODE_NOCMTIME)) return 0;
This splits off the function need_remove_file_privs() from the function do_remove_file_privs() from the function file_remove_privs(). No intended functional changes in this patch. Signed-off-by: Stefan Roesch <shr@fb.com> --- fs/inode.c | 57 ++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 38 insertions(+), 19 deletions(-)