Re: [Drbd-dev] history uuids misaligned within device_statistics

[Lars Ellenberg <lars.ellenberg@linbit.com>]

On Wed, Jun 05, 2019 at 09:57:32AM -0600, David Butterfield wrote:
> While playing with DRBD I've noticed a few things I'll mention here.
> I already pointed out the problem that was most clearly a bug (indexing drbd_genl_ops by command number).
> I'll put most of these comments into separate e-mail messages to facilitate their separate dispositions.
> 
> Regards,
> David Butterfield
> -----
> 
> The history_uuids in the device_statistics are 64 bits wide, but they are defined as a
> __bin_field which does not align to a 64-bit boundary.  The history_uuids field follows a 32-bit
> field and is always 64-bit MIS-aligned.
> 
> This leads to a misaligned access at runtime during a "drbdsetup attach" command.  On x86 the 
> misaligned access will work (for non-atomic operations), but not as fast as an aligned access.
> Other architectures may produce a runtime fault.
> 
> In drbd_nl.c:
> 257 GENL_struct(DRBD_NLA_DEVICE_STATISTICS, 20, device_statistics,
> 258         __u64_field(1, 0, dev_size)  /* (sectors) */
> 259         __u64_field(2, 0, dev_read)  /* (sectors) */
> 260         __u64_field(3, 0, dev_write)  /* (sectors) */
> 261         __u64_field(4, 0, dev_al_writes)  /* activity log writes (count) */
> 262         __u64_field(5, 0, dev_bm_writes)  /*  bitmap writes  (count) */
> 263         __u32_field(6, 0, dev_upper_pending)  /* application requests in progress */
> 264         __u32_field(7, 0, dev_lower_pending)  /* backing device requests in progress */
> 265         __flg_field(8, 0, dev_upper_blocked)
> 266         __flg_field(9, 0, dev_lower_blocked)
> 267         __flg_field(10, 0, dev_al_suspended)  /* activity log suspended */
> 268         __u64_field(11, 0, dev_exposed_data_uuid)
> 269         __u64_field(12, 0, dev_current_uuid)
> 270         __u32_field(13, 0, dev_disk_flags)
> 271         //XXX This misaligns the 64-bit history_uuids, leading to misaligned CPU access
> 272         __bin_field(14, 0, history_uuids, HISTORY_UUIDS * sizeof(__u64))
> 273 )


I don't think this is "packed",
the compiler is free to align the actual struct however it feels like,
it may or may not have "padding" holes.

struct to skb and back is done by memcpy.

	Lars