linux-kernel - Re: [PATCH] dmaengine: idxd: uapi: use UAPI types

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] [day] [month] [year] [list]

Message-Id: <a4c50d48-8b5f-4b5f-9a4e-6ea1f9433c0b@app.fastmail.com>
Date: Mon, 22 Dec 2025 10:21:31 +0100
From: "Arnd Bergmann" <arnd@...db.de>
To: Thomas Weißschuh <thomas.weissschuh@...utronix.de>,
 "Vinicius Costa Gomes" <vinicius.gomes@...el.com>,
 "Dave Jiang" <dave.jiang@...el.com>
Cc: dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dmaengine: idxd: uapi: use UAPI types

On Mon, Dec 22, 2025, at 09:04, Thomas Weißschuh wrote:
> Using libc types and headers from the UAPI headers is problematic as it
> introduces a dependency on a full C toolchain.
>
> Use the fixed-width integer types provided by the UAPI headers instead.
>
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@...utronix.de>

Acked-by: Arnd Bergmann <arnd@...db.de>

This is certainly an improvement. 

Since I'm looking at this file, I'd point out a few other oddities
in the header that have absolutely nothing to do with your patch:

> @@ -176,132 +172,132 @@ enum iax_completion_status {
>  #define DSA_COMP_STATUS(status)		((status) & DSA_COMP_STATUS_MASK)
> 
>  struct dsa_hw_desc {
> -	uint32_t	pasid:20;
> -	uint32_t	rsvd:11;
> -	uint32_t	priv:1;
> -	uint32_t	flags:24;
> -	uint32_t	opcode:8;
> -	uint64_t	completion_addr;
> +	__u32	pasid:20;
> +	__u32	rsvd:11;
> +	__u32	priv:1;
> +	__u32	flags:24;
> +	__u32	opcode:8;
> +	__u64	completion_addr;

Bitfields are usually a bad idea to describe hardware structures
since the actual layout is ABI specific.

>  	union {
> -		uint8_t		expected_res;
> +		__u8		expected_res;
>  		/* create delta record */
>  		struct {
> -			uint64_t	delta_addr;
> -			uint32_t	max_delta_size;
> -			uint32_t 	delt_rsvd;
> -			uint8_t 	expected_res_mask;
> +			__u64	delta_addr;
> +			__u32	max_delta_size;
> +			__u32	delt_rsvd;
> +			__u8	expected_res_mask;
>  		};

All the outer structures are marked as __packed, but
the unions and inner structures are not, so this still
ends up with padding, and a -Wpadded warning. This came
up as one of the uapi structures that is incompatible
between x86-64 and compat i386 user space.

This ends up being harmless since other members of the
union are the same 24 byte length. For consistency, I'd
always add explicit padding, and I plan to send a patch
for all uapi structures that need this in the future.

> -		uint8_t		op_specific[24];
> +		__u8		op_specific[24];
>  	};
>  } __attribute__((packed));

The packed attribute here makes the structure unsuitable
for hardware DMA access since the compiler may generate
bytewise access. These should probably all be dropped or
changed into

__attribute__((packed, aligned(8)))

to make the structure itself aligned by the members if
some members need packing.

>   * volatile and prevent the compiler from optimize the read.
>   */
>  struct dsa_completion_record {
> -	volatile uint8_t	status;
> +	volatile __u8	status;

volatile by itself is not really sufficient to do that in
portable code, there should also be a dmb_mb() or similar
around it.

As far as I can tell, the driver cannot ever be used on
anything other than x86, so there is no harm in practice.

        Arnd