Miklos Szeredi <miklos@xxxxxxxxxx> writes: > On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes > <vinicius.gomes@xxxxxxxxx> wrote: >> >> Replace the override_creds_light()/revert_creds_light() pairs of >> operations with cred_guard()/cred_scoped_guard(). >> >> Only ovl_copyfile() and ovl_fallocate() use cred_scoped_guard(), >> because of 'goto', which can cause the cleanup flow to run on garbage >> memory. > > This doesn't sound good. Is this a compiler bug or a limitation of guards? > This is a gcc bug, that it accepts invalid code: i.e. with a goto you can skip the declaration of a variable and as the cleanup is inserted by the compiler unconditionally, the cleanup will run with garbage value. clang refuses to compile and emits an error. Link to a simpler version of the bug I am seeing: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91951 >> @@ -211,9 +208,8 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence) >> ovl_inode_lock(inode); >> real.file->f_pos = file->f_pos; >> >> - old_cred = ovl_override_creds_light(inode->i_sb); >> + cred_guard(ovl_creds(inode->i_sb)); >> ret = vfs_llseek(real.file, offset, whence); >> - revert_creds_light(old_cred); > > Why not use scoped guard, like in fallocate? No reason. I was only under the impression that cred_guard() was preferred over cred_scoped_guard(). > >> @@ -398,9 +393,8 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync) >> >> /* Don't sync lower file for fear of receiving EROFS error */ >> if (file_inode(real.file) == ovl_inode_upper(file_inode(file))) { >> - old_cred = ovl_override_creds_light(file_inode(file)->i_sb); >> + cred_guard(ovl_creds(file_inode(file)->i_sb)); >> ret = vfs_fsync_range(real.file, start, end, datasync); >> - revert_creds_light(old_cred); > > Same here. > Will keep it consistent whatever version is chosen. >> @@ -584,9 +571,8 @@ static int ovl_flush(struct file *file, fl_owner_t id) >> return err; >> >> if (real.file->f_op->flush) { >> - old_cred = ovl_override_creds_light(file_inode(file)->i_sb); >> + cred_guard(ovl_creds(file_inode(file)->i_sb)); > > What's the scope of this? The function or the inner block? > As far as I understand, the inner block. > Thanks, > Miklos Cheers, -- Vinicius
