Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] reserve_module_tags() silently succeeds and returns a valid-looking pointer when mas_store() fails, corrupting the module tags allocation space. - [High] release_module_tags() ignores failures from mas_store(), permanently leaking the module's maple tree range. -- --- Patch [1]: [PATCH v4 1/2] alloc_tag: move release_module_tags() above reserve_module_tags() --- commit 96b4c3fa8241c380a13e2a15009ac553cfc71daa Author: Hao Ge <[email protected]> alloc_tag: move release_module_tags() above reserve_module_tags() release_module_tags() is a cleanup helper. reserve_module_tags() can also fail after storing the reservation in the maple tree, in which case it should call release_module_tags() to undo it. Move the helper above reserve_module_tags() so no forward declaration is needed. No functional change. > diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c > index e93e7fec1f066..96a9196635fea 100644 > --- a/mm/alloc_tag.c > +++ b/mm/alloc_tag.c > @@ -493,6 +493,52 @@ static int vm_module_tags_populate(void) > return 0; > } > > +static void release_module_tags(struct module *mod, bool used) > +{ [ ... ] > +release_area: > + mas_store(&mas, used ? &unloaded_mod : NULL); > + val = mas_prev_range(&mas, 0); > + if (val == &prepend_mod) > + mas_store(&mas, NULL); [Severity: High] This isn't a bug introduced by this patch, but does this ignore failures from mas_store()? Since mas_store() is called with the maple tree lock held, it forces an implicit GFP_NOWAIT allocation. If this allocation fails under memory pressure, mas_store() sets mas into an error state without modifying the tree. Because the return value is ignored, the next call mas_prev_range(&mas, 0) would immediately return NULL upon seeing the error state, causing the padding cleanup mas_store(&mas, NULL) to be skipped. Could this result in the module's maple tree range being permanently leaked because the original module pointer remains in the tree? > +out: > + mas_unlock(&mas); > +} > + > +static void *reserve_module_tags(struct module *mod, unsigned long size, > + unsigned int prepend, unsigned long align) > +{ [Severity: High] This isn't a bug introduced by this patch, but does reserve_module_tags() silently succeed when mas_store() fails? If mas_store(&mas, mod) fails due to memory pressure: if (mas_is_err(&mas)) { mas.index = pad_start; mas_erase(&mas); ret = ERR_PTR(xa_err(mas.node)); } Does mas_erase(&mas) destroy the error state in mas.node before it can be read? mas_erase() resets the maple state and walks the tree, overwriting the error pointer in mas.node with a valid tree node or NULL. The subsequent ret = ERR_PTR(xa_err(mas.node)) then reads this non-error node, resulting in ret = NULL. Since ret is not an error pointer, would IS_ERR(ret) evaluate to false, causing the function to incorrectly return success? [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
