Re: [PATCH 1/2] xfs: don't take addresses of packed xfs_agfl_t member

Christoph Hellwig
2020-01-29T18:28:39Z

On Wed, Jan 29, 2020 at 12:18:16PM -0600, Eric Sandeen wrote:
> >
> > But I absolutely do not see the point. If agfl_bno was unalgined
> > so is adding the offsetoff. The warnings makes no sense, and there is
> > a good reason the kernel build turned it off.
>
> Why do the warnings make no sense?

Because taking the address of a member of a packed struct itself doesn't
mean it is misaligned. Taking the address of misaligned member does
that. And adding a non-aligned offset to a pointer will make it just
as misaligned.

> TBH, the above construction actually makes a lot more intuitive sense to
> me, alignment concerns or not.

Using offsetoff to take the address of a struct member is really
strange.

If we want to stop taking the address of agfl_bno we should just remove
the field entirely:

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index fc93fd88ec89..d91177c4a1e4 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -585,11 +585,12 @@ xfs_alloc_fixup_trees(

static xfs_failaddr_t
xfs_agfl_verify(
- struct xfs_buf *bp)
+ struct xfs_buf *bp)
{
- struct xfs_mount *mp = bp->b_mount;
- struct xfs_agfl *agfl = XFS_BUF_TO_AGFL(bp);
- int i;
+ struct xfs_mount *mp = bp->b_mount;
+ struct xfs_agfl *agfl = XFS_BUF_TO_AGFL(bp);
+ __be32 *agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, bp);
+ int i;

/*
* There is no verification of non-crc AGFLs because mkfs does not
@@ -614,8 +615,8 @@ xfs_agfl_verify(
return __this_address;

for (i = 0; i < xfs_agfl_size(mp); i++) {
- if (be32_to_cpu(agfl->agfl_bno[i]) != NULLAGBLOCK &&
- be32_to_cpu(agfl->agfl_bno[i]) >= mp->m_sb.sb_agblocks)
+ if (be32_to_cpu(agfl_bno[i]) != NULLAGBLOCK &&
+ be32_to_cpu(agfl_bno[i]) >= mp->m_sb.sb_agblocks)
return __this_address;
}

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 77e9fa385980..0d0a6616e129 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -783,21 +783,21 @@ typedef struct xfs_agi {
*/
#define XFS_AGFL_DADDR(mp) ((xfs_daddr_t)(3 << (mp)->m_sectbb_log))
#define XFS_AGFL_BLOCK(mp) XFS_HDR_BLOCK(mp, XFS_AGFL_DADDR(mp))
-#define XFS_BUF_TO_AGFL(bp) ((xfs_agfl_t *)((bp)->b_addr))
+#define XFS_BUF_TO_AGFL(bp) ((struct xfs_agfl *)((bp)->b_addr))

-#define XFS_BUF_TO_AGFL_BNO(mp, bp) \
- (xfs_sb_version_hascrc(&((mp)->m_sb)) ? \
- &(XFS_BUF_TO_AGFL(bp)->agfl_bno[0]) : \
- (__be32 *)(bp)->b_addr)
+#define XFS_BUF_TO_AGFL_BNO(mp, bp) \
+ ((__be32 *) \
+ (xfs_sb_version_hascrc(&((mp)->m_sb)) ? \
+ (bp)->b_addr : \
+ ((bp)->b_addr + sizeof(struct xfs_agfl))))

-typedef struct xfs_agfl {
+struct xfs_agfl {
__be32 agfl_magicnum;
__be32 agfl_seqno;
uuid_t agfl_uuid;
__be64 agfl_lsn;
__be32 agfl_crc;
- __be32 agfl_bno[]; /* actually xfs_agfl_size(mp) */
-} __attribute__((packed)) xfs_agfl_t;
+} __attribute__((packed));

#define XFS_AGFL_CRC_OFF offsetof(struct xfs_agfl, agfl_crc)