Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment

[Beau Belgrave <beaub@linux.microsoft.com>]

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 ?
> 

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);