| Message ID | 20220202203036.744010-3-longman@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/4] lib/vsprintf: Avoid redundant work with 0 size | expand |
On 2/2/22 21:30, Waiman Long wrote: > The snprintf() function can return a length greater than the given > input size. That will require a check for buffer overrun after each > invocation of snprintf(). scnprintf(), on the other hand, will never > return a greater length. By using scnprintf() in selected places, we > can avoid some buffer overrun checks except after stack_depot_snprint() > and after the last snprintf(). > > Signed-off-by: Waiman Long <longman@redhat.com> > Acked-by: David Rientjes <rientjes@google.com> > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org> Looks like this will work, but note that if the purpose of patch 1/4 was that after the first scnprintf() that overflows the following calls will be short-cut thanks to passing the size as 0, AFAICS that won't work. Because scnprintf() returns the number without trailing zero, 'ret' will be 'count - 1' after the overflow, so 'count - ret' will be 1, never 0. > --- > mm/page_owner.c | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/mm/page_owner.c b/mm/page_owner.c > index 99e360df9465..28dac73e0542 100644 > --- a/mm/page_owner.c > +++ b/mm/page_owner.c > @@ -338,19 +338,16 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, > if (!kbuf) > return -ENOMEM; > > - ret = snprintf(kbuf, count, > + ret = scnprintf(kbuf, count, > "Page allocated via order %u, mask %#x(%pGg), pid %d, ts %llu ns, free_ts %llu ns\n", > page_owner->order, page_owner->gfp_mask, > &page_owner->gfp_mask, page_owner->pid, > page_owner->ts_nsec, page_owner->free_ts_nsec); > > - if (ret >= count) > - goto err; > - > /* Print information relevant to grouping pages by mobility */ > pageblock_mt = get_pageblock_migratetype(page); > page_mt = gfp_migratetype(page_owner->gfp_mask); > - ret += snprintf(kbuf + ret, count - ret, > + ret += scnprintf(kbuf + ret, count - ret, > "PFN %lu type %s Block %lu type %s Flags %pGp\n", > pfn, > migratetype_names[page_mt], > @@ -358,19 +355,14 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, > migratetype_names[pageblock_mt], > &page->flags); > > - if (ret >= count) > - goto err; > - > ret += stack_depot_snprint(handle, kbuf + ret, count - ret, 0); > if (ret >= count) > goto err; > > if (page_owner->last_migrate_reason != -1) { > - ret += snprintf(kbuf + ret, count - ret, > + ret += scnprintf(kbuf + ret, count - ret, > "Page has been migrated, last migrate reason: %s\n", > migrate_reason_names[page_owner->last_migrate_reason]); > - if (ret >= count) > - goto err; > } > > ret += snprintf(kbuf + ret, count - ret, "\n");
On 2/3/22 10:46, Vlastimil Babka wrote: > On 2/2/22 21:30, Waiman Long wrote: >> The snprintf() function can return a length greater than the given >> input size. That will require a check for buffer overrun after each >> invocation of snprintf(). scnprintf(), on the other hand, will never >> return a greater length. By using scnprintf() in selected places, we >> can avoid some buffer overrun checks except after stack_depot_snprint() >> and after the last snprintf(). >> >> Signed-off-by: Waiman Long <longman@redhat.com> >> Acked-by: David Rientjes <rientjes@google.com> >> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org> > Looks like this will work, but note that if the purpose of patch 1/4 was > that after the first scnprintf() that overflows the following calls will be > short-cut thanks to passing the size as 0, AFAICS that won't work. Because > scnprintf() returns the number without trailing zero, 'ret' will be 'count - > 1' after the overflow, so 'count - ret' will be 1, never 0. Yes, I am aware of that. Patch 1 is just a micro-optimization for the very rare case. Cheers, Longman
On Thu 2022-02-03 13:49:02, Waiman Long wrote: > On 2/3/22 10:46, Vlastimil Babka wrote: > > On 2/2/22 21:30, Waiman Long wrote: > > > The snprintf() function can return a length greater than the given > > > input size. That will require a check for buffer overrun after each > > > invocation of snprintf(). scnprintf(), on the other hand, will never > > > return a greater length. By using scnprintf() in selected places, we > > > can avoid some buffer overrun checks except after stack_depot_snprint() > > > and after the last snprintf(). > > > > > > Signed-off-by: Waiman Long <longman@redhat.com> > > > Acked-by: David Rientjes <rientjes@google.com> > > > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org> > > Looks like this will work, but note that if the purpose of patch 1/4 was > > that after the first scnprintf() that overflows the following calls will be > > short-cut thanks to passing the size as 0, AFAICS that won't work. Because > > scnprintf() returns the number without trailing zero, 'ret' will be 'count - > > 1' after the overflow, so 'count - ret' will be 1, never 0. > > Yes, I am aware of that. Patch 1 is just a micro-optimization for the very > rare case. In theory, we might micro-optimize also the case when "size == 1". Well, I am not sure if it is worth it. After all, the primary use-case is to print the message into a big-enough buffer. Lost information is a bigger problem than the speed ;-) Best Regards, Petr
diff --git a/mm/page_owner.c b/mm/page_owner.c index 99e360df9465..28dac73e0542 100644 --- a/mm/page_owner.c +++ b/mm/page_owner.c @@ -338,19 +338,16 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, if (!kbuf) return -ENOMEM; - ret = snprintf(kbuf, count, + ret = scnprintf(kbuf, count, "Page allocated via order %u, mask %#x(%pGg), pid %d, ts %llu ns, free_ts %llu ns\n", page_owner->order, page_owner->gfp_mask, &page_owner->gfp_mask, page_owner->pid, page_owner->ts_nsec, page_owner->free_ts_nsec); - if (ret >= count) - goto err; - /* Print information relevant to grouping pages by mobility */ pageblock_mt = get_pageblock_migratetype(page); page_mt = gfp_migratetype(page_owner->gfp_mask); - ret += snprintf(kbuf + ret, count - ret, + ret += scnprintf(kbuf + ret, count - ret, "PFN %lu type %s Block %lu type %s Flags %pGp\n", pfn, migratetype_names[page_mt], @@ -358,19 +355,14 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, migratetype_names[pageblock_mt], &page->flags); - if (ret >= count) - goto err; - ret += stack_depot_snprint(handle, kbuf + ret, count - ret, 0); if (ret >= count) goto err; if (page_owner->last_migrate_reason != -1) { - ret += snprintf(kbuf + ret, count - ret, + ret += scnprintf(kbuf + ret, count - ret, "Page has been migrated, last migrate reason: %s\n", migrate_reason_names[page_owner->last_migrate_reason]); - if (ret >= count) - goto err; } ret += snprintf(kbuf + ret, count - ret, "\n");