| Message ID | 20250116025005.55846-1-liuye@kylinos.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/vma: Add VM_WARN_ON for commit_merge | expand |
On Thu, 16 Jan 2025 10:50:05 +0800 Liu Ye <liuye@kylinos.cn> wrote: > Add VM_WARN_ON to prevent 'adjust' from accessing NULL pointers > when 'adjust' is NULL and 'expanded' is false or 'adj_start' is > not zero. > > ... > > --- a/mm/vma.c > +++ b/mm/vma.c > @@ -641,6 +641,7 @@ static int commit_merge(struct vma_merge_struct *vmg, > > init_multi_vma_prep(&vp, vmg->vma, adjust, remove, remove2); > > + VM_WARN_ON(!adjust && (!expanded || adj_start)); > VM_WARN_ON(vp.anon_vma && adjust && adjust->anon_vma && > vp.anon_vma != adjust->anon_vma); > This won't prevent a null deref. It will emit a warning which duplicates all the information which we're about to emit from the oops handler. Are there any reports of an oops from a NULL deref of `adjust'?
在 2025/1/16 12:01, Andrew Morton 写道: > On Thu, 16 Jan 2025 10:50:05 +0800 Liu Ye<liuye@kylinos.cn> wrote: > >> Add VM_WARN_ON to prevent 'adjust' from accessing NULL pointers >> when 'adjust' is NULL and 'expanded' is false or 'adj_start' is >> not zero. >> >> ... >> >> --- a/mm/vma.c >> +++ b/mm/vma.c >> @@ -641,6 +641,7 @@ static int commit_merge(struct vma_merge_struct *vmg, >> >> init_multi_vma_prep(&vp, vmg->vma, adjust, remove, remove2); >> >> + VM_WARN_ON(!adjust && (!expanded || adj_start)); >> VM_WARN_ON(vp.anon_vma && adjust && adjust->anon_vma && >> vp.anon_vma != adjust->anon_vma); >> > This won't prevent a null deref. It will emit a warning which > duplicates all the information which we're about to emit from the oops > handler. Yes, the accurate description should be that an oops warning message will be generated when the corresponding input parameter is illegal. This helps to find the problem. > Are there any reports of an oops from a NULL deref of `adjust'? This issue is not from any report yet, but by cppcheck tool only. mm/vma.c:652:29: warning: Possible null pointer dereference: adjust [nullPointer] vma_iter_config(vmg->vmi, adjust->vm_start + adj_start, ^ mm/vma.c:1072:24: note: Calling function 'commit_merge', 2nd argument 'NULL' value is 0 if (commit_merge(vmg, NULL, remove_next ? next : NULL, NULL, 0, true)) ^ mm/vma.c:652:29: note: Null pointer dereference vma_iter_config(vmg->vmi, adjust->vm_start + adj_start, ^ mm/vma.c:653:5: warning: Possible null pointer dereference: adjust [nullPointer] adjust->vm_end); ^ mm/vma.c:1072:24: note: Calling function 'commit_merge', 2nd argument 'NULL' value is 0 if (commit_merge(vmg, NULL, remove_next ? next : NULL, NULL, 0, true)) ^ mm/vma.c:653:5: note: Null pointer dereference adjust->vm_end); ^ Before calling commit_merge, the correct relationship between adjust, adj_start, and expanded must be ensured, such as the functions vma_merge_existing_range and vma_expand. Therefore, VM_WARN_ON is added inside the function to detect incorrect relationships. Of course, commit_merge is not used anywhere else at present, so adding VM_WARN_ON is just a suggestion.
Thanks for the patch! It's not possible for this scenario to happen in practice, so this check just adds confusion. There are two callers - the one from vma_expand() which unconditionally sets expand true, and the one from vma_merge_existing_range() (note that vma_merge_new_range() ultimately invokes vma_expand()). The vma_merge_existing_range() case will nearly always set expand, should a merge be indicated. In the one instance where it will not, adjust will always be set to a non-NULL value. Unfortunately this is super unclear, which is a big hint that refactoring is required - and it's something I've already started working on - so I will take this as a signal to increase the priority on this. So this has to be a no unfortunately, but thank you very much highlighting this as it clearly indicates the need to improve this, which is hugely valuable! Thanks, Lorenzo On Thu, Jan 16, 2025 at 10:50:05AM +0800, Liu Ye wrote: > Add VM_WARN_ON to prevent 'adjust' from accessing NULL pointers > when 'adjust' is NULL and 'expanded' is false or 'adj_start' is > not zero. > Signed-off-by: Liu Ye <liuye@kylinos.cn> > --- > mm/vma.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mm/vma.c b/mm/vma.c > index 31c9c6f51c9f..36b5ac675504 100644 > --- a/mm/vma.c > +++ b/mm/vma.c > @@ -641,6 +641,7 @@ static int commit_merge(struct vma_merge_struct *vmg, > > init_multi_vma_prep(&vp, vmg->vma, adjust, remove, remove2); > > + VM_WARN_ON(!adjust && (!expanded || adj_start)); > VM_WARN_ON(vp.anon_vma && adjust && adjust->anon_vma && > vp.anon_vma != adjust->anon_vma); > > -- > 2.25.1 >
diff --git a/mm/vma.c b/mm/vma.c index 31c9c6f51c9f..36b5ac675504 100644 --- a/mm/vma.c +++ b/mm/vma.c @@ -641,6 +641,7 @@ static int commit_merge(struct vma_merge_struct *vmg, init_multi_vma_prep(&vp, vmg->vma, adjust, remove, remove2); + VM_WARN_ON(!adjust && (!expanded || adj_start)); VM_WARN_ON(vp.anon_vma && adjust && adjust->anon_vma && vp.anon_vma != adjust->anon_vma);
Add VM_WARN_ON to prevent 'adjust' from accessing NULL pointers when 'adjust' is NULL and 'expanded' is false or 'adj_start' is not zero. Signed-off-by: Liu Ye <liuye@kylinos.cn> --- mm/vma.c | 1 + 1 file changed, 1 insertion(+)