| Message ID | 84d7cd03-1cf8-401a-8edf-2524db0bd6d5@oppo.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/vmalloc: Fix return value check for vb_alloc | expand |
On Wed, Feb 28, 2024 at 9:51 PM 刘海龙(LaoLiu) <liuhailong@oppo.com> wrote: > > If vm_map_ram(page, 0, 0) would cause panic by vmap_pages_range_noflush, so > change IS_ERR to IS_ERR_OR_NULL to fix this. > > Signed-off-by: Hailong.Liu <liuhailong@oppo.com> > --- > mm/vmalloc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index d12a17fc0c17..109732006cf7 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -2387,7 +2387,7 @@ void *vm_map_ram(struct page **pages, unsigned int > count, int node) > > if (likely(count <= VMAP_MAX_ALLOC)) { > mem = vb_alloc(size, GFP_KERNEL); > - if (IS_ERR(mem)) > + if (IS_ERR_OR_NULL(mem)) it seems the only case for vb_alloc to return NULL is size = 0, isn't it a bug of caller? > return NULL; > addr = (unsigned long)mem; > } else { > -- > 2.34.1 Thanks Barry
On Wed, Feb 28, 2024 at 10:34 PM Barry Song <21cnbao@gmail.com> wrote: > > On Wed, Feb 28, 2024 at 9:51 PM 刘海龙(LaoLiu) <liuhailong@oppo.com> wrote: > > > > If vm_map_ram(page, 0, 0) would cause panic by vmap_pages_range_noflush, so > > change IS_ERR to IS_ERR_OR_NULL to fix this. > > > > Signed-off-by: Hailong.Liu <liuhailong@oppo.com> > > --- > > mm/vmalloc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > > index d12a17fc0c17..109732006cf7 100644 > > --- a/mm/vmalloc.c > > +++ b/mm/vmalloc.c > > @@ -2387,7 +2387,7 @@ void *vm_map_ram(struct page **pages, unsigned int > > count, int node) > > > > if (likely(count <= VMAP_MAX_ALLOC)) { > > mem = vb_alloc(size, GFP_KERNEL); > > - if (IS_ERR(mem)) > > + if (IS_ERR_OR_NULL(mem)) > > it seems the only case for vb_alloc to return NULL is size = 0, isn't > it a bug of > caller? what about the below? diff --git a/mm/vmalloc.c b/mm/vmalloc.c index 25a8df497255..640157221c95 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -2834,6 +2834,9 @@ void *vm_map_ram(struct page **pages, unsigned int count, int node) unsigned long addr; void *mem; + if (unlikely(count == 0)) + return NULL; + if (likely(count <= VMAP_MAX_ALLOC)) { mem = vb_alloc(size, GFP_KERNEL); if (IS_ERR(mem)) > > > return NULL; > > addr = (unsigned long)mem; > > } else { > > -- > > 2.34.1 Thanks Barry
On 2024/2/28 17:34, Barry Song wrote: > On Wed, Feb 28, 2024 at 9:51 PM 刘海龙(LaoLiu) <liuhailong@oppo.com> wrote: >> >> If vm_map_ram(page, 0, 0) would cause panic by vmap_pages_range_noflush, so >> change IS_ERR to IS_ERR_OR_NULL to fix this. >> >> Signed-off-by: Hailong.Liu <liuhailong@oppo.com> >> --- >> mm/vmalloc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/vmalloc.c b/mm/vmalloc.c >> index d12a17fc0c17..109732006cf7 100644 >> --- a/mm/vmalloc.c >> +++ b/mm/vmalloc.c >> @@ -2387,7 +2387,7 @@ void *vm_map_ram(struct page **pages, unsigned int >> count, int node) >> >> if (likely(count <= VMAP_MAX_ALLOC)) { >> mem = vb_alloc(size, GFP_KERNEL); >> - if (IS_ERR(mem)) >> + if (IS_ERR_OR_NULL(mem)) > > it seems the only case for vb_alloc to return NULL is size = 0, isn't > it a bug of > caller? vb_alloc had already checked the size == 0, so it should be return NULL to caller or not panic here. In fact, we encounter z_erofs_lz4_decompress issue. [54032.383633][T25392] vmap_pages_range_noflush+0x790/0x8f8 [54032.383637][T25392] vm_map_ram+0x1c8/0x10b0 [54032.383642][T25392] z_erofs_lz4_decompress+0x60/0x1e8 [54032.383648][T25392] z_erofs_decompress_pcluster+0x624/0x9fc [54032.383653][T25392] z_erofs_decompress_kickoff+0x18c/0x224 [54032.383658][T25392] z_erofs_decompressqueue_endio+0x1a8/0x1e0 [54032.383663][T25392] bio_endio+0x188/0x47c [54032.383667][T25392] clone_endio+0x1a0/0x550 [54032.383674][T25392] bio_endio+0x14c/0x47c [54032.383678][T25392] verity_work.60258+0x7c/0x13c [54032.383682][T25392] process_one_work+0x1b8/0xa98 [54032.383687][T25392] worker_thread+0x160/0x6c0 [54032.383691][T25392] kthread+0x15c/0x1d0 [54032.383696][T25392] ret_from_fork+0x10/0x20 z_erofs_lz4_decompress has checked the return value, so it's reasonable to return NULL if size == 0. Brs, Hailong. > >> return NULL; >> addr = (unsigned long)mem; >> } else { >> -- >> 2.34.1 > > Thanks > Barry
On Wed, Feb 28, 2024 at 11:02 PM 刘海龙(LaoLiu) <liuhailong@oppo.com> wrote: > > On 2024/2/28 17:34, Barry Song wrote: > > On Wed, Feb 28, 2024 at 9:51 PM 刘海龙(LaoLiu) <liuhailong@oppo.com> wrote: > >> > >> If vm_map_ram(page, 0, 0) would cause panic by vmap_pages_range_noflush, so > >> change IS_ERR to IS_ERR_OR_NULL to fix this. > >> > >> Signed-off-by: Hailong.Liu <liuhailong@oppo.com> > >> --- > >> mm/vmalloc.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/mm/vmalloc.c b/mm/vmalloc.c > >> index d12a17fc0c17..109732006cf7 100644 > >> --- a/mm/vmalloc.c > >> +++ b/mm/vmalloc.c > >> @@ -2387,7 +2387,7 @@ void *vm_map_ram(struct page **pages, unsigned int > >> count, int node) > >> > >> if (likely(count <= VMAP_MAX_ALLOC)) { > >> mem = vb_alloc(size, GFP_KERNEL); > >> - if (IS_ERR(mem)) > >> + if (IS_ERR_OR_NULL(mem)) > > > > it seems the only case for vb_alloc to return NULL is size = 0, isn't > > it a bug of > > caller? > vb_alloc had already checked the size == 0, so it should be return NULL > to caller or not panic here. > > In fact, we encounter z_erofs_lz4_decompress issue. > > [54032.383633][T25392] vmap_pages_range_noflush+0x790/0x8f8 > [54032.383637][T25392] vm_map_ram+0x1c8/0x10b0 > [54032.383642][T25392] z_erofs_lz4_decompress+0x60/0x1e8 > [54032.383648][T25392] z_erofs_decompress_pcluster+0x624/0x9fc > [54032.383653][T25392] z_erofs_decompress_kickoff+0x18c/0x224 > [54032.383658][T25392] z_erofs_decompressqueue_endio+0x1a8/0x1e0 > [54032.383663][T25392] bio_endio+0x188/0x47c > [54032.383667][T25392] clone_endio+0x1a0/0x550 > [54032.383674][T25392] bio_endio+0x14c/0x47c > [54032.383678][T25392] verity_work.60258+0x7c/0x13c > [54032.383682][T25392] process_one_work+0x1b8/0xa98 > [54032.383687][T25392] worker_thread+0x160/0x6c0 > [54032.383691][T25392] kthread+0x15c/0x1d0 > [54032.383696][T25392] ret_from_fork+0x10/0x20 > > z_erofs_lz4_decompress has checked the return value, so it's reasonable > to return NULL if size == 0. I agree. but there is no reason to activate a WARN_ON in vb_alloc as obviously it doesn't like it. so fix it earlier. I even feel z_erofs_lz4_decompress is a better place than vm_map_ram according to your description. but at least vm_map_ram is better than checking a vb_alloc's ret after it gives a parameter obviously hated and causes complaints. > > Brs, > Hailong. > > > >> return NULL; > >> addr = (unsigned long)mem; > >> } else { > >> -- > >> 2.34.1 > > Thanks Barry
diff --git a/mm/vmalloc.c b/mm/vmalloc.c index d12a17fc0c17..109732006cf7 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -2387,7 +2387,7 @@ void *vm_map_ram(struct page **pages, unsigned int count, int node) if (likely(count <= VMAP_MAX_ALLOC)) { mem = vb_alloc(size, GFP_KERNEL); - if (IS_ERR(mem)) + if (IS_ERR_OR_NULL(mem)) return NULL; addr = (unsigned long)mem;
If vm_map_ram(page, 0, 0) would cause panic by vmap_pages_range_noflush, so change IS_ERR to IS_ERR_OR_NULL to fix this. Signed-off-by: Hailong.Liu <liuhailong@oppo.com> --- mm/vmalloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) } else { -- 2.34.1