| Message ID | 09382ee9-8539-2f1d-bd4d-7256daf38a40@redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | xfs: don't take addresses of packed structures | expand |
On Wed, Jan 29, 2020 at 11:43:13AM -0600, Eric Sandeen wrote: > gcc now warns about taking an address of a packed structure member. > > Work around this by using offsetof() instead. > > Thanks to bfoster for the suggestion and djwong for reiterating it. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > index 1b7dcbae051c..7bfc8e2437e9 100644 > --- a/fs/xfs/libxfs/xfs_format.h > +++ b/fs/xfs/libxfs/xfs_format.h > @@ -787,7 +787,8 @@ typedef struct xfs_agi { > > #define XFS_BUF_TO_AGFL_BNO(mp, bp) \ > (xfs_sb_version_hascrc(&((mp)->m_sb)) ? \ > - &(XFS_BUF_TO_AGFL(bp)->agfl_bno[0]) : \ > + (__be32 *)((char *)(bp)->b_addr + \ > + offsetof(struct xfs_agfl, agfl_bno)) : \ > (__be32 *)(bp)->b_addr) Yikes. If we want to go down this route this really needs to become an inline function (and fiven that it touches buffer is has no business in xfs_format.h). 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.
On 1/29/20 12:09 PM, Christoph Hellwig wrote: > On Wed, Jan 29, 2020 at 11:43:13AM -0600, Eric Sandeen wrote: >> gcc now warns about taking an address of a packed structure member. >> >> Work around this by using offsetof() instead. >> >> Thanks to bfoster for the suggestion and djwong for reiterating it. >> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- >> >> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h >> index 1b7dcbae051c..7bfc8e2437e9 100644 >> --- a/fs/xfs/libxfs/xfs_format.h >> +++ b/fs/xfs/libxfs/xfs_format.h >> @@ -787,7 +787,8 @@ typedef struct xfs_agi { >> >> #define XFS_BUF_TO_AGFL_BNO(mp, bp) \ >> (xfs_sb_version_hascrc(&((mp)->m_sb)) ? \ >> - &(XFS_BUF_TO_AGFL(bp)->agfl_bno[0]) : \ >> + (__be32 *)((char *)(bp)->b_addr + \ >> + offsetof(struct xfs_agfl, agfl_bno)) : \ >> (__be32 *)(bp)->b_addr) > > Yikes. If we want to go down this route this really needs to become > an inline function (and fiven that it touches buffer is has no business > in xfs_format.h). fair point > > 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? TBH, the above construction actually makes a lot more intuitive sense to me, alignment concerns or not. -Eric
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)
On 1/29/20 12:28 PM, Christoph Hellwig wrote: > 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)))) This is backwards > > -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) <peers past whitespace changes> ;) other than that, this approach seems reasonable too. -Eric
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h index 1b7dcbae051c..7bfc8e2437e9 100644 --- a/fs/xfs/libxfs/xfs_format.h +++ b/fs/xfs/libxfs/xfs_format.h @@ -787,7 +787,8 @@ typedef struct xfs_agi { #define XFS_BUF_TO_AGFL_BNO(mp, bp) \ (xfs_sb_version_hascrc(&((mp)->m_sb)) ? \ - &(XFS_BUF_TO_AGFL(bp)->agfl_bno[0]) : \ + (__be32 *)((char *)(bp)->b_addr + \ + offsetof(struct xfs_agfl, agfl_bno)) : \ (__be32 *)(bp)->b_addr) typedef struct xfs_agfl {
gcc now warns about taking an address of a packed structure member. Work around this by using offsetof() instead. Thanks to bfoster for the suggestion and djwong for reiterating it. Signed-off-by: Eric Sandeen <sandeen@redhat.com> ---