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

[David Butterfield <dab21774@gmail.com>]

I should clarify that I observed the history_uuids misalignment as a runtime error from libubsan:

drbd_nl.c:5091:21: runtime error: store to misaligned address 0x7fc223ffd33c for type 'u64', which requires 8 byte alignment
0x7fc223ffd33c: note: pointer points here
  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
              ^

5076 static void device_to_statistics(struct device_statistics *s,
5077                                  struct drbd_device *device)
...
5090                 for (n = 0; n < ARRAY_SIZE(md->history_uuids); n++)
5091                         history_uuids[n] = md->history_uuids[n];

The history_uuids are declared with __bin_field() which does not appear to specify an alignment.
It happens to follow a 32-bit field, so that's where it lands.
>> 272         __bin_field(14, 0, history_uuids, HISTORY_UUIDS * sizeof(__u64))

x86 won't have a problem with this; I don't know which architectures would fault.

Perhaps the easiest way to make it legal (if you care about non-x86) would be to use
the "unaligned" family of functions for these assignments, e.g. put_unaligned_le64().

Regards,
David Butterfield

On 6/12/19 7:58 AM, Lars Ellenberg wrote:
> On Wed, Jun 05, 2019 at 09:57:32AM -0600, David Butterfield wrote:
>> 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.