On 8/27/20 12:10 PM, Catalin Marinas wrote:

On Thu, Aug 27, 2020 at 11:31:56AM +0100, Vincenzo Frascino wrote:

quoted

On 8/27/20 10:38 AM, Catalin Marinas wrote:

quoted

On Fri, Aug 14, 2020 at 07:27:02PM +0200, Andrey Konovalov wrote:

quoted

+void * __must_check mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
+{
+	void *ptr = addr;
+
+	if ((!system_supports_mte()) || (size == 0))
+		return addr;
+
+	tag = 0xF0 | (tag & 0xF);
+	ptr = (void *)__tag_set(ptr, tag);
+	size = ALIGN(size, MTE_GRANULE_SIZE);

I think aligning the size is dangerous. Can we instead turn it into a
WARN_ON if not already aligned? At a quick look, the callers of
kasan_{un,}poison_memory() already align the size.

The size here is used only for tagging purposes and if we want to tag a
subgranule amount of memory we end up tagging the granule anyway. Why do you
think it can be dangerous?

In principle, I don't like expanding the size unless you are an
allocator. Since this code doesn't control the placement of the object
it was given, a warn seems more appropriate.

That's a good point. Ok, we can change this in a warning.

quoted

quoted

quoted

+/*
+ * Assign allocation tags for a region of memory based on the pointer tag
+ *   x0 - source pointer
+ *   x1 - size
+ *
+ * Note: size is expected to be MTE_GRANULE_SIZE aligned
+ */
+SYM_FUNC_START(mte_assign_mem_tag_range)
+	/* if (src == NULL) return; */
+	cbz	x0, 2f
+	/* if (size == 0) return; */

You could skip the cbz here and just document that the size should be
non-zero and aligned. The caller already takes care of this check.

I would prefer to keep the check here, unless there is a valid reason, since
allocate(0) is a viable option hence tag(x, 0) should be as well. The caller
takes care of it in one place, today, but I do not know where the API will be
used in future.

That's why I said just document it in the comment above the function.

The check is also insufficient if the size is not aligned to an MTE
granule, so it's not really consistent. This function should end with a
subs followed by b.gt as cbnz will get stuck in a loop for unaligned
size.

That's correct. Thanks for pointing this out. I currently used it only in places
where the caller took care to align the size. But in future we cannot know hence
we should harden the function with what you are suggesting.

-- 
Regards,
Vincenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel