* [PATCH 5/7] xfs: fix off-by-one error in fsmap's end_daddr usage [not found] <20241114153353.318020-1-hch@lst.de> @ 2024-11-14 15:33 ` Christoph Hellwig 2024-11-14 15:55 ` Darrick J. Wong 0 siblings, 1 reply; 3+ messages in thread From: Christoph Hellwig @ 2024-11-14 15:33 UTC (permalink / raw) To: Hans Holmberg; +Cc: Darrick J. Wong, stable, Zizhi Wo From: "Darrick J. Wong" <djwong@kernel.org> In commit ca6448aed4f10a, we created an "end_daddr" variable to fix fsmap reporting when the end of the range requested falls in the middle of an unknown (aka free on the rmapbt) region. Unfortunately, I didn't notice that the the code sets end_daddr to the last sector of the device but then uses that quantity to compute the length of the synthesized mapping. Zizhi Wo later observed that when end_daddr isn't set, we still don't report the last fsblock on a device because in that case (aka when info->last is true), the info->high mapping that we pass to xfs_getfsmap_group_helper has a startblock that points to the last fsblock. This is also wrong because the code uses startblock to compute the length of the synthesized mapping. Fix the second problem by setting end_daddr unconditionally, and fix the first problem by setting start_daddr to one past the end of the range to query. Cc: <stable@vger.kernel.org> # v6.11 Fixes: ca6448aed4f10a ("xfs: Fix missing interval for missing_owner in xfs fsmap") Signed-off-by: Darrick J. Wong <djwong@kernel.org> Reported-by: Zizhi Wo <wozizhi@huawei.com> --- fs/xfs/xfs_fsmap.c | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c index 8d5d4d172d15..59b7a8e50414 100644 --- a/fs/xfs/xfs_fsmap.c +++ b/fs/xfs/xfs_fsmap.c @@ -165,7 +165,8 @@ struct xfs_getfsmap_info { xfs_daddr_t next_daddr; /* next daddr we expect */ /* daddr of low fsmap key when we're using the rtbitmap */ xfs_daddr_t low_daddr; - xfs_daddr_t end_daddr; /* daddr of high fsmap key */ + /* daddr of high fsmap key, or the last daddr on the device */ + xfs_daddr_t end_daddr; u64 missing_owner; /* owner of holes */ u32 dev; /* device id */ /* @@ -388,8 +389,8 @@ xfs_getfsmap_group_helper( * we calculated from userspace's high key to synthesize the record. * Note that if the btree query found a mapping, there won't be a gap. */ - if (info->last && info->end_daddr != XFS_BUF_DADDR_NULL) - frec->start_daddr = info->end_daddr; + if (info->last) + frec->start_daddr = info->end_daddr + 1; else frec->start_daddr = xfs_gbno_to_daddr(xg, startblock); @@ -737,8 +738,8 @@ xfs_getfsmap_rtdev_rtbitmap_helper( * we calculated from userspace's high key to synthesize the record. * Note that if the btree query found a mapping, there won't be a gap. */ - if (info->last && info->end_daddr != XFS_BUF_DADDR_NULL) { - frec.start_daddr = info->end_daddr; + if (info->last) { + frec.start_daddr = info->end_daddr + 1; } else { frec.start_daddr = xfs_rtb_to_daddr(mp, start_rtb); } @@ -1108,7 +1109,10 @@ xfs_getfsmap( struct xfs_trans *tp = NULL; struct xfs_fsmap dkeys[2]; /* per-dev keys */ struct xfs_getfsmap_dev handlers[XFS_GETFSMAP_DEVS]; - struct xfs_getfsmap_info info = { NULL }; + struct xfs_getfsmap_info info = { + .fsmap_recs = fsmap_recs, + .head = head, + }; bool use_rmap; int i; int error = 0; @@ -1185,9 +1189,6 @@ xfs_getfsmap( info.next_daddr = head->fmh_keys[0].fmr_physical + head->fmh_keys[0].fmr_length; - info.end_daddr = XFS_BUF_DADDR_NULL; - info.fsmap_recs = fsmap_recs; - info.head = head; /* For each device we support... */ for (i = 0; i < XFS_GETFSMAP_DEVS; i++) { @@ -1200,17 +1201,23 @@ xfs_getfsmap( break; /* - * If this device number matches the high key, we have - * to pass the high key to the handler to limit the - * query results. If the device number exceeds the - * low key, zero out the low key so that we get - * everything from the beginning. + * If this device number matches the high key, we have to pass + * the high key to the handler to limit the query results, and + * set the end_daddr so that we can synthesize records at the + * end of the query range or device. */ if (handlers[i].dev == head->fmh_keys[1].fmr_device) { dkeys[1] = head->fmh_keys[1]; info.end_daddr = min(handlers[i].nr_sectors - 1, dkeys[1].fmr_physical); + } else { + info.end_daddr = handlers[i].nr_sectors - 1; } + + /* + * If the device number exceeds the low key, zero out the low + * key so that we get everything from the beginning. + */ if (handlers[i].dev > head->fmh_keys[0].fmr_device) memset(&dkeys[0], 0, sizeof(struct xfs_fsmap)); -- 2.45.2 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 5/7] xfs: fix off-by-one error in fsmap's end_daddr usage 2024-11-14 15:33 ` [PATCH 5/7] xfs: fix off-by-one error in fsmap's end_daddr usage Christoph Hellwig @ 2024-11-14 15:55 ` Darrick J. Wong 2024-11-14 15:56 ` Christoph Hellwig 0 siblings, 1 reply; 3+ messages in thread From: Darrick J. Wong @ 2024-11-14 15:55 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Hans Holmberg, stable, Zizhi Wo On Thu, Nov 14, 2024 at 04:33:49PM +0100, Christoph Hellwig wrote: > From: "Darrick J. Wong" <djwong@kernel.org> > > In commit ca6448aed4f10a, we created an "end_daddr" variable to fix > fsmap reporting when the end of the range requested falls in the middle > of an unknown (aka free on the rmapbt) region. Unfortunately, I didn't > notice that the the code sets end_daddr to the last sector of the device > but then uses that quantity to compute the length of the synthesized > mapping. Er... I'm not sure why you're sending this patch to Hans and stable but not linux-xfs? Also it's already on the list, though nobody's responded to it yet: https://lore.kernel.org/linux-xfs/20241108173907.GB168069@frogsfrogsfrogs/ --D > Zizhi Wo later observed that when end_daddr isn't set, we still don't > report the last fsblock on a device because in that case (aka when > info->last is true), the info->high mapping that we pass to > xfs_getfsmap_group_helper has a startblock that points to the last > fsblock. This is also wrong because the code uses startblock to > compute the length of the synthesized mapping. > > Fix the second problem by setting end_daddr unconditionally, and fix the > first problem by setting start_daddr to one past the end of the range to > query. > > Cc: <stable@vger.kernel.org> # v6.11 > Fixes: ca6448aed4f10a ("xfs: Fix missing interval for missing_owner in xfs fsmap") > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > Reported-by: Zizhi Wo <wozizhi@huawei.com> > --- > fs/xfs/xfs_fsmap.c | 35 +++++++++++++++++++++-------------- > 1 file changed, 21 insertions(+), 14 deletions(-) > > diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c > index 8d5d4d172d15..59b7a8e50414 100644 > --- a/fs/xfs/xfs_fsmap.c > +++ b/fs/xfs/xfs_fsmap.c > @@ -165,7 +165,8 @@ struct xfs_getfsmap_info { > xfs_daddr_t next_daddr; /* next daddr we expect */ > /* daddr of low fsmap key when we're using the rtbitmap */ > xfs_daddr_t low_daddr; > - xfs_daddr_t end_daddr; /* daddr of high fsmap key */ > + /* daddr of high fsmap key, or the last daddr on the device */ > + xfs_daddr_t end_daddr; > u64 missing_owner; /* owner of holes */ > u32 dev; /* device id */ > /* > @@ -388,8 +389,8 @@ xfs_getfsmap_group_helper( > * we calculated from userspace's high key to synthesize the record. > * Note that if the btree query found a mapping, there won't be a gap. > */ > - if (info->last && info->end_daddr != XFS_BUF_DADDR_NULL) > - frec->start_daddr = info->end_daddr; > + if (info->last) > + frec->start_daddr = info->end_daddr + 1; > else > frec->start_daddr = xfs_gbno_to_daddr(xg, startblock); > > @@ -737,8 +738,8 @@ xfs_getfsmap_rtdev_rtbitmap_helper( > * we calculated from userspace's high key to synthesize the record. > * Note that if the btree query found a mapping, there won't be a gap. > */ > - if (info->last && info->end_daddr != XFS_BUF_DADDR_NULL) { > - frec.start_daddr = info->end_daddr; > + if (info->last) { > + frec.start_daddr = info->end_daddr + 1; > } else { > frec.start_daddr = xfs_rtb_to_daddr(mp, start_rtb); > } > @@ -1108,7 +1109,10 @@ xfs_getfsmap( > struct xfs_trans *tp = NULL; > struct xfs_fsmap dkeys[2]; /* per-dev keys */ > struct xfs_getfsmap_dev handlers[XFS_GETFSMAP_DEVS]; > - struct xfs_getfsmap_info info = { NULL }; > + struct xfs_getfsmap_info info = { > + .fsmap_recs = fsmap_recs, > + .head = head, > + }; > bool use_rmap; > int i; > int error = 0; > @@ -1185,9 +1189,6 @@ xfs_getfsmap( > > info.next_daddr = head->fmh_keys[0].fmr_physical + > head->fmh_keys[0].fmr_length; > - info.end_daddr = XFS_BUF_DADDR_NULL; > - info.fsmap_recs = fsmap_recs; > - info.head = head; > > /* For each device we support... */ > for (i = 0; i < XFS_GETFSMAP_DEVS; i++) { > @@ -1200,17 +1201,23 @@ xfs_getfsmap( > break; > > /* > - * If this device number matches the high key, we have > - * to pass the high key to the handler to limit the > - * query results. If the device number exceeds the > - * low key, zero out the low key so that we get > - * everything from the beginning. > + * If this device number matches the high key, we have to pass > + * the high key to the handler to limit the query results, and > + * set the end_daddr so that we can synthesize records at the > + * end of the query range or device. > */ > if (handlers[i].dev == head->fmh_keys[1].fmr_device) { > dkeys[1] = head->fmh_keys[1]; > info.end_daddr = min(handlers[i].nr_sectors - 1, > dkeys[1].fmr_physical); > + } else { > + info.end_daddr = handlers[i].nr_sectors - 1; > } > + > + /* > + * If the device number exceeds the low key, zero out the low > + * key so that we get everything from the beginning. > + */ > if (handlers[i].dev > head->fmh_keys[0].fmr_device) > memset(&dkeys[0], 0, sizeof(struct xfs_fsmap)); > > -- > 2.45.2 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 5/7] xfs: fix off-by-one error in fsmap's end_daddr usage 2024-11-14 15:55 ` Darrick J. Wong @ 2024-11-14 15:56 ` Christoph Hellwig 0 siblings, 0 replies; 3+ messages in thread From: Christoph Hellwig @ 2024-11-14 15:56 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Christoph Hellwig, Hans Holmberg, stable, Zizhi Wo On Thu, Nov 14, 2024 at 07:55:35AM -0800, Darrick J. Wong wrote: > Er... I'm not sure why you're sending this patch to Hans and stable but > not linux-xfs? Also it's already on the list, though nobody's responded > to it yet: That's because git-send-email has completely stupid defaults. I picked it up as a baseline for some of my fsmap, and it just added random Ccs :( ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-11-14 15:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20241114153353.318020-1-hch@lst.de>
2024-11-14 15:33 ` [PATCH 5/7] xfs: fix off-by-one error in fsmap's end_daddr usage Christoph Hellwig
2024-11-14 15:55 ` Darrick J. Wong
2024-11-14 15:56 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox