diff mbox series

[3/3] xfs: make fsmap backend function key parameters const

Message ID 162872993322.1220643.17973810836146274147.stgit@magnolia (mailing list archive)
State Accepted, archived
Headers show
Series xfs: fix various bugs in fsmap | expand

Commit Message

Darrick J. Wong Aug. 12, 2021, 12:58 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

There are several GETFSMAP backend functions for XFS to cover the three
devices and various feature support.  Each of these functions are passed
pointers to the low and high keys for the dataset that userspace
requested, and a pointer to scratchpad variables that are used to
control the iteration and fill out records.  The scratchpad data can be
changed arbitrarily, but the keys are supposed to remain unchanged (and
under the control of the outermost loop in xfs_getfsmap).

Unfortunately, the data and rt backends modify the keys that are passed
in from the main control loop, which causes subsequent calls to return
incorrect query results.  Specifically, each of those two functions set
the block number in the high key to the size of their respective device.
Since fsmap results are sorted in device number order, if the lower
numbered device is smaller than the higher numbered device, the first
function will set the high key to the small size, and the key remains
unchanged as it is passed into the function for the higher numbered
device.  The second function will then fail to return all of the results
for the dataset that userspace is asking for because the keyspace is
incorrectly constrained.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_fsmap.c |   30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

Comments

Christoph Hellwig Aug. 12, 2021, 8:40 a.m. UTC | #1 
Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
Chandan Babu R Aug. 13, 2021, 11:03 a.m. UTC | #2 
On 11 Aug 2021 at 17:58, "Darrick J. Wong" <djwong@kernel.org> wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> There are several GETFSMAP backend functions for XFS to cover the three
> devices and various feature support.  Each of these functions are passed
> pointers to the low and high keys for the dataset that userspace
> requested, and a pointer to scratchpad variables that are used to
> control the iteration and fill out records.  The scratchpad data can be
> changed arbitrarily, but the keys are supposed to remain unchanged (and
> under the control of the outermost loop in xfs_getfsmap).
>
> Unfortunately, the data and rt backends modify the keys that are passed
> in from the main control loop, which causes subsequent calls to return
> incorrect query results.  Specifically, each of those two functions set
> the block number in the high key to the size of their respective device.
> Since fsmap results are sorted in device number order, if the lower
> numbered device is smaller than the higher numbered device, the first
> function will set the high key to the small size, and the key remains
> unchanged as it is passed into the function for the higher numbered
> device.  The second function will then fail to return all of the results
> for the dataset that userspace is asking for because the keyspace is
> incorrectly constrained.
>

