| Message ID | 20191012102512.28051-1-pugaowei@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mm/mmap.c: use IS_ERR_VALUE to check return value of get_unmapped_area | expand |
On Sat 12-10-19 18:25:12, Gaowei Pu wrote: > get_unmapped_area already cover the offset_in_page() check and returned > with error ptr. So replace offset_in_page() with IS_ERR_VALUE(). I have to say that I found offset_in_page check quite unintuitive. It is a relict from the past I suspect. A newer code seems to use IS_ERR_VALUE. If we want to make the code consistent then I am for it. Could you check other users as well? git grep suggests checking kernel/events/uprobes.c, mm/mmap.c and mm/mremap.c. I would simply use the following for the changelog " get_unmapped_area returns an address or -errno on failure. Historically we have checked for the failure by offset_in_page() which is correct but quite hard to read. Newer code started using IS_ERR_VALUE which is much easier to read. Convert remaining users of offset_in_page as well. This shouldn't introduce any functional changes. " > Signed-off-by: Gaowei Pu <pugaowei@gmail.com> > > V2: fix compile warnning. > --- > mm/mmap.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index 7e8c3e8ae75f..6aebfeb6a486 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1430,7 +1430,7 @@ unsigned long do_mmap(struct file *file, unsigned long addr, > * that it represents a valid section of the address space. > */ > addr = get_unmapped_area(file, addr, len, pgoff, flags); > - if (offset_in_page(addr)) > + if (IS_ERR_VALUE(addr)) > return addr; > > if (flags & MAP_FIXED_NOREPLACE) { > @@ -2990,15 +2990,16 @@ static int do_brk_flags(unsigned long addr, unsigned long len, unsigned long fla > struct rb_node **rb_link, *rb_parent; > pgoff_t pgoff = addr >> PAGE_SHIFT; > int error; > + unsigned long mapped_addr; > > /* Until we need other flags, refuse anything except VM_EXEC. */ > if ((flags & (~VM_EXEC)) != 0) > return -EINVAL; > flags |= VM_DATA_DEFAULT_FLAGS | VM_ACCOUNT | mm->def_flags; > > - error = get_unmapped_area(NULL, addr, len, 0, MAP_FIXED); > - if (offset_in_page(error)) > - return error; > + mapped_addr = get_unmapped_area(NULL, addr, len, 0, MAP_FIXED); > + if (IS_ERR_VALUE(mapped_addr)) > + return mapped_addr; > > error = mlock_future_check(mm, mm->def_flags, len); > if (error) > -- > 2.23.0 >
On Tue, Oct 15, 2019 at 8:33 PM Michal Hocko <mhocko@suse.com> wrote: > On Sat 12-10-19 18:25:12, Gaowei Pu wrote: > > get_unmapped_area already cover the offset_in_page() check and returned > > with error ptr. So replace offset_in_page() with IS_ERR_VALUE(). > > I have to say that I found offset_in_page check quite unintuitive. It is > a relict from the past I suspect. A newer code seems to use > IS_ERR_VALUE. If we want to make the code consistent then I am for it. > Could you check other users as well? git grep suggests checking > kernel/events/uprobes.c, mm/mmap.c and mm/mremap.c. > I only noticed the offset_in_page check after returned from get_unmapped_area is redundant. So I just replace the places where get_unmapped_area returned. As you mentioned, IS_ERR_VALUE maybe the newer code to check the error ptr compared to offset_in_page which is really unintuitive and therefore I will replace them in mm/mmap.c and mm/mremap.c. > I would simply use the following for the changelog > " > get_unmapped_area returns an address or -errno on failure. Historically > we have checked for the failure by offset_in_page() which is correct but > quite hard to read. Newer code started using IS_ERR_VALUE which is much > easier to read. Convert remaining users of offset_in_page as well. > > This shouldn't introduce any functional changes. > " > > OK, I will use your changelog and resend a patch. Thanks. > > > 2.23.0 > > > > -- > Michal Hocko > SUSE Labs >
diff --git a/mm/mmap.c b/mm/mmap.c index 7e8c3e8ae75f..6aebfeb6a486 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1430,7 +1430,7 @@ unsigned long do_mmap(struct file *file, unsigned long addr, * that it represents a valid section of the address space. */ addr = get_unmapped_area(file, addr, len, pgoff, flags); - if (offset_in_page(addr)) + if (IS_ERR_VALUE(addr)) return addr; if (flags & MAP_FIXED_NOREPLACE) { @@ -2990,15 +2990,16 @@ static int do_brk_flags(unsigned long addr, unsigned long len, unsigned long fla struct rb_node **rb_link, *rb_parent; pgoff_t pgoff = addr >> PAGE_SHIFT; int error; + unsigned long mapped_addr; /* Until we need other flags, refuse anything except VM_EXEC. */ if ((flags & (~VM_EXEC)) != 0) return -EINVAL; flags |= VM_DATA_DEFAULT_FLAGS | VM_ACCOUNT | mm->def_flags; - error = get_unmapped_area(NULL, addr, len, 0, MAP_FIXED); - if (offset_in_page(error)) - return error; + mapped_addr = get_unmapped_area(NULL, addr, len, 0, MAP_FIXED); + if (IS_ERR_VALUE(mapped_addr)) + return mapped_addr; error = mlock_future_check(mm, mm->def_flags, len); if (error)
get_unmapped_area already cover the offset_in_page() check and returned with error ptr. So replace offset_in_page() with IS_ERR_VALUE(). Signed-off-by: Gaowei Pu <pugaowei@gmail.com> V2: fix compile warnning. --- mm/mmap.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)