Re: [PATCH 1/1] v4l: Avoid unaligned access warnings when printing 4cc modifiers

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Nick,

On Mon, Jan 10, 2022 at 03:11:18PM -0800, Nick Desaulniers wrote:
> On Mon, Jan 10, 2022 at 2:48 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Pointers V4L2 pixelformat and dataformat fields in a few packed structs
> > are directly passed to printk family of functions.
> 
> I would rephrase the below statement...
> 
> > This could result in an
> > unaligned access albeit no such possibility appears to exist at the
> > moment i.e. this clang warning appears to be a false positive.
> 
> ...to:
> 
> warning: taking address of packed member 'pixelformat' of class or
> structure 'v4l2_pix_format_mplane' may result in an unaligned pointer
> value [-Waddress-of-packed-member]
> 
> The warning is correct; because `struct v4l2_pix_format_mplane` is
> __packed, it's members also have __aligned(1).  Taking the address of
> such members results in the use of underaligned pointers which is UB
> and may be caught by UBSAN or fault on architectures without unaligned
> loads should the struct instance happen to be allocated without any
> natural alignment.

Wouldn't that be the case only if the __packed attribute resulted in a
different memory layout than not having that attribute?

All these fields are aligned by 4 so I don't see how this could be an
actual problem.

> 
> >
> > Address the warning by copying the pixelformat or dataformat value to a
> > local variable first.
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Fixes: e927e1e0f0dd ("v4l: ioctl: Use %p4cc printk modifier to print FourCC codes")
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > Hi Andy, Nick,
> >
> > How about this one?
> >
> > I believe it does address the clang warning although I haven't tested it.
> 
> LGTM. Thanks Sakari and Andy for pursuing this. Just a minor nit on my
> side about the framing of this warning being a false positive; I don't
> think it is.  With that amended,
> 
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

Thanks!

-- 
Kind regards,

Sakari Ailus