Re: [ndctl PATCH v3 1/8] ndctl/build: Suppress -Waddress-of-packed-member

[Dan Williams <dan.j.williams@intel.com>]

On Mon, Aug 5, 2019 at 10:34 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Mon, Aug 5, 2019 at 10:06 AM Jeff Moyer <jmoyer@redhat.com> wrote:
> >
> > Dan Williams <dan.j.williams@intel.com> writes:
> >
> > > gcc 9.1.1 emits a slew of warnings for many of the command field
> > > accesses. I.e. warnings of the form:
> > >
> > > libndctl.c:2586:21: warning: taking address of packed member of ‘struct nd_cmd_get_config_data_hdr’ may result in an unaligned pointer value [-Waddress-of-packed-member]
> > >  2586 |  cmd->iter.offset = &cmd->get_data->in_offset;
> > >       |                     ^~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > Suppress these as fixing the warning would defeat the abstraction of being able
> > > to have common code that operates on commands with common fields at different
> > > offsets in the payload.
> >
> > As I understand it, taking a pointer to this potentially unaligned
> > member can result in bus errors (or worse, accessing wrong data) on
> > architectures that don't support unaligned accesses.  I'd be a whole lot
> > happier with this changelog if it mentioned that you had considered what
> > the warning actually meant, and decided it didn't matter for the
> > architectures you want to support.
>
> True, it was a fleeting thought, but not something I considered
> fleshing out... I'll send a revision.
>
> >
> > x86 is, of course, safe.  I believe aarch64 is, as well.  I didn't look
> > into others.
> >
>
> Keep in mind that this code is for interfacing with the ACPI DSM path.
> If an unaligned-incapable architecture defined an NVDIMM command set
> it is highly unlikely it would be ACPI based, or pick these
> problematic command formats. I can add these notes to the changelog,
> but the unfortunate definition of these payloads that require __packed
> is something I hope other architectures avoid.

...and it turns out this is wrong because we have the default ioctls
that also use these packed structures

        ND_CMD_ARS_CAP = 1,
        ND_CMD_ARS_START = 2,
        ND_CMD_ARS_STATUS = 3,
        ND_CMD_CLEAR_ERROR = 4,

        ND_CMD_SMART = 1,
        ND_CMD_SMART_THRESHOLD = 2,
        ND_CMD_DIMM_FLAGS = 3,
        ND_CMD_GET_CONFIG_SIZE = 4,
        ND_CMD_GET_CONFIG_DATA = 5,
        ND_CMD_SET_CONFIG_DATA = 6,
        ND_CMD_VENDOR = 9,

I'll take a look at reworking this.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm