| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <YCPWypWctDMGWUpA@alley>
Date: Wed, 10 Feb 2021 13:51:22 +0100
From: Petr Mladek <pmladek@...e.com>
To: Yafang Shao <laoar.shao@...il.com>
Cc: Matthew Wilcox <willy@...radead.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
David Hildenbrand <david@...hat.com>,
Miaohe Lin <linmiaohe@...wei.com>,
Vlastimil Babka <vbabka@...e.cz>,
Christoph Lameter <cl@...ux.com>, penberg@...nel.org,
David Rientjes <rientjes@...gle.com>, iamjoonsoo.kim@....com,
Andrew Morton <akpm@...ux-foundation.org>,
Steven Rostedt <rostedt@...dmis.org>,
Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
Joe Perches <joe@...ches.com>, Linux MM <linux-mm@...ck.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 3/3] vsprintf: dump full information of page flags in
pGp
On Wed 2021-02-10 00:21:37, Yafang Shao wrote:
> On Tue, Feb 9, 2021 at 9:53 PM Petr Mladek <pmladek@...e.com> wrote:
> >
> > On Tue 2021-02-09 18:56:13, Yafang Shao wrote:
> > > Currently the pGp only shows the names of page flags, rather than
> > > the full information including section, node, zone, last cpupid and
> > > kasan tag. While it is not easy to parse these information manually
> > > because there're so many flavors. Let's interpret them in pGp as well.
> > >
> > > To be compitable with the existed format of pGp, the new introduced ones
> > > also use '|' as the separator, then the user tools parsing pGp won't
> > > need to make change, suggested by Matthew. The new information is
> > > tracked onto the end of the existed one.
> > >
> > > On example of the output in mm/slub.c as follows,
> > > - Before the patch,
> > > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head)
> > >
> > > - After the patch,
> > > [ 8838.835456] Slab 0x000000002828b78a objects=33 used=3 fp=0x00000000d04efc88 flags=0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
> > >
> > > The documentation and test cases are also updated. The output of the
> > > test cases as follows,
> > > [ 501.485081] test_printf: loaded.
> > > [ 501.485768] test_printf: all 388 tests passed
> > > [ 501.488762] test_printf: unloaded.
> > >
> >
> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > index 14c9a6af1b23..3f26611adb34 100644
> > > --- a/lib/vsprintf.c
> > > +++ b/lib/vsprintf.c
> > > @@ -1916,6 +1916,66 @@ char *format_flags(char *buf, char *end, unsigned long flags,
> > > return buf;
> > > }
> > >
> > > +struct page_flags_layout {
> > > + int width;
> > > + int shift;
> > > + int mask;
> > > + const struct printf_spec *spec;
> > > + const char *name;
> > > +};
> > > +
> > > +static const struct page_flags_layout pfl[] = {
> > > + {SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK,
> > > + &default_dec_spec, "section"},
> > > + {NODES_WIDTH, NODES_PGSHIFT, NODES_MASK,
> > > + &default_dec_spec, "node"},
> > > + {ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK,
> > > + &default_dec_spec, "zone"},
> > > + {LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK,
> > > + &default_flag_spec, "lastcpupid"},
> > > + {KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK,
> > > + &default_flag_spec, "kasantag"},
> > > +};
> > > +
> > > +static
> > > +char *format_page_flags(char *buf, char *end, unsigned long flags)
> > > +{
> > > + DECLARE_BITMAP(mask, ARRAY_SIZE(pfl));
> > > + unsigned long last;
> > > + int i;
> > > +
> > > + if (flags & (BIT(NR_PAGEFLAGS) - 1)) {
> > > + if (buf < end)
> > > + *buf = '|';
> > > + buf++;
> > > + }
> >
> > This is far from obvious. You print '|' here because you printed
> > something somewhere else. See below.
> >
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(pfl); i++)
> > > + __assign_bit(i, mask, pfl[i].width);
> >
> > The bitmap looks like an overkill. If I get it correctly, it is a
> > tricky way to handle only flags defined by the used build
> > configuration. See below.
> >
> > > + last = find_last_bit(mask, ARRAY_SIZE(pfl));
> > > +
> > > + for_each_set_bit(i, mask, ARRAY_SIZE(pfl)) {
> > > + /* Format: Flag Name + '=' (equals sign) + Number + '|' (separator) */
> > > + buf = string(buf, end, pfl[i].name, *pfl[i].spec);
> > > +
> > > + if (buf < end)
> > > + *buf = '=';
> > > + buf++;
> > > + buf = number(buf, end, (flags >> pfl[i].shift) & pfl[i].mask,
> > > + *pfl[i].spec);
> > > +
> > > + /* No separator for the last entry */
> > > + if (i != last) {
> > > + if (buf < end)
> > > + *buf = '|';
> > > + buf++;
> > > + }
> > > + }
> > > +
> > > + return buf;
> > > +}
> > > +
> > > static noinline_for_stack
> > > char *flags_string(char *buf, char *end, void *flags_ptr,
> > > struct printf_spec spec, const char *fmt)
> > > @@ -1929,10 +1989,10 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
> > > switch (fmt[1]) {
> > > case 'p':
> > > flags = *(unsigned long *)flags_ptr;
> > > - /* Remove zone id */
> > > - flags &= (1UL << NR_PAGEFLAGS) - 1;
> > > names = pageflag_names;
> >
> > The "names" variable is needed only with "break;" when using the final
> > format_flags(buf, end, flags, names);
> >
> > > - break;
> > > + buf = format_flags(buf, end, flags & (BIT(NR_PAGEFLAGS) - 1), names);
> > > + buf = format_page_flags(buf, end, flags);
> >
> > I am sorry for my ignorance. I am not familiar with MM.
> > But it is pretty hard to understand what call does what.
> >
> > I have found the following comment in include/linux/page_flags.h:
> >
> > * The page flags field is split into two parts, the main flags area
> > * which extends from the low bits upwards, and the fields area which
> > * extends from the high bits downwards.
> >
> > Sigh, I know that you already reworked this several times because
> > people "nitpicked" about the code style. But it seems that it
> > rather diverged instead of converged.
> >
> > What about the following?
> >
> > Note: It is inpired by the names "main area" and "fields area"
> > mentioned in the above comment from page_flags.h.
> > I have later realized that "page_flags_layout" actually made
> > sense as well. Feel free to rename page_flags_fileds
> > back to page_flags_layout.
> >
> > Anyway, this is my proposal:
> >
>
> This proposal is similar to v2.
> I don't mind changing it back with your additional better naming.
Great.
> By the way, it will be better to make a little change per Joe's
> suggestion on v2 that using a pointer instead of the index, for
> example,
>
> for (p = pff; p < pff + ARRAY_SIZE(pff); p++) {
This looks a bit non-standard. IMHO, Joe was not against using index.
He proposed:
for (i = 0; i < ARRAY_SIZE(pfl) && buf < end; i++) {
, see
https://lore.kernel.org/lkml/e5ea9e8b1190c2a397a1b84dd55bb9c706dc7058.camel@perches.com/
I am not sure about the (buf < end) check. It might be some
optimization or it did fit the the old code.
Anyway, I like the currently used:
for (i = 0; i < ARRAY_SIZE(pff); i++) {
It is standard, easy to understand, and thus more safe. I am sure that
compiler will optimize it very well.
Best Regards,
Petr
Powered by blists - more mailing lists