netdev - RE: [PATCH v5] can: add Renesas R-Car CAN driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 22 Jan 2014 11:58:50 +0000
From:	David Laight <David.Laight@...LAB.COM>
To:	'Ben Dooks' <ben.dooks@...ethink.co.uk>,
	Marc Kleine-Budde <mkl@...gutronix.de>
CC:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
	David Miller <davem@...emloft.net>,
	"geert@...ux-m68k.org" <geert@...ux-m68k.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"wg@...ndegger.com" <wg@...ndegger.com>,
	"linux-can@...r.kernel.org" <linux-can@...r.kernel.org>,
	"linux-sh@...r.kernel.org" <linux-sh@...r.kernel.org>,
	"vksavl@...il.com" <vksavl@...il.com>
Subject: RE: [PATCH v5] can: add Renesas R-Car CAN driver

From: Ben Dooks
> On 20/01/14 21:17, Marc Kleine-Budde wrote:
> > On 01/20/2014 11:12 PM, Sergei Shtylyov wrote:
> > [...]
> >
> >>> I truly think using packed here is rediculous, please remove it unless
> >>> you can prove that things won't work without it.
> >>
> >>     They will. But how about the following 'struct rcar_can_regs'?
> >
> > The strcuts in question are:
> >
> >> +/* Mailbox registers structure */
> >> +struct rcar_can_mbox_regs {
> >> +	u32 id;		/* IDE and RTR bits, SID and EID */
> >> +	u8 stub;	/* Not used */
> >> +	u8 dlc;		/* Data Length Code - bits [0..3] */
> >> +	u8 data[8];	/* Data Bytes */
> >> +	u8 tsh;		/* Time Stamp Higher Byte */
> >> +	u8 tsl;		/* Time Stamp Lower Byte */
> >> +} __packed;
> >> +
> >> +struct rcar_can_regs {
> >> +	struct rcar_can_mbox_regs mb[RCAR_CAN_N_MBX]; /* Mailbox registers */
> >> +	u32 mkr_2_9[8];	/* Mask Registers 2-9 */
> >> +	u32 fidcr[2];	/* FIFO Received ID Compare Register */
> >> +	u32 mkivlr1;	/* Mask Invalid Register 1 */
> >> +	u32 mier1;	/* Mailbox Interrupt Enable Register 1 */
> >> +	u32 mkr_0_1[2];	/* Mask Registers 0-1 */
> >> +	u32 mkivlr0;    /* Mask Invalid Register 0*/
> >> +	u32 mier0;      /* Mailbox Interrupt Enable Register 0 */
> >> +	u8 pad_440[0x3c0];
> >> +	u8 mctl[64];	/* Message Control Registers */
> >> +	u16 ctlr;	/* Control Register */
> >> +	u16 str;	/* Status register */
> >> +	u8 bcr[3];	/* Bit Configuration Register */
> >> +	u8 clkr;	/* Clock Select Register */
> >> +	u8 rfcr;	/* Receive FIFO Control Register */
> >> +	u8 rfpcr;	/* Receive FIFO Pointer Control Register */
> >> +	u8 tfcr;	/* Transmit FIFO Control Register */
> >> +	u8 tfpcr;       /* Transmit FIFO Pointer Control Register */
> >> +	u8 eier;	/* Error Interrupt Enable Register */
> >> +	u8 eifr;	/* Error Interrupt Factor Judge Register */
> >> +	u8 recr;	/* Receive Error Count Register */
> >> +	u8 tecr;        /* Transmit Error Count Register */
> >> +	u8 ecsr;	/* Error Code Store Register */
> >> +	u8 cssr;	/* Channel Search Support Register */
> >> +	u8 mssr;	/* Mailbox Search Status Register */
> >> +	u8 msmr;	/* Mailbox Search Mode Register */
> >> +	u16 tsr;	/* Time Stamp Register */
> >> +	u8 afsr;	/* Acceptance Filter Support Register */
> >> +	u8 pad_857;
> >> +	u8 tcr;		/* Test Control Register */
> >> +	u8 pad_859[7];
> >> +	u8 ier;		/* Interrupt Enable Register */
> >> +	u8 isr;		/* Interrupt Status Register */
> >> +	u8 pad_862;
> >> +	u8 mbsmr;	/* Mailbox Search Mask Register */
> >> +} __packed;
> >
> > I think they should work without packed, too.
> 
> I think this discussion proves why this is not a good idea.

You need the second structure to be the correct size, with or without
__packed there could easily be a typo - so add a compile-type assert
on the size.

If this matches some hardware spec, and the hardware spec doesn't
have anything misaligned, then you don't want to specify __packed.
Without the __packed the size check will detect more typos.

> IIRC, a compiler has the right to pad, or re-order structure
> elements as it sees fit depending on the architecture and options.

The language might allow many things, the ABI is much more explicit.
In this case you really care about the ABI.

	David