Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
On 25/09/2023 20:04, Clément Léger wrote:
>
>
> On 25/09/2023 18:04, Beau Belgrave wrote:
>> On Mon, Sep 25, 2023 at 09:53:16AM +0200, Clément Léger wrote:
>>>
>>>
>>> On 22/09/2023 21:22, 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 wrote:
>>
>>> Now lets look at big endian layout:
>>>
>>> uaddr = 0xbeef0004
>>> enabler = 1;
>>>
>>> memory at 0xbeef: 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 =
>>> 0xbeef
>>>
>>> ptr = kaddr + (uaddr & ~PAGE_MASK);
>>>
>>> clear_bit(1 + 32, ptr);
>>>
>>> memory at 0xbeef: 00 00 00 00 00 00 00 02
>>> ^
>>> bit 33 of 0xbeef
>>>
>>> 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 ?
>
I have a ppc 64bit BE VM I've been validating this on. If we do the
shifting within user_events (vs a generic set_bit_aligned approach)
64bit BE does not need additional bit manipulation. However, if we were
to blindly pass the address and bit as is to set_bit_aligned() it
assumes the bit number is for a long, not a 32 bit word. So for that
approach we would need to offset the bit in the unaligned case.
Here's a patch I have that I've validated on ppc64 BE, aarch64 LE, and
x86_64 LE. I personally feel more comfortable with this approach than
the generic set_bit_aligned() one.
Thanks,
-Beau
diff --git a/kernel/trace/trace_events_user.c
b/kernel/trace/trace_events_user.c
index e3f2b8d72e01..ae854374d0b7 100644
--- a/kernel/trace/trace_events_user.c
+++ b/kernel/trace/trace_events_user.c
@@ -162,6 +162,23 @@ struct user_event_validator {
int flags;
};
+static inline void align_addr_bit(unsigned long *addr, int *bit)
+{
+ if (IS_ALIGNED(*addr, sizeof(long)))
+ return;
+
+ *addr = ALIGN_DOWN(*addr, sizeof(long));
+
+ /*
+ * We only support 32 and 64 bit values. The only time we need
+ * to align is a 32 bit value on a 64 bit kernel, which on LE
+ * is always 32 bits, and on BE requires no change.
+ */
+#ifdef __LITTLE_ENDIAN
+ *bit += 32;
+#endif
>>>
>>> Hi Beau, except the specific alignment that is basically what I ended up
>>> with for the BE 64bit case (ie just bit += 32). Regarding the generic
>>> alignment, depends on what the maintainers wishes (generic of user_event
>>> specific). I also feel like this shoulmd be handle specifically for
>>> user_events which uses set_bit in some non standard way. Any suggestion ?
>>>
>>
>> Looking at this deeper, user_events needs to track some of this
>> alignment requirements within each enabler. This is needed because when
>> enablers are disabled/removed they do not have the actual size. The size
>> is needed to know if we need to update the bits, etc. (IE: If originally
>> a 32-bit value was used and it's aligned and it's on BE, it needs the
>> bits shifted.)
>>
>> My final version that I played around with looked like this for just
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
On 25/09/2023 18:04, Beau Belgrave wrote:
> On Mon, Sep 25, 2023 at 09:53:16AM +0200, Clément Léger wrote:
>>
>>
>> On 22/09/2023 21:22, 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 wrote:
>
>> Now lets look at big endian layout:
>>
>> uaddr = 0xbeef0004
>> enabler = 1;
>>
>> memory at 0xbeef: 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 = 0xbeef
>>
>> ptr = kaddr + (uaddr & ~PAGE_MASK);
>>
>> clear_bit(1 + 32, ptr);
>>
>> memory at 0xbeef: 00 00 00 00 00 00 00 02
>> ^
>> bit 33 of 0xbeef
>>
>> 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 ?
>>>
>>> I have a ppc 64bit BE VM I've been validating this on. If we do the
>>> shifting within user_events (vs a generic set_bit_aligned approach)
>>> 64bit BE does not need additional bit manipulation. However, if we were
>>> to blindly pass the address and bit as is to set_bit_aligned() it
>>> assumes the bit number is for a long, not a 32 bit word. So for that
>>> approach we would need to offset the bit in the unaligned case.
>>>
>>> Here's a patch I have that I've validated on ppc64 BE, aarch64 LE, and
>>> x86_64 LE. I personally feel more comfortable with this approach than
>>> the generic set_bit_aligned() one.
>>>
>>> Thanks,
>>> -Beau
>>>
>>> diff --git a/kernel/trace/trace_events_user.c
>>> b/kernel/trace/trace_events_user.c
>>> index e3f2b8d72e01..ae854374d0b7 100644
>>> --- a/kernel/trace/trace_events_user.c
>>> +++ b/kernel/trace/trace_events_user.c
>>> @@ -162,6 +162,23 @@ struct user_event_validator {
>>> int flags;
>>> };
>>>
>>> +static inline void align_addr_bit(unsigned long *addr, int *bit)
>>> +{
>>> + if (IS_ALIGNED(*addr, sizeof(long)))
>>> + return;
>>> +
>>> + *addr = ALIGN_DOWN(*addr, sizeof(long));
>>> +
>>> + /*
>>> +* We only support 32 and 64 bit values. The only time we need
>>> +* to align is a 32 bit value on a 64 bit kernel, which on LE
>>> +* is always 32 bits, and on BE requires no change.
>>> +*/
>>> +#ifdef __LITTLE_ENDIAN
>>> + *bit += 32;
>>> +#endif
>>
>> Hi Beau, except the specific alignment that is basically what I ended up
>> with for the BE 64bit case (ie just bit += 32). Regarding the generic
>> alignment, depends on what the maintainers wishes (generic of user_event
>> specific). I also feel like this shoulmd be handle specifically for
>> user_events which uses set_bit in some non standard way. Any suggestion ?
>>
>
> Looking at this deeper, user_events needs to track some of this
> alignment requirements within each enabler. This is needed because when
> enablers are disabled/removed they do not have the actual size. The size
> is needed to know if we need to update the bits, etc. (IE: If originally
> a 32-bit value was used and it's aligned and it's on BE, it needs the
> bits shifted.)
>
> My final version that I played around with looked like this for just the
> alignment requirements:
> +static inline void align_addr_bit(unsigned long *addr, int *bit,
> + unsigned long *flags)
> +{
> + if (IS_ALIGNED(*addr, sizeof(long))) {
>
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
On Mon, Sep 25, 2023 at 09:53:16AM +0200, Clément Léger wrote:
>
>
> On 22/09/2023 21:22, 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 wrote:
> >>>
> Now lets look at big endian layout:
>
> uaddr = 0xbeef0004
> enabler = 1;
>
> memory at 0xbeef: 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 = 0xbeef
>
> ptr = kaddr + (uaddr & ~PAGE_MASK);
>
> clear_bit(1 + 32, ptr);
>
> memory at 0xbeef: 00 00 00 00 00 00 00 02
> ^
> bit 33 of 0xbeef
>
> 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 ?
> >>
> >
> > I have a ppc 64bit BE VM I've been validating this on. If we do the
> > shifting within user_events (vs a generic set_bit_aligned approach)
> > 64bit BE does not need additional bit manipulation. However, if we were
> > to blindly pass the address and bit as is to set_bit_aligned() it
> > assumes the bit number is for a long, not a 32 bit word. So for that
> > approach we would need to offset the bit in the unaligned case.
> >
> > Here's a patch I have that I've validated on ppc64 BE, aarch64 LE, and
> > x86_64 LE. I personally feel more comfortable with this approach than
> > the generic set_bit_aligned() one.
> >
> > Thanks,
> > -Beau
> >
> > diff --git a/kernel/trace/trace_events_user.c
> > b/kernel/trace/trace_events_user.c
> > index e3f2b8d72e01..ae854374d0b7 100644
> > --- a/kernel/trace/trace_events_user.c
> > +++ b/kernel/trace/trace_events_user.c
> > @@ -162,6 +162,23 @@ struct user_event_validator {
> > int flags;
> > };
> >
> > +static inline void align_addr_bit(unsigned long *addr, int *bit)
> > +{
> > + if (IS_ALIGNED(*addr, sizeof(long)))
> > + return;
> > +
> > + *addr = ALIGN_DOWN(*addr, sizeof(long));
> > +
> > + /*
> > +* We only support 32 and 64 bit values. The only time we need
> > +* to align is a 32 bit value on a 64 bit kernel, which on LE
> > +* is always 32 bits, and on BE requires no change.
> > +*/
> > +#ifdef __LITTLE_ENDIAN
> > + *bit += 32;
> > +#endif
>
> Hi Beau, except the specific alignment that is basically what I ended up
> with for the BE 64bit case (ie just bit += 32). Regarding the generic
> alignment, depends on what the maintainers wishes (generic of user_event
> specific). I also feel like this shoulmd be handle specifically for
> user_events which uses set_bit in some non standard way. Any suggestion ?
>
Looking at this deeper, user_events needs to track some of this
alignment requirements within each enabler. This is needed because when
enablers are disabled/removed they do not have the actual size. The size
is needed to know if we need to update the bits, etc. (IE: If originally
a 32-bit value was used and it's aligned and it's on BE, it needs the
bits shifted.)
My final version that I played around with looked like this for just the
alignment requirements:
+static inline void align_addr_bit(unsigned long *addr, int *bit,
+ unsigned long *flags)
+{
+ if (IS_ALIGNED(*addr, sizeof(long))) {
+#ifdef __BIG_ENDIAN
+ if (test_bit(ENABLE_VAL_32_BIT, flags))
+
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
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 wrote:
>>>
Now lets look at big endian layout:
uaddr = 0xbeef0004
enabler = 1;
memory at 0xbeef: 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 = 0xbeef
ptr = kaddr + (uaddr & ~PAGE_MASK);
clear_bit(1 + 32, ptr);
memory at 0xbeef: 00 00 00 00 00 00 00 02
^
bit 33 of 0xbeef
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-be...@linux.microsoft.com/
>
>> Clément
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
On 22/09/2023 21:22, 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 wrote:
>>>
Now lets look at big endian layout:
uaddr = 0xbeef0004
enabler = 1;
memory at 0xbeef: 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 = 0xbeef
ptr = kaddr + (uaddr & ~PAGE_MASK);
clear_bit(1 + 32, ptr);
memory at 0xbeef: 00 00 00 00 00 00 00 02
^
bit 33 of 0xbeef
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 ?
>>
>
> I have a ppc 64bit BE VM I've been validating this on. If we do the
> shifting within user_events (vs a generic set_bit_aligned approach)
> 64bit BE does not need additional bit manipulation. However, if we were
> to blindly pass the address and bit as is to set_bit_aligned() it
> assumes the bit number is for a long, not a 32 bit word. So for that
> approach we would need to offset the bit in the unaligned case.
>
> Here's a patch I have that I've validated on ppc64 BE, aarch64 LE, and
> x86_64 LE. I personally feel more comfortable with this approach than
> the generic set_bit_aligned() one.
>
> Thanks,
> -Beau
>
> diff --git a/kernel/trace/trace_events_user.c
> b/kernel/trace/trace_events_user.c
> index e3f2b8d72e01..ae854374d0b7 100644
> --- a/kernel/trace/trace_events_user.c
> +++ b/kernel/trace/trace_events_user.c
> @@ -162,6 +162,23 @@ struct user_event_validator {
> int flags;
> };
>
> +static inline void align_addr_bit(unsigned long *addr, int *bit)
> +{
> + if (IS_ALIGNED(*addr, sizeof(long)))
> + return;
> +
> + *addr = ALIGN_DOWN(*addr, sizeof(long));
> +
> + /*
> + * We only support 32 and 64 bit values. The only time we need
> + * to align is a 32 bit value on a 64 bit kernel, which on LE
> + * is always 32 bits, and on BE requires no change.
> + */
> +#ifdef __LITTLE_ENDIAN
> + *bit += 32;
> +#endif
Hi Beau, except the specific alignment that is basically what I ended up
with for the BE 64bit case (ie just bit += 32). Regarding the generic
alignment, depends on what the maintainers wishes (generic of user_event
specific). I also feel like this shoulmd be handle specifically for
user_events which uses set_bit in some non standard way. Any suggestion ?
Thanks,
Clément
> +}
> +
> typedef void (*user_event_func_t) (struct user_event *user, struct iov_iter
> *i,
> void *tpdata, bool *faulted);
>
> @@ -481,6 +498,7 @@ static int user_event_enabler_write(struct user_event_mm
> *mm,
> unsigned long *ptr;
> struct page *page;
> void *kaddr;
> + int bit = ENABLE_BIT(enabler);
> int ret;
>
> lockdep_assert_held(_mutex);
> @@ -496,6 +514,8 @@ static int user_event_enabler_write(struct user_event_mm
> *mm,
>test_bit(ENABLE_VAL_FREEING_BIT, ENABLE_BITOPS(enabler
> return -EBUSY;
>
> + align_addr_bit(, );
> +
> ret = pin_user_pages_remote(mm->mm, uaddr, 1, FOLL_WRITE | FOLL_NOFAULT,
> , NULL);
>
> @@ -514,9 +534,9 @@ static int user_event_enabler_write(struct user_event_mm
> *mm,
>
> /* Update bit atomically, user tracers must be atomic as
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
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 wrote:
> >
> >> Now lets look at big endian layout:
> >>
> >> uaddr = 0xbeef0004
> >> enabler = 1;
> >>
> >> memory at 0xbeef: 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 = 0xbeef
> >>
> >>ptr = kaddr + (uaddr & ~PAGE_MASK);
> >>
> >>clear_bit(1 + 32, ptr);
> >>
> >> memory at 0xbeef: 00 00 00 00 00 00 00 02
> >> ^
> >>bit 33 of 0xbeef
> >>
> >> 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).
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-be...@linux.microsoft.com/
> Clément
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
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 wrote:
> >
> >> Now lets look at big endian layout:
> >>
> >> uaddr = 0xbeef0004
> >> enabler = 1;
> >>
> >> memory at 0xbeef: 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 = 0xbeef
> >>
> >>ptr = kaddr + (uaddr & ~PAGE_MASK);
> >>
> >>clear_bit(1 + 32, ptr);
> >>
> >> memory at 0xbeef: 00 00 00 00 00 00 00 02
> >> ^
> >>bit 33 of 0xbeef
> >>
> >> 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 ?
>
I have a ppc 64bit BE VM I've been validating this on. If we do the
shifting within user_events (vs a generic set_bit_aligned approach)
64bit BE does not need additional bit manipulation. However, if we were
to blindly pass the address and bit as is to set_bit_aligned() it
assumes the bit number is for a long, not a 32 bit word. So for that
approach we would need to offset the bit in the unaligned case.
Here's a patch I have that I've validated on ppc64 BE, aarch64 LE, and
x86_64 LE. I personally feel more comfortable with this approach than
the generic set_bit_aligned() one.
Thanks,
-Beau
diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
index e3f2b8d72e01..ae854374d0b7 100644
--- a/kernel/trace/trace_events_user.c
+++ b/kernel/trace/trace_events_user.c
@@ -162,6 +162,23 @@ struct user_event_validator {
int flags;
};
+static inline void align_addr_bit(unsigned long *addr, int *bit)
+{
+ if (IS_ALIGNED(*addr, sizeof(long)))
+ return;
+
+ *addr = ALIGN_DOWN(*addr, sizeof(long));
+
+ /*
+* We only support 32 and 64 bit values. The only time we need
+* to align is a 32 bit value on a 64 bit kernel, which on LE
+* is always 32 bits, and on BE requires no change.
+*/
+#ifdef __LITTLE_ENDIAN
+ *bit += 32;
+#endif
+}
+
typedef void (*user_event_func_t) (struct user_event *user, struct iov_iter *i,
void *tpdata, bool *faulted);
@@ -481,6 +498,7 @@ static int user_event_enabler_write(struct user_event_mm
*mm,
unsigned long *ptr;
struct page *page;
void *kaddr;
+ int bit = ENABLE_BIT(enabler);
int ret;
lockdep_assert_held(_mutex);
@@ -496,6 +514,8 @@ static int user_event_enabler_write(struct user_event_mm
*mm,
test_bit(ENABLE_VAL_FREEING_BIT, ENABLE_BITOPS(enabler
return -EBUSY;
+ align_addr_bit(, );
+
ret = pin_user_pages_remote(mm->mm, uaddr, 1, FOLL_WRITE | FOLL_NOFAULT,
, NULL);
@@ -514,9 +534,9 @@ static int user_event_enabler_write(struct user_event_mm
*mm,
/* Update bit atomically, user tracers must be atomic as well */
if (enabler->event && enabler->event->status)
- set_bit(ENABLE_BIT(enabler), ptr);
+ set_bit(bit, ptr);
else
- clear_bit(ENABLE_BIT(enabler), ptr);
+ clear_bit(bit, ptr);
kunmap_local(kaddr);
unpin_user_pages_dirty_lock(, 1, true);
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
On 14/09/2023 19:29, Steven Rostedt wrote:
> On Thu, 14 Sep 2023 13:17:00 -0400
> Steven Rostedt wrote:
>
>> Now lets look at big endian layout:
>>
>> uaddr = 0xbeef0004
>> enabler = 1;
>>
>> memory at 0xbeef: 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 = 0xbeef
>>
>> ptr = kaddr + (uaddr & ~PAGE_MASK);
>>
>> clear_bit(1 + 32, ptr);
>>
>> memory at 0xbeef: 00 00 00 00 00 00 00 02
>> ^
>> bit 33 of 0xbeef
>>
>> 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 ?
Clément
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
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)
RE: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
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)
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
On 15/09/2023 04:54, Masami Hiramatsu (Google) wrote:
> On Thu, 14 Sep 2023 15:11:02 +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
>> ---
>> 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(, 1, true);
>> --
>> 2.40.1
>>
>
>
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
On Thu, 14 Sep 2023 15:11:02 +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
> ---
> 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(, 1, true);
> --
> 2.40.1
>
--
Masami Hiramatsu (Google)
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
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
>
> 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(, 1, true);
>> --
>> 2.40.1
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
On 14/09/2023 19:29, Steven Rostedt wrote:
> On Thu, 14 Sep 2023 13:17:00 -0400
> Steven Rostedt wrote:
>
>> Now lets look at big endian layout:
>>
>> uaddr = 0xbeef0004
>> enabler = 1;
>>
>> memory at 0xbeef: 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 = 0xbeef
>>
>> ptr = kaddr + (uaddr & ~PAGE_MASK);
>>
>> clear_bit(1 + 32, ptr);
>>
>> memory at 0xbeef: 00 00 00 00 00 00 00 02
>> ^
>> bit 33 of 0xbeef
>>
>> 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);
> }
Hi Steven,
Nice catch ! I don't have big endian at end but I'll check that.
Thanks,
>
> -- Steve
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
On Thu, 14 Sep 2023 13:17:00 -0400
Steven Rostedt wrote:
> Now lets look at big endian layout:
>
> uaddr = 0xbeef0004
> enabler = 1;
>
> memory at 0xbeef: 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 = 0xbeef
>
> ptr = kaddr + (uaddr & ~PAGE_MASK);
>
> clear_bit(1 + 32, ptr);
>
> memory at 0xbeef: 00 00 00 00 00 00 00 02
> ^
> bit 33 of 0xbeef
>
> 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
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
On Thu, 14 Sep 2023 15:11:02 +0200
Clément Léger wrote:
> @@ -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(, 1, true);
Does this work for big endian machines too?
Little endian layout:
uaddr = 0xbeef0004
enabler = 1;
memory at 0xbeef: 00 00 00 00 02 00 00 00
^
addr: 0xbeef0004
(enabler is set )
bitoffset = uaddr & (sizeof(unsigned long) - 1); bitoffset = 4
bit_offset *= 8; bitoffset = 32
uaddr &= ~(sizeof(unsigned long) - 1); uaddr = 0xbeef
ptr = kaddr + (uaddr & ~PAGE_MASK);
clear_bit(1 + 32, ptr);
memory at 0xbeef: 00 00 00 00 02 00 00 00
^
bit 33 of 0xbeef
Now lets look at big endian layout:
uaddr = 0xbeef0004
enabler = 1;
memory at 0xbeef: 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 = 0xbeef
ptr = kaddr + (uaddr & ~PAGE_MASK);
clear_bit(1 + 32, ptr);
memory at 0xbeef: 00 00 00 00 00 00 00 02
^
bit 33 of 0xbeef
I don't think that's what you expected!
-- Steve
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
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
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(, 1, true);
> --
> 2.40.1
