[PATCH 2/4][DISCO] shiftfs: rework and extend

[Seth Forshee]

On Wed, Apr 03, 2019 at 10:15:13PM -0500, Tyler Hicks wrote:

<snip>

> > +static int shiftfs_real_fdget(const struct file *file, struct fd *lowerfd)
> > +{
> > +	struct shiftfs_file_info *file_info = file->private_data;
> > +	struct file *realfile = file_info->realfile;
> > +
> > +	lowerfd->flags = 0;
> > +	lowerfd->file = realfile;
> > +
> > +	/* Did the flags change since open? */
> > +	if (unlikely(file->f_flags & lowerfd->file->f_flags))
> 
> Is this the right bitwise operation? Maybe I'm misunderstanding the
> intent but I'd have thought you would want to XOR the two f_flags to
> detect any changes.

You are right, that looks like a typo.

> 
> > +		return shiftfs_change_flags(lowerfd->file, file->f_flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static int shiftfs_open(struct inode *inode, struct file *file)
> > +{
> > +	struct shiftfs_super_info *ssi = inode->i_sb->s_fs_info;
> > +	struct shiftfs_file_info *file_info;
> > +	struct file *realfile;
> > +	struct path *realpath;
> > +
> > +	file_info = kmem_cache_zalloc(shiftfs_file_info_cache, GFP_KERNEL);
> > +	if (!file_info)
> > +		return -ENOMEM;
> > +	file->private_data = file_info;
> > +
> > +	realpath = &file_info->realpath;
> > +	realpath->mnt = ssi->mnt;
> > +	realpath->dentry = file->f_path.dentry->d_fsdata;
> > +
> > +	realfile = shiftfs_open_realfile(file, realpath);
> > +	if (IS_ERR(realfile)) {
> > +		kfree(file_info);
> 
> This should be a call to kmem_cache_free() instead of kfree().
> 
> Also, I think you'll want to set file->private_data to NULL in this
> error path so that we don't try to re-free it shiftfs_release().

Good catch.

> 
> > +		return PTR_ERR(realfile);
> > +	}
> > +
> > +	file_info->realfile = realfile;
> > +	return 0;
> > +}
> > +
> > +static int shiftfs_release(struct inode *inode, struct file *file)
> > +{
> > +	struct shiftfs_file_info *file_info = file->private_data;
> > +
> > +	fput(file_info->realfile);
> 
> As alluded to above, I think shiftfs_release() can be reached when
> shiftfs_open() fails. Check dentry_open() for such a sequence. In that
> case, you'll only want to call fput() when file_info and
> file_info->realfile are both non-NULL.
> 
> > +	kmem_cache_free(shiftfs_file_info_cache, file_info);
> 
> May as well only do this when file_info is non-NULL, too.

Agree with both points.

Thanks,
Seth