| Message ID | 20201117134416.207945-5-chandanrlinux@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bail out if transaction can cause extent count to overflow | expand |
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... > + > 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)" ? --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. > */ > -- > 2.28.0 >
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. > > */ >
On Fri, Dec 04, 2020 at 02:34:17PM +0530, Chandan Babu R wrote: > 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. D'oh, you're right. > > > > > + > > > 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. Ah, right, got it. This looks fine to me then... Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > > > > --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 > > >
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); + 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); @@ -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)) { + 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. */
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(+)