Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <bda1393a-9e11-4d51-8911-f038918d2ddb@rivosinc.com>
Date:   Mon, 25 Sep 2023 10:10:57 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] tracing/user_events: align uaddr on unsigned long
 alignment
Content-Language: en-US
To:     Beau Belgrave <beaub@linux.microsoft.com>
Cc:     Steven Rostedt <rostedt@goodmis.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org
References: <20230914131102.179100-1-cleger@rivosinc.com>
 <20230914131700.0ba3ee80@gandalf.local.home>
 <20230914132956.569dad45@gandalf.local.home>
 <a736f219-9a38-4f95-a874-93e1561906d5@rivosinc.com>
 <20230922200042.GA1981-beaub@linux.microsoft.com>
From:   =?UTF-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= <cleger@rivosinc.com>
In-Reply-To: <20230922200042.GA1981-beaub@linux.microsoft.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 22/09/2023 22:00, Beau Belgrave wrote:
> On Tue, Sep 19, 2023 at 02:59:12PM +0200, Clément Léger wrote:
>>
>>
>> On 14/09/2023 19:29, Steven Rostedt wrote:
>>> On Thu, 14 Sep 2023 13:17:00 -0400
>>> Steven Rostedt <rostedt@goodmis.org> wrote:
>>>
>>>> Now lets look at big endian layout:
>>>>
>>>>  uaddr = 0xbeef0004
>>>>  enabler = 1;
>>>>
>>>>  memory at 0xbeef0000:  00 00 00 00 00 00 00 02
>>>>                                     ^
>>>>                                     addr: 0xbeef0004
>>>>
>>>> 				(enabler is set )
>>>>
>>>> 	bitoffset = uaddr & (sizeof(unsigned long) - 1); bitoffset = 4
>>>> 	bit_offset *= 8;				 bitoffset = 32
>>>> 	uaddr &= ~(sizeof(unsigned long) - 1);		 uaddr = 0xbeef0000
>>>>
>>>> 	ptr = kaddr + (uaddr & ~PAGE_MASK);
>>>>
>>>> 	clear_bit(1 + 32, ptr);
>>>>
>>>>  memory at 0xbeef0000:  00 00 00 00 00 00 00 02
>>>>                                   ^
>>>> 				bit 33 of 0xbeef0000
>>>>
>>>> I don't think that's what you expected!
>>>
>>> I believe the above can be fixed with:
>>>
>>> 	bit_offset = uaddr & (sizeof(unsigned long) - 1);
>>> 	if (bit_offset) {
>>> #ifdef CONFIG_CPU_BIG_ENDIAN
>>> 		bit_offest = 0;
>>> #else
>>> 		bit_offset *= BITS_PER_BYTE;
>>> #endif
>>> 		uaddr &= ~(sizeof(unsigned long) - 1);
>>> 	}
>>>
>>> -- Steve
>>
>>
>> Actually, after looking more in depth at that, it seems like there are
>> actually 2 problems that can happen.
>>
>> First one is atomic access misalignment due to enable_size == 4 and
>> uaddr not being aligned on a (long) boundary on 64 bits architecture.
>> This can generate misaligned exceptions on various architectures. This
>> can be fixed in a more general way according to Masami snippet.
>>
>> Second one that I can see is on 64 bits, big endian architectures with
>> enable_size == 4. In that case, the bit provided by the userspace won't
>> be correctly set since this code kind of assume that the atomic are done
>> on 32bits value. Since the kernel assume long sized atomic operation, on
>> big endian 64 bits architecture, the updated bit will actually be in the
>> next 32 bits word.
>>
>> Can someone confirm my understanding ?
>>
> 
> Actually, I take back some of what I said [1]. If a 32-bit on a 64-bit
> kernel comes in on BE, and is aligned, we do need to offset the bits as
> well (just verified on my ppc64 BE VM).

Yes, that is what I meant in my previous comment. I'll resend my series
which handles that properly (and which includes generic
set_bit_unaligned()).

Thanks,

Clément

> 
> You should be able to use that patch as a base though and add a flag to
> struct user_event_enabler when this case occurs. Then in the
> align_addr_bit() adjust the bits as well upon aligned cases.
> 
> Thanks,
> -Beau
> 
> 1. https://lore.kernel.org/linux-trace-kernel/20230922192231.GA1828-beaub@linux.microsoft.com/
> 
>> Clément