On Wed, Jun 14, 2023 at 4:26 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> On Wed, Jun 14, 2023 at 10:49:06AM +0300, Amir Goldstein wrote:
> > Overlayfs and cachefiles use open_with_fake_path() to allocate internal
> > files, where overlayfs also puts a "fake" path in f_path - a path which
> > is not on the same fs as f_inode.
> >
> > Allocate a container struct backing_file for those internal files, that
> > is used to hold the "fake" ovl path along with the real path.
> >
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > ---
> > fs/cachefiles/namei.c | 4 +--
> > fs/file_table.c | 74 +++++++++++++++++++++++++++++++++++++------
> > fs/internal.h | 5 +--
> > fs/open.c | 30 +++++++++++-------
> > fs/overlayfs/file.c | 4 +--
> > include/linux/fs.h | 24 +++++++++++---
> > 6 files changed, 109 insertions(+), 32 deletions(-)
> >
> > diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> > index 82219a8f6084..283534c6bc8d 100644
> > --- a/fs/cachefiles/namei.c
> > +++ b/fs/cachefiles/namei.c
> > @@ -560,8 +560,8 @@ static bool cachefiles_open_file(struct cachefiles_object *object,
> > */
> > path.mnt = cache->mnt;
> > path.dentry = dentry;
> > - file = open_with_fake_path(&path, O_RDWR | O_LARGEFILE | O_DIRECT,
> > - d_backing_inode(dentry), cache->cache_cred);
> > + file = open_backing_file(&path, O_RDWR | O_LARGEFILE | O_DIRECT,
> > + &path, cache->cache_cred);
> > if (IS_ERR(file)) {
> > trace_cachefiles_vfs_error(object, d_backing_inode(dentry),
> > PTR_ERR(file),
> > diff --git a/fs/file_table.c b/fs/file_table.c
> > index 372653b92617..138d5d405df7 100644
> > --- a/fs/file_table.c
> > +++ b/fs/file_table.c
> > @@ -44,18 +44,40 @@ static struct kmem_cache *filp_cachep __read_mostly;
> >
> > static struct percpu_counter nr_files __cacheline_aligned_in_smp;
> >
> > +/* Container for backing file with optional real path */
> > +struct backing_file {
> > + struct file file;
> > + struct path real_path;
> > +};
> > +
> > +static inline struct backing_file *backing_file(struct file *f)
> > +{
> > + return container_of(f, struct backing_file, file);
> > +}
> > +
> > +struct path *backing_file_real_path(struct file *f)
> > +{
> > + return &backing_file(f)->real_path;
> > +}
> > +EXPORT_SYMBOL_GPL(backing_file_real_path);
> > +
> > static void file_free_rcu(struct rcu_head *head)
> > {
> > struct file *f = container_of(head, struct file, f_rcuhead);
> >
> > put_cred(f->f_cred);
> > - kmem_cache_free(filp_cachep, f);
> > + if (unlikely(f->f_mode & FMODE_BACKING))
> > + kfree(backing_file(f));
> > + else
> > + kmem_cache_free(filp_cachep, f);
> > }
> >
> > static inline void file_free(struct file *f)
> > {
> > security_file_free(f);
> > - if (!(f->f_mode & FMODE_NOACCOUNT))
> > + if (unlikely(f->f_mode & FMODE_BACKING))
> > + path_put(backing_file_real_path(f));
> > + else
> > percpu_counter_dec(&nr_files);
>
> I think this needs to be:
>
> if (unlikely(f->f_mode & FMODE_BACKING))
> path_put(backing_file_real_path(f));
>
> if (likely(!(f->f_mode & FMODE_NOACCOUNT)))
> percpu_counter_dec(&nr_files);
>
> as we do have FMODE_NOACCOUNT without FMODE_BACKING.
>
Ouch! good catch!
Thanks,
Amir.
