[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230922192231.GA1828-beaub@linux.microsoft.com>
Date: Fri, 22 Sep 2023 12:22:31 -0700
From: Beau Belgrave <beaub@...ux.microsoft.com>
To: Clément Léger <cleger@...osinc.com>
Cc: Steven Rostedt <rostedt@...dmis.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: 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 <rostedt@...dmis.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 ?
>
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(&event_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(&uaddr, &bit);
+
ret = pin_user_pages_remote(mm->mm, uaddr, 1, FOLL_WRITE | FOLL_NOFAULT,
&page, 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(&page, 1, true);
Powered by blists - more mailing lists