Re: [PATCH v3 03/33] staging: rtl8192e: Mark unaligned memcpy()

[Dan Carpenter <dan.carpenter@oracle.com>]

On Sun, May 17, 2015 at 10:38:48PM +0200, Mateusz Kulikowski wrote:
> On 15.05.2015 01:14, Dan Carpenter wrote:
> > On Thu, May 14, 2015 at 10:29:39PM +0200, Mateusz Kulikowski wrote:
> >> On 13.05.2015 10:22, Dan Carpenter wrote:
> >>> On Tue, May 12, 2015 at 10:00:13PM +0200, Mateusz Kulikowski wrote:
> >>>> On 11.05.2015 10:26, Dan Carpenter wrote:
> >> (...)
> >>>>>
> >>>>> Which part isn't aligned?  I think they both are.
> >>>>>
> >>>> struct rtllib_rxb *prxb = prxbIndicateArray[j];
> >>>>
> >>>> struct rtllib_rxb {
> >>>> 	u8 nr_subframes;
> >>>> 	struct sk_buff *subframes[MAX_SUBFRAME_COUNT == 64];
> >>>> 	u8 dst[ETH_ALEN]; // here
> >>>> 	u8 src[ETH_ALEN]; // here
> >>>> } __packed;
> (...)
> > 
> > I'm not positive it's "by design" though, this is staging code so maybe
> > they just added __packed to every struct.  In fact, I'm pretty sure
> > unaligned pointers don't work on some arches so the __packed is probably
> > a bug.
> > 
> 
> I doubt anyone uses it on anything else than some low cost x86 netbooks. 
> 

Generally though, there shouldn't be pointers in __packed structs.
__packed means we care about alignment very much, probably more than we
care about speed.  So it means we are sharing the data with the hardware
or with userspace which requires a specific layout.  We shouldn't be
giving kernel pointers to userspace or the hardware.  (These are rules
of thumb).

> I removed __packed and did a small test today (download 10mb of garbage, check md5)
> - it doesn't seem to affect driver (on staging-testing, without this patchset).
> 
> Nevertheless I would prefer to leave it like that for this patch set if it's
> OK with you and perhaps include it for next series I'll work on.

I think Greg asked for a v4 of this patchset?  You can leave the code
as is if you want.  :)  You didn't introduce the bug and I'm not your
boss.  :P

regards,
dan carpenter