On Fri, Jul 25, 2025 at 06:10:51PM +0100, Matthew Wilcox wrote:
On Fri, Jul 25, 2025 at 06:47:01PM +0200, Vlastimil Babka wrote:
On 7/25/25 08:49, Li Qiong wrote:
For debugging, object_err() prints free pointer of the object. However, if check_valid_pointer() returns false for a object, dereferncing `object + s->offset` can lead to a crash. Therefore, print the object's address in such cases.
I don't know where this patch came from (was it cc'd to linux-mm? i don't see it)
https://lore.kernel.org/all/20250725024854.1201926-1-liqiong@nfschina.com
Looks like it's rejected by linux-mm for some reason..
+/*
- object - should be a valid object.
- check_valid_pointer(s, slab, object) should be true.
- */
This comment is very confusing. It tries to ape kernel-doc style, but if it were kernel-doc, the word before the hyphen should be the name of the function, and it isn't. If we did use kernel-doc for this, we'd use @object to denote that we're documenting the argument.
Yes, the comment is indeed confusing and agree with your point.
When I suggested it I expected adding something like:
/* print_trailer() may deref invalid freepointer if object pointer is invalid */ WARN_ON_ONCE(!check_valid_pointer(s, slab, object));
to be added to object_err().
But I don't see the need to pretend this is related to kernel-doc. This would be better:
/*
- 'object' must be a valid pointer into this slab. ie
- check_valid_pointer() would return true
*/
I'm sure better wording for that is possible ...
if (!check_valid_pointer(s, slab, object)) {
object_err(s, slab, object, "Freelist Pointer check fails");
slab_err(s, slab, "Invalid object pointer 0x%p", object);No, the error message is now wrong. It's not an object, it's the freelist pointer.
Because it's the object is about to be allocated, it will look like this:
object pointer -> obj: [ garbage ][ freelist pointer ][ garbage ]
SLUB uses check_valid_pointer() to check either 1) freelist pointer of an object is valid (e.g. in check_object()), or 2) an object pointer points to a valid address (e.g. in free_debug_processing()).
In this case it's an object pointer, not a freelist pointer. Or am I misunderstanding something?
slab_err(s, slab, "Invalid freelist pointer %p", object);(the 0x%p is wrong because it will print 0x twice)
"0x%p" is used all over the place in mm/slub.c.
In the printk documentation [1]:
Plain Pointers %p abcdef12 or 00000000abcdef12
0x%p should be 0xabcdef12 or 0x00000000abcdef12, no?
[1] https://www.kernel.org/doc/html/next/core-api/printk-formats.html#printk-spe...
But I think there are even more things wrong here. Like slab_err() is not nerely as severe as slab_bug(), which is what used to be called.
What do you mean by slab_err() is not as severe as slab_bug()? Both object_err() and slab_err() add a taint and trigger a WARNING.
And object_err() adds a taint, which this skips.
adding a taint is done via slab_err()->__slab_err()->add_taint()