| Message ID | 20231007084433.1417887-2-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Reduce impact of overlayfs backing files fake path | expand |
On Sat, Oct 07, 2023 at 11:44:31AM +0300, Amir Goldstein wrote: > +static inline void file_put_write_access(struct file *file) > +{ > + put_write_access(file->f_inode); > + mnt_put_write_access(file->f_path.mnt); > + if (unlikely(file->f_mode & FMODE_BACKING)) { > + struct path *real_path = backing_file_real_path(file); > + > + if (real_path->mnt) > + mnt_put_write_access(real_path->mnt); IDGI. Where do we get FMODE_BACKING combined with NULL real_path.mnt *AND* put_file_access() possibly called? Or file_get_write_access(), for that matter... FMODE_BACKING is set only in alloc_empty_backing_file(). The only caller is backing_file_open(), which immediately sets real_path to its third argument. That could only come from ovl_open_realfile(). And if that had been called with buggered struct path, it would have already blown up on mnt_idmap(realpath->mnt). The only interval where such beasts exist is from ff->file.f_mode |= FMODE_BACKING | FMODE_NOACCOUNT; return &ff->file; in alloc_empty_backing_file() through f->f_path = *path; path_get(real_path); *backing_file_real_path(f) = *real_path; in backing_file_open(). Where would that struct file (just allocated, never seen outside of local variables in those two scopes) be passed to get_file_write_access() or put_file_access()? Or am I misreading something?
On Mon, Oct 9, 2023 at 9:43 AM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Sat, Oct 07, 2023 at 11:44:31AM +0300, Amir Goldstein wrote: > > +static inline void file_put_write_access(struct file *file) > > +{ > > + put_write_access(file->f_inode); > > + mnt_put_write_access(file->f_path.mnt); > > + if (unlikely(file->f_mode & FMODE_BACKING)) { > > + struct path *real_path = backing_file_real_path(file); > > + > > + if (real_path->mnt) > > + mnt_put_write_access(real_path->mnt); > > IDGI. Where do we get FMODE_BACKING combined with NULL real_path.mnt *AND* > put_file_access() possibly called? Or file_get_write_access(), for > that matter... Right. I was being over prudent here. > > FMODE_BACKING is set only in alloc_empty_backing_file(). The only caller > is backing_file_open(), which immediately sets real_path to its third > argument. That could only come from ovl_open_realfile(). And if that > had been called with buggered struct path, it would have already blown > up on mnt_idmap(realpath->mnt). > > The only interval where such beasts exist is from > ff->file.f_mode |= FMODE_BACKING | FMODE_NOACCOUNT; > return &ff->file; > in alloc_empty_backing_file() through > > f->f_path = *path; > path_get(real_path); > *backing_file_real_path(f) = *real_path; > > in backing_file_open(). Where would that struct file (just allocated, > never seen outside of local variables in those two scopes) be passed > to get_file_write_access() or put_file_access()? > > Or am I misreading something? No. You are right. I admit that I did consider adding use cases in the future where a backing_file real_path is initialized lazily, but that is not the case with current overlayfs code, so we don't need to worry about that now. Thanks, Amir.
diff --git a/fs/internal.h b/fs/internal.h index f08d8fe3ae5e..846d5133dd9c 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -96,13 +96,24 @@ struct file *alloc_empty_file_noaccount(int flags, const struct cred *cred); struct file *alloc_empty_backing_file(int flags, const struct cred *cred); void release_empty_file(struct file *f); +static inline void file_put_write_access(struct file *file) +{ + put_write_access(file->f_inode); + mnt_put_write_access(file->f_path.mnt); + if (unlikely(file->f_mode & FMODE_BACKING)) { + struct path *real_path = backing_file_real_path(file); + + if (real_path->mnt) + mnt_put_write_access(real_path->mnt); + } +} + static inline void put_file_access(struct file *file) { if ((file->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) { i_readcount_dec(file->f_inode); } else if (file->f_mode & FMODE_WRITER) { - put_write_access(file->f_inode); - mnt_put_write_access(file->f_path.mnt); + file_put_write_access(file); } } diff --git a/fs/open.c b/fs/open.c index a65ce47810cf..2f3e28512663 100644 --- a/fs/open.c +++ b/fs/open.c @@ -870,6 +870,33 @@ SYSCALL_DEFINE3(fchown, unsigned int, fd, uid_t, user, gid_t, group) return ksys_fchown(fd, user, group); } +static inline int file_get_write_access(struct file *f) +{ + int error; + + error = get_write_access(f->f_inode); + if (unlikely(error)) + return error; + error = mnt_get_write_access(f->f_path.mnt); + if (unlikely(error)) + goto cleanup_inode; + if (unlikely(f->f_mode & FMODE_BACKING)) { + struct path *real_path = backing_file_real_path(f); + + if (real_path->mnt) + error = mnt_get_write_access(real_path->mnt); + if (unlikely(error)) + goto cleanup_mnt; + } + return 0; + +cleanup_mnt: + mnt_put_write_access(f->f_path.mnt); +cleanup_inode: + put_write_access(f->f_inode); + return error; +} + static int do_dentry_open(struct file *f, struct inode *inode, int (*open)(struct inode *, struct file *)) @@ -892,14 +919,9 @@ static int do_dentry_open(struct file *f, if ((f->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) { i_readcount_inc(inode); } else if (f->f_mode & FMODE_WRITE && !special_file(inode->i_mode)) { - error = get_write_access(inode); + error = file_get_write_access(f); if (unlikely(error)) goto cleanup_file; - error = mnt_get_write_access(f->f_path.mnt); - if (unlikely(error)) { - put_write_access(inode); - goto cleanup_file; - } f->f_mode |= FMODE_WRITER; }
A writeable mapped backing file can perform writes to the real inode. Therefore, the real path mount must be kept writable so long as the writable map exists. This may not be strictly needed for ovelrayfs private upper mount, but it is correct to take the mnt_writers count in the vfs helper. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/internal.h | 15 +++++++++++++-- fs/open.c | 34 ++++++++++++++++++++++++++++------ 2 files changed, 41 insertions(+), 8 deletions(-)