Re: [PATCH V11 04/14] xfs: Check for extent overflow when adding/removing xattrs

From: Chandan Babu R <chandanrlinux@gmail.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH V11 04/14] xfs: Check for extent overflow when adding/removing xattrs
Date: Fri, 04 Dec 2020 14:34:17 +0530	[thread overview]
Message-ID: <4842874.Y7L5Z29mQF@garuda> (raw)
In-Reply-To: <20201203184559.GA106271@magnolia>

On Thu, 03 Dec 2020 10:45:59 -0800, Darrick J. Wong wrote:
> On Tue, Nov 17, 2020 at 07:14:06PM +0530, Chandan Babu R wrote:
> > Adding/removing an xattr can cause XFS_DA_NODE_MAXDEPTH extents to be
> > added. One extra extent for dabtree in case a local attr is large enough
> > to cause a double split.  It can also cause extent count to increase
> > proportional to the size of a remote xattr's value.
> > 
> > To be able to always remove an existing xattr, when adding an xattr we
> > make sure to reserve inode fork extent count required for removing max
> > sized xattr in addition to that required by the xattr add operation.
> > 
> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > ---
> >  fs/xfs/libxfs/xfs_attr.c       | 20 ++++++++++++++++++++
> >  fs/xfs/libxfs/xfs_inode_fork.h | 10 ++++++++++
> >  2 files changed, 30 insertions(+)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index fd8e6418a0d3..d53b3867b308 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -396,6 +396,8 @@ xfs_attr_set(
> >  	struct xfs_trans_res	tres;
> >  	bool			rsvd = (args->attr_filter & XFS_ATTR_ROOT);
> >  	int			error, local;
> > +	int			iext_cnt;
> > +	int			rmt_blks;
> >  	unsigned int		total;
> >  
> >  	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
> > @@ -416,6 +418,9 @@ xfs_attr_set(
> >  	 */
> >  	args->op_flags = XFS_DA_OP_OKNOENT;
> >  
> > +	rmt_blks = xfs_attr3_rmt_blocks(mp, XFS_XATTR_SIZE_MAX);
> > +	iext_cnt = XFS_IEXT_ATTR_MANIP_CNT(rmt_blks);
> 
> These values are only relevant for the xattr removal case, right?
> AFAICT the args->value != NULL case immediately after will set new
> values, so why not just move this into...

The above statements compute the extent count required to remove a maximum
sized remote xattr.

To guarantee that a user can always remove an xattr, the "args->value != NULL"
case adds to the value of iext_cnt that has been computed above. I had
extracted and placed the above set of statements since they were now common to
both "insert" and "remove" xattr operations.

> 
> > +
> >  	if (args->value) {
> >  		XFS_STATS_INC(mp, xs_attr_set);
> >  
> > @@ -442,6 +447,13 @@ xfs_attr_set(
> >  		tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
> >  		tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> >  		total = args->total;
> > +
> > +		if (local)
> > +			rmt_blks = 0;
> > +		else
> > +			rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
> > +
> > +		iext_cnt += XFS_IEXT_ATTR_MANIP_CNT(rmt_blks);
> >  	} else {
> >  		XFS_STATS_INC(mp, xs_attr_remove);
> 
> ...the bottom of this clause here.
> 
> >  
> > @@ -460,6 +472,14 @@ xfs_attr_set(
> >  
> >  	xfs_ilock(dp, XFS_ILOCK_EXCL);
> >  	xfs_trans_ijoin(args->trans, dp, 0);
> > +
> > +	if (args->value || xfs_inode_hasattr(dp)) {
> 
> Can this simply be "if (iext_cnt != 0)" ?

That would lead to a bug since iext_cnt is computed unconditionally at the
beginning of the function. An extent count reservation will be attempted when
xattr delete operation is executed against an inode which does not have an
associated attr fork. This will cause xfs_iext_count_may_overflow() to
dereference the NULL pointer at xfs_inode->i_afp->if_nextents.

> 
> --D
> 
> > +		error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK,
> > +				iext_cnt);
> > +		if (error)
> > +			goto out_trans_cancel;
> > +	}
> > +
> >  	if (args->value) {
> >  		unsigned int	quota_flags = XFS_QMOPT_RES_REGBLKS;
> >  
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > index bcac769a7df6..5de2f07d0dd5 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -47,6 +47,16 @@ struct xfs_ifork {
> >   */
> >  #define XFS_IEXT_PUNCH_HOLE_CNT		(1)
> >  
> > +/*
> > + * Adding/removing an xattr can cause XFS_DA_NODE_MAXDEPTH extents to
> > + * be added. One extra extent for dabtree in case a local attr is
> > + * large enough to cause a double split.  It can also cause extent
> > + * count to increase proportional to the size of a remote xattr's
> > + * value.
> > + */
> > +#define XFS_IEXT_ATTR_MANIP_CNT(rmt_blks) \
> > +	(XFS_DA_NODE_MAXDEPTH + max(1, rmt_blks))
> > +
> >  /*
> >   * Fork handling.
> >   */
> 

-- 
chandan