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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20191107180751.GA3846@google.com>
Date:   Thu, 7 Nov 2019 13:07:51 -0500
From:   Joel Fernandes <joel@...lfernandes.org>
To:     Petr Mladek <pmladek@...e.com>
Cc:     linux-kernel@...r.kernel.org, Ioannis Ilkos <ilkos@...gle.com>,
        minchan@...gle.com, primiano@...gle.com, fmayer@...gle.com,
        hjd@...gle.com, joaodias@...gle.com, lalitm@...gle.com,
        rslawik@...gle.com, sspatil@...gle.com, timmurray@...gle.com,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Changbin Du <changbin.du@...el.com>,
        Ingo Molnar <mingo@...hat.com>, Joe Perches <joe@...ches.com>,
        Kees Cook <keescook@...omium.org>, linux-mm@...ck.org,
        Michal Hocko <mhocko@...e.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH] rss_stat: Add support to detect RSS updates of external
 mm

On Wed, Nov 06, 2019 at 09:59:59AM +0100, Petr Mladek wrote:
> On Tue 2019-11-05 21:44:51, Joel Fernandes (Google) wrote:
> > Also vsprintf.c is refactored a bit to allow reuse of hashing code.
> 
> I agree with Sergey that it would make sense to move this outside
> vsprintf.c.

I am of the opinion that its Ok to have it this way and I am not sure if
another translation unit is worth it just for this. If we have more users,
then yes we can consider splitting into its own translation unit at that
time.

If Andrew Morton objects, then I'll do it since the intention was for this
patch to go through his tree and I believe he is Ok with it this way.

> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index dee8fc467fcf..401baaac1813 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -761,11 +761,34 @@ static int __init initialize_ptr_random(void)
> >  early_initcall(initialize_ptr_random);
> >  
> >  /* Maps a pointer to a 32 bit unique identifier. */
> > +int ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
> > +{
> > +	const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
> 
> str is unused.

I believe Andrew has already fixed this in his tree.

linux-next has local variable removed now:
7422993b4f8e ("rss_stat-add-support-to-detect-rss-updates-of-external-mm-fix")

> > +	unsigned long hashval;
> 
> IMHO, this local variable unnecessarily complicates the code.
> I would use the pointer directly and let compiler to optimize.

Agreed.

thanks,

 - Joel


> > +	if (static_branch_unlikely(&not_filled_random_ptr_key))
> > +		return -EAGAIN;
> > +
> > +#ifdef CONFIG_64BIT
> > +	hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
> > +	/*
> > +	 * Mask off the first 32 bits, this makes explicit that we have
> > +	 * modified the address (and 32 bits is plenty for a unique ID).
> > +	 */
> > +	hashval = hashval & 0xffffffff;
> > +#else
> > +	hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
> > +#endif
> > +	*hashval_out = hashval;
> > +	return 0;
> > +}
> 
> Best Regards,
> Petr

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