linux-kernel - Re: [Xen-devel] [PATCH RFC] xen-block: correctly define structures in public headers

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:	Tue, 3 Dec 2013 11:05:29 +0000
From:	Ian Campbell <Ian.Campbell@...rix.com>
To:	Roger Pau Monne <roger.pau@...rix.com>
CC:	<xen-devel@...ts.xenproject.org>, <linux-kernel@...r.kernel.org>,
	"Stefano Stabellini" <stefano.stabellini@...citrix.com>,
	Julien Grall <julien.grall@...aro.org>,
	David Vrabel <david.vrabel@...rix.com>,
	"Boris Ostrovsky" <boris.ostrovsky@...cle.com>
Subject: Re: [Xen-devel] [PATCH RFC] xen-block: correctly define structures
 in public headers

On Tue, 2013-12-03 at 11:57 +0100, Roger Pau Monne wrote:
> Using __packed__ on the public interface is not correct, this
> structures should be compiled using the native ABI, and __packed__
> should only be used in the backend counterpart of those structures
> (which needs to handle different ABIs).
> 
> This was even worse in the ARM case, where the Linux kernel was
> incorrectly using the X86_32 protocol ABI. This patch fixes it, but
> also breaks compatibility, so an ARM DomU kernel compiled with
> this patch will fail to communicate with PV disk devices unless the
> Dom0 also has this patch.

This is acceptable IMHO, the ARM ABI is clearly defined and previous
kernels were simply buggy. The fact that front and backend were
equivalently buggy and so it happened to work is not an excuse.

> Signed-off-by: Roger Pau Monné <roger.pau@...rix.com>
> Reported-by: Julien Grall <julien.grall@...aro.org>
> Cc: Julien Grall <julien.grall@...aro.org>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
> Cc: David Vrabel <david.vrabel@...rix.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@...cle.com>
> Cc: Stefano Stabellini <stefano.stabellini@...citrix.com>
> ---
>  include/xen/interface/io/blkif.h |   28 +++++++---------------------
>  1 files changed, 7 insertions(+), 21 deletions(-)
> 
> diff --git a/include/xen/interface/io/blkif.h b/include/xen/interface/io/blkif.h
> index 65e1209..002ea22 100644
> --- a/include/xen/interface/io/blkif.h
> +++ b/include/xen/interface/io/blkif.h
> @@ -141,14 +141,11 @@ struct blkif_request_segment_aligned {
>  	/* @last_sect: last sector in frame to transfer (inclusive).     */
>  	uint8_t     first_sect, last_sect;
>  	uint16_t    _pad; /* padding to make it 8 bytes, so it's cache-aligned */
> -} __attribute__((__packed__));
> +};
>  
>  struct blkif_request_rw {
>  	uint8_t        nr_segments;  /* number of segments                   */
>  	blkif_vdev_t   handle;       /* only for read/write requests         */
> -#ifdef CONFIG_X86_64
> -	uint32_t       _pad1;	     /* offsetof(blkif_request,u.rw.id) == 8 */
> -#endif

These padding fields would still serve a purpose even after removing the
packing, which is to document/clarify where there are holes for various
architectures. They could either be retained or perhaps replaced by a
comment?

Ian.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/