linux-kernel - Re: [PATCH 3/5] fs: sys

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]

Message-ID: <87a5jb9rnk.ffs@tglx>
Date: Mon, 24 Jun 2024 01:16:15 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Kent Overstreet <kent.overstreet@...ux.dev>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, Jens Axboe
 <axboe@...nel.dk>, brauner@...nel.org, viro@...iv.linux.org.uk, Bernd
 Schubert <bernd.schubert@...tmail.fm>, linux-mm@...ck.org, Josef Bacik
 <josef@...icpanda.com>
Subject: Re: [PATCH 3/5] fs: sys_ringbuffer

Kent!

On Sun, Jun 23 2024 at 18:21, Kent Overstreet wrote:
> On Mon, Jun 24, 2024 at 12:13:36AM +0200, Thomas Gleixner wrote:
>> > +	/*
>> > +	 * We use u32s because this type is shared between the kernel and
>> > +	 * userspace - ulong/size_t won't work here, we might be 32bit userland
>> > +	 * and 64 bit kernel, and u64 would be preferable (reduced probability
>> > +	 * of ABA) but not all architectures can atomically read/write to a u64;
>> > +	 * we need to avoid torn reads/writes.
>> 
>> union rbmagic {
>> 	u64	__val64;
>>         struct {
>>                 // TOOTIRED: Add big/little endian voodoo
>> 	        u32	__val32;
>>                 u32	__unused;
>>         };
>> };
>> 
>> Plus a bunch of accessors which depend on BITS_PER_LONG, no?
>
> Not sure I follow?
>
> I know biendian machines exist, but I've never heard of both big and
> little endian being used at the same time. Nor why we'd care about
> BITS_PER_LONG? This just uses fixed size integer types.

Read your comment above. Ideally you want to use u64, right?

The problem is that you can't do this unconditionally because of 32-bit
systems which do not support 64-bit atomics.

So a binary which is compiled for 32-bit might unconditionally want the
32-bit accessors. Ditto for 32-bit kernels.

The 64bit kernel where it runs on wants to utilize u64, right?

That's fortunately a unidirectional problem as 64-bit user space cannot
run on a 32-bit kernel ever.

struct ringbuffer_ctrl {
       union rbmagic	head;
...
};

#ifdef __BITS_PER_LONG == 64
static __always_inline u64 read_head(struct ringbuffer_ctrl *rb)
{
        return rb->head.__val64;
}

static __always_inline void write_head(struct ringbuffer_ctrl *rb, u64 val)
{
        rb->head.__val64 = val;
}
#else
static __always_inline u64 read_head(struct ringbuffer_ctrl *rb)
{
        return rb->head.__val32;
}

static __always_inline void write_head(struct ringbuffer_ctrl *rb, u64 val)
{
        rb->head.__val32 = (u32)val;
}
#endif

A 64-bit kernel uses u64 while a 32-bit kernel uses u32. Same for user
space.

The ABA concern for 32-bit does not go away, but for 64-bit userspace
you get what you want, no?

Now why do you have to care about endianess?

union rbmagic {
	u64	__val64;
	struct {
		u32	__val32;
		u32	__unused;
	};
};

works only correctly for LE. But it does not work for BE because BE
obviously requires the u32 members to be in reverse order:

union rbmagic {
	u64	__val64;
	struct {
		u32	__unused;
		u32	__val32;
	};
};

That's a compile time decision. You can't run a BE binary on a LE kernel
or the other way around.

So they have to agree on the endianess, but BE has the reverse byte
order. That's why you need to have another #ifdef there.

Thanks,

        tglx