enabler->uaddr can be aligned on 32 or 64 bits. If aligned on 32 bits,
this will result in a misaligned access on 64 bits architectures since
set_bit()/clear_bit() are expecting an unsigned long (aligned) pointer.
On architecture that do not support misaligned access, this will crash
the kernel. Align uaddr on unsigned long size to avoid such behavior.
This bug was found while running kselftests on RISC-V.
Fixes: 7235759084a4 ("tracing/user_events: Use remote writes for event enablement")
Signed-off-by: Clément Léger <[email protected]>
---
kernel/trace/trace_events_user.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
index 6f046650e527..580c0fe4b23e 100644
--- a/kernel/trace/trace_events_user.c
+++ b/kernel/trace/trace_events_user.c
@@ -479,7 +479,7 @@ static int user_event_enabler_write(struct user_event_mm *mm,
bool fixup_fault, int *attempt)
{
unsigned long uaddr = enabler->addr;
- unsigned long *ptr;
+ unsigned long *ptr, bit_offset;
struct page *page;
void *kaddr;
int ret;
@@ -511,13 +511,19 @@ static int user_event_enabler_write(struct user_event_mm *mm,
}
kaddr = kmap_local_page(page);
+
+ bit_offset = uaddr & (sizeof(unsigned long) - 1);
+ if (bit_offset) {
+ bit_offset *= 8;
+ uaddr &= ~(sizeof(unsigned long) - 1);
+ }
ptr = kaddr + (uaddr & ~PAGE_MASK);
/* Update bit atomically, user tracers must be atomic as well */
if (enabler->event && enabler->event->status)
- set_bit(ENABLE_BIT(enabler), ptr);
+ set_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
else
- clear_bit(ENABLE_BIT(enabler), ptr);
+ clear_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
kunmap_local(kaddr);
unpin_user_pages_dirty_lock(&page, 1, true);
--
2.40.1
On Thu, Sep 14, 2023 at 03:11:02PM +0200, Cl?ment L?ger wrote:
> enabler->uaddr can be aligned on 32 or 64 bits. If aligned on 32 bits,
> this will result in a misaligned access on 64 bits architectures since
> set_bit()/clear_bit() are expecting an unsigned long (aligned) pointer.
> On architecture that do not support misaligned access, this will crash
> the kernel. Align uaddr on unsigned long size to avoid such behavior.
> This bug was found while running kselftests on RISC-V.
>
> Fixes: 7235759084a4 ("tracing/user_events: Use remote writes for event enablement")
> Signed-off-by: Cl?ment L?ger <[email protected]>
Thanks for fixing! I have a few comments on this.
I unfortunately do not have RISC-V hardware to validate this on.
> ---
> kernel/trace/trace_events_user.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> index 6f046650e527..580c0fe4b23e 100644
> --- a/kernel/trace/trace_events_user.c
> +++ b/kernel/trace/trace_events_user.c
> @@ -479,7 +479,7 @@ static int user_event_enabler_write(struct user_event_mm *mm,
> bool fixup_fault, int *attempt)
> {
> unsigned long uaddr = enabler->addr;
> - unsigned long *ptr;
> + unsigned long *ptr, bit_offset;
> struct page *page;
> void *kaddr;
> int ret;
> @@ -511,13 +511,19 @@ static int user_event_enabler_write(struct user_event_mm *mm,
> }
>
> kaddr = kmap_local_page(page);
> +
> + bit_offset = uaddr & (sizeof(unsigned long) - 1);
> + if (bit_offset) {
> + bit_offset *= 8;
I think for future readers of this code it would be more clear to use
BITS_PER_BYTE instead of the hardcoded 8. Given we always align on a
"natural" boundary, I believe the bit_offset will always be 32 bits.
A comment here might help clarify why we do this as well in case folks
don't see the change description.
> + uaddr &= ~(sizeof(unsigned long) - 1);
Shouldn't this uaddr change be done before calling pin_user_pages_remote()
to ensure things cannot go bad? (I don't think they can, but it looks a
little odd).
Thanks,
-Beau
> + }
> ptr = kaddr + (uaddr & ~PAGE_MASK);
>
> /* Update bit atomically, user tracers must be atomic as well */
> if (enabler->event && enabler->event->status)
> - set_bit(ENABLE_BIT(enabler), ptr);
> + set_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
> else
> - clear_bit(ENABLE_BIT(enabler), ptr);
> + clear_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
>
> kunmap_local(kaddr);
> unpin_user_pages_dirty_lock(&page, 1, true);
> --
> 2.40.1
On 14/09/2023 18:42, Beau Belgrave wrote:
> On Thu, Sep 14, 2023 at 03:11:02PM +0200, Clément Léger wrote:
>> enabler->uaddr can be aligned on 32 or 64 bits. If aligned on 32 bits,
>> this will result in a misaligned access on 64 bits architectures since
>> set_bit()/clear_bit() are expecting an unsigned long (aligned) pointer.
>> On architecture that do not support misaligned access, this will crash
>> the kernel. Align uaddr on unsigned long size to avoid such behavior.
>> This bug was found while running kselftests on RISC-V.
>>
>> Fixes: 7235759084a4 ("tracing/user_events: Use remote writes for event enablement")
>> Signed-off-by: Clément Léger <[email protected]>
>
> Thanks for fixing! I have a few comments on this.
>
> I unfortunately do not have RISC-V hardware to validate this on.
>
>> ---
>> kernel/trace/trace_events_user.c | 12 +++++++++---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
>> index 6f046650e527..580c0fe4b23e 100644
>> --- a/kernel/trace/trace_events_user.c
>> +++ b/kernel/trace/trace_events_user.c
>> @@ -479,7 +479,7 @@ static int user_event_enabler_write(struct user_event_mm *mm,
>> bool fixup_fault, int *attempt)
>> {
>> unsigned long uaddr = enabler->addr;
>> - unsigned long *ptr;
>> + unsigned long *ptr, bit_offset;
>> struct page *page;
>> void *kaddr;
>> int ret;
>> @@ -511,13 +511,19 @@ static int user_event_enabler_write(struct user_event_mm *mm,
>> }
>>
>> kaddr = kmap_local_page(page);
>> +
>> + bit_offset = uaddr & (sizeof(unsigned long) - 1);
>> + if (bit_offset) {
>> + bit_offset *= 8;
>
> I think for future readers of this code it would be more clear to use
> BITS_PER_BYTE instead of the hardcoded 8. Given we always align on a
> "natural" boundary, I believe the bit_offset will always be 32 bits.
>
> A comment here might help clarify why we do this as well in case folks
> don't see the change description.
Hi Beau,
Yes sure, I'll add a comment and use the define as well.
>
>> + uaddr &= ~(sizeof(unsigned long) - 1);
>
> Shouldn't this uaddr change be done before calling pin_user_pages_remote()
> to ensure things cannot go bad? (I don't think they can, but it looks a
> little odd).
Indeed, I don't think that will cause any problem since pin_user_pages
will return a page aligned address anyway and that aligning uaddr will
not yield any page crossing. But I'll check to be sure and move that
before the call if needed.
Clément
>
> Thanks,
> -Beau
>
>> + }
>> ptr = kaddr + (uaddr & ~PAGE_MASK);
>>
>> /* Update bit atomically, user tracers must be atomic as well */
>> if (enabler->event && enabler->event->status)
>> - set_bit(ENABLE_BIT(enabler), ptr);
>> + set_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
>> else
>> - clear_bit(ENABLE_BIT(enabler), ptr);
>> + clear_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
>>
>> kunmap_local(kaddr);
>> unpin_user_pages_dirty_lock(&page, 1, true);
>> --
>> 2.40.1
On Thu, 14 Sep 2023 15:11:02 +0200
Clément Léger <[email protected]> wrote:
> enabler->uaddr can be aligned on 32 or 64 bits. If aligned on 32 bits,
> this will result in a misaligned access on 64 bits architectures since
> set_bit()/clear_bit() are expecting an unsigned long (aligned) pointer.
> On architecture that do not support misaligned access, this will crash
> the kernel. Align uaddr on unsigned long size to avoid such behavior.
> This bug was found while running kselftests on RISC-V.
>
> Fixes: 7235759084a4 ("tracing/user_events: Use remote writes for event enablement")
> Signed-off-by: Clément Léger <[email protected]>
> ---
> kernel/trace/trace_events_user.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> index 6f046650e527..580c0fe4b23e 100644
> --- a/kernel/trace/trace_events_user.c
> +++ b/kernel/trace/trace_events_user.c
> @@ -479,7 +479,7 @@ static int user_event_enabler_write(struct user_event_mm *mm,
> bool fixup_fault, int *attempt)
> {
> unsigned long uaddr = enabler->addr;
> - unsigned long *ptr;
> + unsigned long *ptr, bit_offset;
> struct page *page;
> void *kaddr;
> int ret;
> @@ -511,13 +511,19 @@ static int user_event_enabler_write(struct user_event_mm *mm,
> }
>
> kaddr = kmap_local_page(page);
> +
> + bit_offset = uaddr & (sizeof(unsigned long) - 1);
> + if (bit_offset) {
> + bit_offset *= 8;
> + uaddr &= ~(sizeof(unsigned long) - 1);
> + }
> ptr = kaddr + (uaddr & ~PAGE_MASK);
>
> /* Update bit atomically, user tracers must be atomic as well */
> if (enabler->event && enabler->event->status)
> - set_bit(ENABLE_BIT(enabler), ptr);
> + set_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
> else
> - clear_bit(ENABLE_BIT(enabler), ptr);
> + clear_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
What we need are generic set_bit_aligned() and clear_bit_aligned(), which align the ptr
by unsigned long. (I think it should be done in set_bit/clear_bit, for architecture
which requires aligned access...)
#define LONG_ALIGN_DIFF(p) (p) & (sizeof(long) -1)
#define LONG_ALINGNED(p) (p) & ~(sizeof(long) - 1)
static inline void set_bit_aligned(int bit, unsigned long *ptr)
{
int offs = LONG_ALIGN_DIFF(ptr) * 8;
#ifdef __BIGENDIAN
if (bit >= offs) {
set_bit(bit - offs, LONG_ALIGNED(ptr));
} else {
set_bit(bit + BITS_PER_LONG - offs, LONG_ALIGNED(ptr) + 1);
}
#else
if (bit < BITS_PER_LONG - offs) {
set_bit(bit + offs, LONG_ALIGNED(ptr));
} else {
set_bit(bit - BITS_PER_LONG + offs, LONG_ALIGNED(ptr) + 1);
}
#endif
}
And use it.
Thank you,
>
> kunmap_local(kaddr);
> unpin_user_pages_dirty_lock(&page, 1, true);
> --
> 2.40.1
>
--
Masami Hiramatsu (Google) <[email protected]>
On 15/09/2023 04:54, Masami Hiramatsu (Google) wrote:
> On Thu, 14 Sep 2023 15:11:02 +0200
> Clément Léger <[email protected]> wrote:
>
>> enabler->uaddr can be aligned on 32 or 64 bits. If aligned on 32 bits,
>> this will result in a misaligned access on 64 bits architectures since
>> set_bit()/clear_bit() are expecting an unsigned long (aligned) pointer.
>> On architecture that do not support misaligned access, this will crash
>> the kernel. Align uaddr on unsigned long size to avoid such behavior.
>> This bug was found while running kselftests on RISC-V.
>>
>> Fixes: 7235759084a4 ("tracing/user_events: Use remote writes for event enablement")
>> Signed-off-by: Clément Léger <[email protected]>
>> ---
>> kernel/trace/trace_events_user.c | 12 +++++++++---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
>> index 6f046650e527..580c0fe4b23e 100644
>> --- a/kernel/trace/trace_events_user.c
>> +++ b/kernel/trace/trace_events_user.c
>> @@ -479,7 +479,7 @@ static int user_event_enabler_write(struct user_event_mm *mm,
>> bool fixup_fault, int *attempt)
>> {
>> unsigned long uaddr = enabler->addr;
>> - unsigned long *ptr;
>> + unsigned long *ptr, bit_offset;
>> struct page *page;
>> void *kaddr;
>> int ret;
>> @@ -511,13 +511,19 @@ static int user_event_enabler_write(struct user_event_mm *mm,
>> }
>>
>> kaddr = kmap_local_page(page);
>> +
>> + bit_offset = uaddr & (sizeof(unsigned long) - 1);
>> + if (bit_offset) {
>> + bit_offset *= 8;
>> + uaddr &= ~(sizeof(unsigned long) - 1);
>> + }
>> ptr = kaddr + (uaddr & ~PAGE_MASK);
>>
>> /* Update bit atomically, user tracers must be atomic as well */
>> if (enabler->event && enabler->event->status)
>> - set_bit(ENABLE_BIT(enabler), ptr);
>> + set_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
>> else
>> - clear_bit(ENABLE_BIT(enabler), ptr);
>> + clear_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
>
> What we need are generic set_bit_aligned() and clear_bit_aligned(), which align the ptr
> by unsigned long. (I think it should be done in set_bit/clear_bit, for architecture
> which requires aligned access...)
>
> #define LONG_ALIGN_DIFF(p) (p) & (sizeof(long) -1)
> #define LONG_ALINGNED(p) (p) & ~(sizeof(long) - 1)
>
> static inline void set_bit_aligned(int bit, unsigned long *ptr)
> {
> int offs = LONG_ALIGN_DIFF(ptr) * 8;
>
> #ifdef __BIGENDIAN
> if (bit >= offs) {
> set_bit(bit - offs, LONG_ALIGNED(ptr));
> } else {
> set_bit(bit + BITS_PER_LONG - offs, LONG_ALIGNED(ptr) + 1);
> }
> #else
> if (bit < BITS_PER_LONG - offs) {
> set_bit(bit + offs, LONG_ALIGNED(ptr));
> } else {
> set_bit(bit - BITS_PER_LONG + offs, LONG_ALIGNED(ptr) + 1);
> }
> #endif
> }
Hi Masami,
Indeed, that is a more elegant version, thanks for the snippet.
Clément
>
> And use it.
>
> Thank you,
>
>>
>> kunmap_local(kaddr);
>> unpin_user_pages_dirty_lock(&page, 1, true);
>> --
>> 2.40.1
>>
>
>
From: Clément Léger
> Sent: 14 September 2023 14:11
>
> enabler->uaddr can be aligned on 32 or 64 bits. If aligned on 32 bits,
> this will result in a misaligned access on 64 bits architectures since
> set_bit()/clear_bit() are expecting an unsigned long (aligned) pointer.
> On architecture that do not support misaligned access, this will crash
> the kernel. Align uaddr on unsigned long size to avoid such behavior.
> This bug was found while running kselftests on RISC-V.
You don't want to do it on x86-64 either.
A locked accesses that crosses a cache line boundary is horrid.
So horrid that recent cpu's can be made to fault.
I'd also doubt that other cpu that can do misaligned transfers
can even do locked ones.
For x86 (and LE) the long[] bitmap can be treated as char[]
avoiding all the problems.
Perhaps there ought to be bit a bit-array based on char[]
(not long[]) that would be endianness independent and
use byte-sized atomics.
(IIRC that is still an issue on sparc32...)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On 17/09/2023 23:09, David Laight wrote:
> From: Clément Léger
>> Sent: 14 September 2023 14:11
>>
>> enabler->uaddr can be aligned on 32 or 64 bits. If aligned on 32 bits,
>> this will result in a misaligned access on 64 bits architectures since
>> set_bit()/clear_bit() are expecting an unsigned long (aligned) pointer.
>> On architecture that do not support misaligned access, this will crash
>> the kernel. Align uaddr on unsigned long size to avoid such behavior.
>> This bug was found while running kselftests on RISC-V.
>
> You don't want to do it on x86-64 either.
> A locked accesses that crosses a cache line boundary is horrid.
> So horrid that recent cpu's can be made to fault.
Hi David,
Thanks for the additional information.
>
> I'd also doubt that other cpu that can do misaligned transfers
> can even do locked ones.
>
> For x86 (and LE) the long[] bitmap can be treated as char[]
> avoiding all the problems.
>
> Perhaps there ought to be bit a bit-array based on char[]
> (not long[]) that would be endianness independent and
> use byte-sized atomics.
That would work for a few architectures but I don't think all of them
have byte "grain" atomics. So I guess Masami solution (long aligned
set/clear_bit()) remains the best out there.
Clément
> (IIRC that is still an issue on sparc32...)
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)