Looks good.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_fsmap.c |   30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index a0e8ab58124b..7bcc2ab68b8d 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -61,7 +61,7 @@ xfs_fsmap_to_internal(
>  static int
>  xfs_fsmap_owner_to_rmap(
>  	struct xfs_rmap_irec	*dest,
> -	struct xfs_fsmap	*src)
> +	const struct xfs_fsmap	*src)
>  {
>  	if (!(src->fmr_flags & FMR_OF_SPECIAL_OWNER)) {
>  		dest->rm_owner = src->fmr_owner;
> @@ -171,7 +171,7 @@ struct xfs_getfsmap_info {
>  struct xfs_getfsmap_dev {
>  	u32			dev;
>  	int			(*fn)(struct xfs_trans *tp,
> -				      struct xfs_fsmap *keys,
> +				      const struct xfs_fsmap *keys,
>  				      struct xfs_getfsmap_info *info);
>  };
>  
> @@ -389,7 +389,7 @@ xfs_getfsmap_datadev_bnobt_helper(
>  static void
>  xfs_getfsmap_set_irec_flags(
>  	struct xfs_rmap_irec	*irec,
> -	struct xfs_fsmap	*fmr)
> +	const struct xfs_fsmap	*fmr)
>  {
>  	irec->rm_flags = 0;
>  	if (fmr->fmr_flags & FMR_OF_ATTR_FORK)
> @@ -404,7 +404,7 @@ xfs_getfsmap_set_irec_flags(
>  STATIC int
>  xfs_getfsmap_logdev(
>  	struct xfs_trans		*tp,
> -	struct xfs_fsmap		*keys,
> +	const struct xfs_fsmap		*keys,
>  	struct xfs_getfsmap_info	*info)
>  {
>  	struct xfs_mount		*mp = tp->t_mountp;
> @@ -473,7 +473,7 @@ xfs_getfsmap_rtdev_rtbitmap_helper(
>  STATIC int
>  __xfs_getfsmap_rtdev(
>  	struct xfs_trans		*tp,
> -	struct xfs_fsmap		*keys,
> +	const struct xfs_fsmap		*keys,
>  	int				(*query_fn)(struct xfs_trans *,
>  						    struct xfs_getfsmap_info *),
>  	struct xfs_getfsmap_info	*info)
> @@ -481,16 +481,14 @@ __xfs_getfsmap_rtdev(
>  	struct xfs_mount		*mp = tp->t_mountp;
>  	xfs_fsblock_t			start_fsb;
>  	xfs_fsblock_t			end_fsb;
> -	xfs_daddr_t			eofs;
> +	uint64_t			eofs;
>  	int				error = 0;
>  
>  	eofs = XFS_FSB_TO_BB(mp, mp->m_sb.sb_rblocks);
>  	if (keys[0].fmr_physical >= eofs)
>  		return 0;
> -	if (keys[1].fmr_physical >= eofs)
> -		keys[1].fmr_physical = eofs - 1;
>  	start_fsb = XFS_BB_TO_FSBT(mp, keys[0].fmr_physical);
> -	end_fsb = XFS_BB_TO_FSB(mp, keys[1].fmr_physical);
> +	end_fsb = XFS_BB_TO_FSB(mp, min(eofs - 1, keys[1].fmr_physical));
>  
>  	/* Set up search keys */
>  	info->low.rm_startblock = start_fsb;
> @@ -563,7 +561,7 @@ xfs_getfsmap_rtdev_rtbitmap_query(
>  STATIC int
>  xfs_getfsmap_rtdev_rtbitmap(
>  	struct xfs_trans		*tp,
> -	struct xfs_fsmap		*keys,
> +	const struct xfs_fsmap		*keys,
>  	struct xfs_getfsmap_info	*info)
>  {
>  	info->missing_owner = XFS_FMR_OWN_UNKNOWN;
> @@ -576,7 +574,7 @@ xfs_getfsmap_rtdev_rtbitmap(
>  STATIC int
>  __xfs_getfsmap_datadev(
>  	struct xfs_trans		*tp,
> -	struct xfs_fsmap		*keys,
> +	const struct xfs_fsmap		*keys,
>  	struct xfs_getfsmap_info	*info,
>  	int				(*query_fn)(struct xfs_trans *,
>  						    struct xfs_getfsmap_info *,
> @@ -591,16 +589,14 @@ __xfs_getfsmap_datadev(
>  	xfs_fsblock_t			end_fsb;
>  	xfs_agnumber_t			start_ag;
>  	xfs_agnumber_t			end_ag;
> -	xfs_daddr_t			eofs;
> +	uint64_t			eofs;
>  	int				error = 0;
>  
>  	eofs = XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks);
>  	if (keys[0].fmr_physical >= eofs)
>  		return 0;
> -	if (keys[1].fmr_physical >= eofs)
> -		keys[1].fmr_physical = eofs - 1;
>  	start_fsb = XFS_DADDR_TO_FSB(mp, keys[0].fmr_physical);
> -	end_fsb = XFS_DADDR_TO_FSB(mp, keys[1].fmr_physical);
> +	end_fsb = XFS_DADDR_TO_FSB(mp, min(eofs - 1, keys[1].fmr_physical));
>  
>  	/*
>  	 * Convert the fsmap low/high keys to AG based keys.  Initialize
> @@ -728,7 +724,7 @@ xfs_getfsmap_datadev_rmapbt_query(
>  STATIC int
>  xfs_getfsmap_datadev_rmapbt(
>  	struct xfs_trans		*tp,
> -	struct xfs_fsmap		*keys,
> +	const struct xfs_fsmap		*keys,
>  	struct xfs_getfsmap_info	*info)
>  {
>  	info->missing_owner = XFS_FMR_OWN_FREE;
> @@ -763,7 +759,7 @@ xfs_getfsmap_datadev_bnobt_query(
>  STATIC int
>  xfs_getfsmap_datadev_bnobt(
>  	struct xfs_trans		*tp,
> -	struct xfs_fsmap		*keys,
> +	const struct xfs_fsmap		*keys,
>  	struct xfs_getfsmap_info	*info)
>  {
>  	struct xfs_alloc_rec_incore	akeys[2];
diff mbox series

Patch

diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index a0e8ab58124b..7bcc2ab68b8d 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -61,7 +61,7 @@  xfs_fsmap_to_internal(
 static int
 xfs_fsmap_owner_to_rmap(
 	struct xfs_rmap_irec	*dest,
-	struct xfs_fsmap	*src)
+	const struct xfs_fsmap	*src)
 {
 	if (!(src->fmr_flags & FMR_OF_SPECIAL_OWNER)) {
 		dest->rm_owner = src->fmr_owner;
@@ -171,7 +171,7 @@  struct xfs_getfsmap_info {
 struct xfs_getfsmap_dev {
 	u32			dev;
 	int			(*fn)(struct xfs_trans *tp,
-				      struct xfs_fsmap *keys,
+				      const struct xfs_fsmap *keys,
 				      struct xfs_getfsmap_info *info);
 };
 
@@ -389,7 +389,7 @@  xfs_getfsmap_datadev_bnobt_helper(
 static void
 xfs_getfsmap_set_irec_flags(
 	struct xfs_rmap_irec	*irec,
-	struct xfs_fsmap	*fmr)
+	const struct xfs_fsmap	*fmr)
 {
 	irec->rm_flags = 0;
 	if (fmr->fmr_flags & FMR_OF_ATTR_FORK)
@@ -404,7 +404,7 @@  xfs_getfsmap_set_irec_flags(
 STATIC int
 xfs_getfsmap_logdev(
 	struct xfs_trans		*tp,
-	struct xfs_fsmap		*keys,
+	const struct xfs_fsmap		*keys,
 	struct xfs_getfsmap_info	*info)
 {
 	struct xfs_mount		*mp = tp->t_mountp;
@@ -473,7 +473,7 @@  xfs_getfsmap_rtdev_rtbitmap_helper(
 STATIC int
 __xfs_getfsmap_rtdev(
 	struct xfs_trans		*tp,
-	struct xfs_fsmap		*keys,
+	const struct xfs_fsmap		*keys,
 	int				(*query_fn)(struct xfs_trans *,
 						    struct xfs_getfsmap_info *),
 	struct xfs_getfsmap_info	*info)
@@ -481,16 +481,14 @@  __xfs_getfsmap_rtdev(
 	struct xfs_mount		*mp = tp->t_mountp;
 	xfs_fsblock_t			start_fsb;
 	xfs_fsblock_t			end_fsb;
-	xfs_daddr_t			eofs;
+	uint64_t			eofs;
 	int				error = 0;
 
 	eofs = XFS_FSB_TO_BB(mp, mp->m_sb.sb_rblocks);
 	if (keys[0].fmr_physical >= eofs)
 		return 0;
-	if (keys[1].fmr_physical >= eofs)
-		keys[1].fmr_physical = eofs - 1;
 	start_fsb = XFS_BB_TO_FSBT(mp, keys[0].fmr_physical);
-	end_fsb = XFS_BB_TO_FSB(mp, keys[1].fmr_physical);
+	end_fsb = XFS_BB_TO_FSB(mp, min(eofs - 1, keys[1].fmr_physical));
 
 	/* Set up search keys */
 	info->low.rm_startblock = start_fsb;
@@ -563,7 +561,7 @@  xfs_getfsmap_rtdev_rtbitmap_query(
 STATIC int
 xfs_getfsmap_rtdev_rtbitmap(
 	struct xfs_trans		*tp,
-	struct xfs_fsmap		*keys,
+	const struct xfs_fsmap		*keys,
 	struct xfs_getfsmap_info	*info)
 {
 	info->missing_owner = XFS_FMR_OWN_UNKNOWN;
@@ -576,7 +574,7 @@  xfs_getfsmap_rtdev_rtbitmap(
 STATIC int
 __xfs_getfsmap_datadev(
 	struct xfs_trans		*tp,
-	struct xfs_fsmap		*keys,
+	const struct xfs_fsmap		*keys,
 	struct xfs_getfsmap_info	*info,
 	int				(*query_fn)(struct xfs_trans *,
 						    struct xfs_getfsmap_info *,
@@ -591,16 +589,14 @@  __xfs_getfsmap_datadev(
 	xfs_fsblock_t			end_fsb;
 	xfs_agnumber_t			start_ag;
 	xfs_agnumber_t			end_ag;
-	xfs_daddr_t			eofs;
+	uint64_t			eofs;
 	int				error = 0;
 
 	eofs = XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks);
 	if (keys[0].fmr_physical >= eofs)
 		return 0;
-	if (keys[1].fmr_physical >= eofs)
-		keys[1].fmr_physical = eofs - 1;
 	start_fsb = XFS_DADDR_TO_FSB(mp, keys[0].fmr_physical);
-	end_fsb = XFS_DADDR_TO_FSB(mp, keys[1].fmr_physical);
+	end_fsb = XFS_DADDR_TO_FSB(mp, min(eofs - 1, keys[1].fmr_physical));
 
 	/*
 	 * Convert the fsmap low/high keys to AG based keys.  Initialize
@@ -728,7 +724,7 @@  xfs_getfsmap_datadev_rmapbt_query(
 STATIC int
 xfs_getfsmap_datadev_rmapbt(
 	struct xfs_trans		*tp,
-	struct xfs_fsmap		*keys,
+	const struct xfs_fsmap		*keys,
 	struct xfs_getfsmap_info	*info)
 {
 	info->missing_owner = XFS_FMR_OWN_FREE;
@@ -763,7 +759,7 @@  xfs_getfsmap_datadev_bnobt_query(
 STATIC int
 xfs_getfsmap_datadev_bnobt(
 	struct xfs_trans		*tp,
-	struct xfs_fsmap		*keys,
+	const struct xfs_fsmap		*keys,
 	struct xfs_getfsmap_info	*info)
 {
 	struct xfs_alloc_rec_incore	akeys[2];