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

From: Christoph Hellwig <hch@infradead.org>
To: Eric Sandeen <sandeen@sandeen.net>
Cc: Christoph Hellwig <hch@infradead.org>,
	Eric Sandeen <sandeen@redhat.com>,
	linux-xfs <linux-xfs@vger.kernel.org>
Subject: Re: [PATCH 1/2] xfs: don't take addresses of packed xfs_agfl_t member
Date: Wed, 29 Jan 2020 10:28:37 -0800	[thread overview]
Message-ID: <20200129182837.GD14855@infradead.org> (raw)
In-Reply-To: <1e0fada6-1a6f-6980-ab2c-85aa8b4998e7@sandeen.net>

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)
 

