| Message ID | 20200820054349.5525-2-chandanrlinux@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bail out if transaction can cause extent count to overflow | expand |
On Thu, Aug 20, 2020 at 11:13:40AM +0530, Chandan Babu R wrote: > XFS does not check for possible overflow of per-inode extent counter > fields when adding extents to either data or attr fork. > > For e.g. > 1. Insert 5 million xattrs (each having a value size of 255 bytes) and > then delete 50% of them in an alternating manner. > > 2. On a 4k block sized XFS filesystem instance, the above causes 98511 > extents to be created in the attr fork of the inode. > > xfsaild/loop0 2008 [003] 1475.127209: probe:xfs_inode_to_disk: (ffffffffa43fb6b0) if_nextents=98511 i_ino=131 > > 3. The incore inode fork extent counter is a signed 32-bit > quantity. However the on-disk extent counter is an unsigned 16-bit > quantity and hence cannot hold 98511 extents. > > 4. The following incorrect value is stored in the attr extent counter, > # xfs_db -f -c 'inode 131' -c 'print core.naextents' /dev/loop0 > core.naextents = -32561 > > This commit adds a new helper function (i.e. > xfs_iext_count_may_overflow()) to check for overflow of the per-inode > data and xattr extent counters. Future patches will use this function to > make sure that an FS operation won't cause the extent counter to > overflow. > > Suggested-by: Darrick J. Wong <darrick.wong@oracle.com> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com> Seems reasonable so far... Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > fs/xfs/libxfs/xfs_inode_fork.c | 23 +++++++++++++++++++++++ > fs/xfs/libxfs/xfs_inode_fork.h | 2 ++ > 2 files changed, 25 insertions(+) > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c > index 0cf853d42d62..3a084aea8f85 100644 > --- a/fs/xfs/libxfs/xfs_inode_fork.c > +++ b/fs/xfs/libxfs/xfs_inode_fork.c > @@ -23,6 +23,7 @@ > #include "xfs_da_btree.h" > #include "xfs_dir2_priv.h" > #include "xfs_attr_leaf.h" > +#include "xfs_types.h" > > kmem_zone_t *xfs_ifork_zone; > > @@ -728,3 +729,25 @@ xfs_ifork_verify_local_attr( > > return 0; > } > + > +int > +xfs_iext_count_may_overflow( > + struct xfs_inode *ip, > + int whichfork, > + int nr_to_add) > +{ > + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork); > + uint64_t max_exts; > + uint64_t nr_exts; > + > + if (whichfork == XFS_COW_FORK) > + return 0; > + > + max_exts = (whichfork == XFS_ATTR_FORK) ? MAXAEXTNUM : MAXEXTNUM; > + > + nr_exts = ifp->if_nextents + nr_to_add; > + if (nr_exts < ifp->if_nextents || nr_exts > max_exts) > + return -EFBIG; > + > + return 0; > +} > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h > index a4953e95c4f3..0beb8e2a00be 100644 > --- a/fs/xfs/libxfs/xfs_inode_fork.h > +++ b/fs/xfs/libxfs/xfs_inode_fork.h > @@ -172,5 +172,7 @@ extern void xfs_ifork_init_cow(struct xfs_inode *ip); > > int xfs_ifork_verify_local_data(struct xfs_inode *ip); > int xfs_ifork_verify_local_attr(struct xfs_inode *ip); > +int xfs_iext_count_may_overflow(struct xfs_inode *ip, int whichfork, > + int nr_to_add); > > #endif /* __XFS_INODE_FORK_H__ */ > -- > 2.28.0 >
On Mon, Aug 31, 2020 at 09:08:23AM -0700, Darrick J. Wong wrote: > On Thu, Aug 20, 2020 at 11:13:40AM +0530, Chandan Babu R wrote: > > XFS does not check for possible overflow of per-inode extent counter > > fields when adding extents to either data or attr fork. > > > > For e.g. > > 1. Insert 5 million xattrs (each having a value size of 255 bytes) and > > then delete 50% of them in an alternating manner. > > > > 2. On a 4k block sized XFS filesystem instance, the above causes 98511 > > extents to be created in the attr fork of the inode. > > > > xfsaild/loop0 2008 [003] 1475.127209: probe:xfs_inode_to_disk: (ffffffffa43fb6b0) if_nextents=98511 i_ino=131 > > > > 3. The incore inode fork extent counter is a signed 32-bit > > quantity. However the on-disk extent counter is an unsigned 16-bit > > quantity and hence cannot hold 98511 extents. > > > > 4. The following incorrect value is stored in the attr extent counter, > > # xfs_db -f -c 'inode 131' -c 'print core.naextents' /dev/loop0 > > core.naextents = -32561 > > > > This commit adds a new helper function (i.e. > > xfs_iext_count_may_overflow()) to check for overflow of the per-inode > > data and xattr extent counters. Future patches will use this function to > > make sure that an FS operation won't cause the extent counter to > > overflow. > > > > Suggested-by: Darrick J. Wong <darrick.wong@oracle.com> > > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com> > > Seems reasonable so far... > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > --D > > > --- > > fs/xfs/libxfs/xfs_inode_fork.c | 23 +++++++++++++++++++++++ > > fs/xfs/libxfs/xfs_inode_fork.h | 2 ++ > > 2 files changed, 25 insertions(+) > > > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c > > index 0cf853d42d62..3a084aea8f85 100644 > > --- a/fs/xfs/libxfs/xfs_inode_fork.c > > +++ b/fs/xfs/libxfs/xfs_inode_fork.c > > @@ -23,6 +23,7 @@ > > #include "xfs_da_btree.h" > > #include "xfs_dir2_priv.h" > > #include "xfs_attr_leaf.h" > > +#include "xfs_types.h" > > > > kmem_zone_t *xfs_ifork_zone; > > > > @@ -728,3 +729,25 @@ xfs_ifork_verify_local_attr( > > > > return 0; > > } > > + > > +int > > +xfs_iext_count_may_overflow( > > + struct xfs_inode *ip, > > + int whichfork, > > + int nr_to_add) > > +{ > > + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork); > > + uint64_t max_exts; > > + uint64_t nr_exts; > > + > > + if (whichfork == XFS_COW_FORK) > > + return 0; > > + > > + max_exts = (whichfork == XFS_ATTR_FORK) ? MAXAEXTNUM : MAXEXTNUM; > > + > > + nr_exts = ifp->if_nextents + nr_to_add; > > + if (nr_exts < ifp->if_nextents || nr_exts > max_exts) > > + return -EFBIG; Something I thought of after the fact -- can you add a new fault injection point to lower the max extent count? That way we can facilitate the construction of fstests cases to check the operation of the new predicate without having to spend lots of time constructing huge fragmented files. (There /are/ test cases somewhere, riiight? ;)) No need to add it here, you can tack it onto the end of the series as a new patch. --D > > + > > + return 0; > > +} > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h > > index a4953e95c4f3..0beb8e2a00be 100644 > > --- a/fs/xfs/libxfs/xfs_inode_fork.h > > +++ b/fs/xfs/libxfs/xfs_inode_fork.h > > @@ -172,5 +172,7 @@ extern void xfs_ifork_init_cow(struct xfs_inode *ip); > > > > int xfs_ifork_verify_local_data(struct xfs_inode *ip); > > int xfs_ifork_verify_local_attr(struct xfs_inode *ip); > > +int xfs_iext_count_may_overflow(struct xfs_inode *ip, int whichfork, > > + int nr_to_add); > > > > #endif /* __XFS_INODE_FORK_H__ */ > > -- > > 2.28.0 > >
On Monday 31 August 2020 10:14:35 PM IST Darrick J. Wong wrote: > On Mon, Aug 31, 2020 at 09:08:23AM -0700, Darrick J. Wong wrote: > > On Thu, Aug 20, 2020 at 11:13:40AM +0530, Chandan Babu R wrote: > > > XFS does not check for possible overflow of per-inode extent counter > > > fields when adding extents to either data or attr fork. > > > > > > For e.g. > > > 1. Insert 5 million xattrs (each having a value size of 255 bytes) and > > > then delete 50% of them in an alternating manner. > > > > > > 2. On a 4k block sized XFS filesystem instance, the above causes 98511 > > > extents to be created in the attr fork of the inode. > > > > > > xfsaild/loop0 2008 [003] 1475.127209: probe:xfs_inode_to_disk: (ffffffffa43fb6b0) if_nextents=98511 i_ino=131 > > > > > > 3. The incore inode fork extent counter is a signed 32-bit > > > quantity. However the on-disk extent counter is an unsigned 16-bit > > > quantity and hence cannot hold 98511 extents. > > > > > > 4. The following incorrect value is stored in the attr extent counter, > > > # xfs_db -f -c 'inode 131' -c 'print core.naextents' /dev/loop0 > > > core.naextents = -32561 > > > > > > This commit adds a new helper function (i.e. > > > xfs_iext_count_may_overflow()) to check for overflow of the per-inode > > > data and xattr extent counters. Future patches will use this function to > > > make sure that an FS operation won't cause the extent counter to > > > overflow. > > > > > > Suggested-by: Darrick J. Wong <darrick.wong@oracle.com> > > > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com> > > > > Seems reasonable so far... > > > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > > > --D > > > > > --- > > > fs/xfs/libxfs/xfs_inode_fork.c | 23 +++++++++++++++++++++++ > > > fs/xfs/libxfs/xfs_inode_fork.h | 2 ++ > > > 2 files changed, 25 insertions(+) > > > > > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c > > > index 0cf853d42d62..3a084aea8f85 100644 > > > --- a/fs/xfs/libxfs/xfs_inode_fork.c > > > +++ b/fs/xfs/libxfs/xfs_inode_fork.c > > > @@ -23,6 +23,7 @@ > > > #include "xfs_da_btree.h" > > > #include "xfs_dir2_priv.h" > > > #include "xfs_attr_leaf.h" > > > +#include "xfs_types.h" > > > > > > kmem_zone_t *xfs_ifork_zone; > > > > > > @@ -728,3 +729,25 @@ xfs_ifork_verify_local_attr( > > > > > > return 0; > > > } > > > + > > > +int > > > +xfs_iext_count_may_overflow( > > > + struct xfs_inode *ip, > > > + int whichfork, > > > + int nr_to_add) > > > +{ > > > + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork); > > > + uint64_t max_exts; > > > + uint64_t nr_exts; > > > + > > > + if (whichfork == XFS_COW_FORK) > > > + return 0; > > > + > > > + max_exts = (whichfork == XFS_ATTR_FORK) ? MAXAEXTNUM : MAXEXTNUM; > > > + > > > + nr_exts = ifp->if_nextents + nr_to_add; > > > + if (nr_exts < ifp->if_nextents || nr_exts > max_exts) > > > + return -EFBIG; > > Something I thought of after the fact -- can you add a new fault > injection point to lower the max extent count? That way we can > facilitate the construction of fstests cases to check the operation of > the new predicate without having to spend lots of time constructing huge > fragmented files. Sure, I will do that. > > (There /are/ test cases somewhere, riiight? ;)) Apart from executing xfstests, I had tested the patchset with the use case described in the commit message of this patch. But with an error injection facility available, it should be easier to add tests to fstests. I will work on that. Thanks for the suggestion. > > No need to add it here, you can tack it onto the end of the series as a > new patch. > > --D > > > > + > > > + return 0; > > > +} > > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h > > > index a4953e95c4f3..0beb8e2a00be 100644 > > > --- a/fs/xfs/libxfs/xfs_inode_fork.h > > > +++ b/fs/xfs/libxfs/xfs_inode_fork.h > > > @@ -172,5 +172,7 @@ extern void xfs_ifork_init_cow(struct xfs_inode *ip); > > > > > > int xfs_ifork_verify_local_data(struct xfs_inode *ip); > > > int xfs_ifork_verify_local_attr(struct xfs_inode *ip); > > > +int xfs_iext_count_may_overflow(struct xfs_inode *ip, int whichfork, > > > + int nr_to_add); > > > > > > #endif /* __XFS_INODE_FORK_H__ */ >
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c index 0cf853d42d62..3a084aea8f85 100644 --- a/fs/xfs/libxfs/xfs_inode_fork.c +++ b/fs/xfs/libxfs/xfs_inode_fork.c @@ -23,6 +23,7 @@ #include "xfs_da_btree.h" #include "xfs_dir2_priv.h" #include "xfs_attr_leaf.h" +#include "xfs_types.h" kmem_zone_t *xfs_ifork_zone; @@ -728,3 +729,25 @@ xfs_ifork_verify_local_attr( return 0; } + +int +xfs_iext_count_may_overflow( + struct xfs_inode *ip, + int whichfork, + int nr_to_add) +{ + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork); + uint64_t max_exts; + uint64_t nr_exts; + + if (whichfork == XFS_COW_FORK) + return 0; + + max_exts = (whichfork == XFS_ATTR_FORK) ? MAXAEXTNUM : MAXEXTNUM; + + nr_exts = ifp->if_nextents + nr_to_add; + if (nr_exts < ifp->if_nextents || nr_exts > max_exts) + return -EFBIG; + + return 0; +} diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h index a4953e95c4f3..0beb8e2a00be 100644 --- a/fs/xfs/libxfs/xfs_inode_fork.h +++ b/fs/xfs/libxfs/xfs_inode_fork.h @@ -172,5 +172,7 @@ extern void xfs_ifork_init_cow(struct xfs_inode *ip); int xfs_ifork_verify_local_data(struct xfs_inode *ip); int xfs_ifork_verify_local_attr(struct xfs_inode *ip); +int xfs_iext_count_may_overflow(struct xfs_inode *ip, int whichfork, + int nr_to_add); #endif /* __XFS_INODE_FORK_H__ */
XFS does not check for possible overflow of per-inode extent counter fields when adding extents to either data or attr fork. For e.g. 1. Insert 5 million xattrs (each having a value size of 255 bytes) and then delete 50% of them in an alternating manner. 2. On a 4k block sized XFS filesystem instance, the above causes 98511 extents to be created in the attr fork of the inode. xfsaild/loop0 2008 [003] 1475.127209: probe:xfs_inode_to_disk: (ffffffffa43fb6b0) if_nextents=98511 i_ino=131 3. The incore inode fork extent counter is a signed 32-bit quantity. However the on-disk extent counter is an unsigned 16-bit quantity and hence cannot hold 98511 extents. 4. The following incorrect value is stored in the attr extent counter, # xfs_db -f -c 'inode 131' -c 'print core.naextents' /dev/loop0 core.naextents = -32561 This commit adds a new helper function (i.e. xfs_iext_count_may_overflow()) to check for overflow of the per-inode data and xattr extent counters. Future patches will use this function to make sure that an FS operation won't cause the extent counter to overflow. Suggested-by: Darrick J. Wong <darrick.wong@oracle.com> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com> --- fs/xfs/libxfs/xfs_inode_fork.c | 23 +++++++++++++++++++++++ fs/xfs/libxfs/xfs_inode_fork.h | 2 ++ 2 files changed, 25 insertions(+)