Issue in count_mm_mlocked_page_nr() with commit afcf76df48f9 ("mm/mlock: use vma iterator and maple state instead of vma linked list")

Issue in count_mm_mlocked_page_nr() with commit afcf76df48f9 ("mm/mlock: use vma iterator and maple state instead of vma linked list")

[Lukas Bulwahn]

[-- Attachment #1: Type: text/plain, Size: 1165 bytes --]

Dear Matthew, dear Liam,

The commit afcf76df48f9 ("mm/mlock: use vma iterator and maple state instead
of vma linked list") refactors the function count_mm_mlocked_page_nr() in
mm/mlock.c, such that the special case:

        if (mm == NULL)
                mm = current->mm;

is now dead code. Before the refactoring, after this if statement, the
variable
mm was used further in vma = find_vma(mm, start) in the following line, but
now,
with the introduction of VMA_ITERATOR(vmi, mm, start) before the if
statement,
the assignment within the if statement is simply ignored.

I assume that:
- If the semantics of this function shall be preserved, the VMA_ITERATOR
macro
needs to move after the if statement.

- or, if the consideration of the special case on mm == NULL is actually
obsolete with this refactoring, it could be simply deleted.

I will keep it up to you how this issue is correctly resolved.

This was discovered due to tracking all dead stores that clang-analyzer
reports
on an x86 defconfig. The underlying issue here is more severe than "just a
dead
store in the kernel code base", but it is probably easy for you to resolve.

Best regards,

Lukas

[-- Attachment #2: Type: text/html, Size: 1331 bytes --]

Re: Issue in count_mm_mlocked_page_nr() with commit afcf76df48f9 ("mm/mlock: use vma iterator and maple state instead of vma linked list")

[Liam Howlett]

* Lukas Bulwahn <lukas.bulwahn@gmail.com> [220614 07:33]:
> Dear Matthew, dear Liam,
> 
> The commit afcf76df48f9 ("mm/mlock: use vma iterator and maple state instead
> of vma linked list") refactors the function count_mm_mlocked_page_nr() in
> mm/mlock.c, such that the special case:
> 
>         if (mm == NULL)
>                 mm = current->mm;
> 
> is now dead code. Before the refactoring, after this if statement, the variable
> mm was used further in vma = find_vma(mm, start) in the following line, but now,
> with the introduction of VMA_ITERATOR(vmi, mm, start) before the if statement,
> the assignment within the if statement is simply ignored.
> 
> I assume that:
> - If the semantics of this function shall be preserved, the VMA_ITERATOR macro
> needs to move after the if statement.
> 
> - or, if the consideration of the special case on mm == NULL is actually
> obsolete with this refactoring, it could be simply deleted.

I think this code was never necessary.  This statement was added in the
same commit that added the only user of the function [1], which passes
in current->mm as the argument.

> 
> I will keep it up to you how this issue is correctly resolved.
> 
> This was discovered due to tracking all dead stores that clang-analyzer reports
> on an x86 defconfig. The underlying issue here is more severe than "just a dead
> store in the kernel code base", but it is probably easy for you to resolve.
> 

Thanks for the report.  This code should be dropped.

[1] Commit ID 0cf2f6f6dc605

Regards,
Liam

Issue in count_mm_mlocked_page_nr() with commit afcf76df48f9 ("mm/mlock: use vma iterator and maple state instead of vma linked list")

[Lukas Bulwahn]

Dear Matthew, dear Liam,

The commit afcf76df48f9 ("mm/mlock: use vma iterator and maple state instead
of vma linked list") refactors the function count_mm_mlocked_page_nr() in
mm/mlock.c, such that the special case:

        if (mm == NULL)
                mm = current->mm;

is now dead code. Before the refactoring, after this if statement, the variable
mm was used further in vma = find_vma(mm, start) in the following line, but now,
with the introduction of VMA_ITERATOR(vmi, mm, start) before the if statement,
the assignment within the if statement is simply ignored.

I assume that:
- If the semantics of this function shall be preserved, the VMA_ITERATOR macro
needs to move after the if statement.

- or, if the consideration of the special case on mm == NULL is actually
obsolete with this refactoring, it could be simply deleted.

I will keep it up to you how this issue is correctly resolved.

This was discovered due to tracking all dead stores that clang-analyzer reports
on an x86 defconfig. The underlying issue here is more severe than "just a dead
store in the kernel code base", but it is probably easy for you to resolve.

Best regards,

Lukas

P.S.: Sorry, if you get this mail twice, the first time my email
client tricked me into unintentionally sending a HTML email rather
than a text email.