linux-kernel - Re: [PATCH 1/2] drm: replace drm_[cm]alloc* by kvmalloc alternatives

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 17 May 2017 10:12:41 +0100
From:   Chris Wilson <chris@...is-wilson.co.uk>
To:     Michal Hocko <mhocko@...nel.org>
Cc:     dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, Daniel Vetter <daniel.vetter@...el.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Sean Paul <seanpaul@...omium.org>,
        David Airlie <airlied@...ux.ie>
Subject: Re: [PATCH 1/2] drm: replace drm_[cm]alloc* by kvmalloc alternatives

On Wed, May 17, 2017 at 11:03:50AM +0200, Michal Hocko wrote:
> On Wed 17-05-17 08:38:09, Chris Wilson wrote:
> > On Wed, May 17, 2017 at 08:55:08AM +0200, Michal Hocko wrote:
> > > From: Michal Hocko <mhocko@...e.com>
> > > 
> > > drm_[cm]alloc* has grown their own kvmalloc with vmalloc fallback
> > > implementations. MM has grown kvmalloc* helpers in the meantime. Let's
> > > use those because it a) reduces the code and b) MM has a better idea
> > > how to implement fallbacks (e.g. do not vmalloc before kmalloc is tried
> > > with __GFP_NORETRY).
> > > 
> > > drm_calloc_large needs to get __GFP_ZERO explicitly but it is the same
> > > thing as kvmalloc_array in principle.
> > > 
> > > Signed-off-by: Michal Hocko <mhocko@...e.com>
> > 
> > Just a little surprised that calloc_large users still exist.
> > 
> > Reviewed-by: Chris Wilson <chris@...is-wilson.co.uk>
> 
> Thanks!
> 
> > One more feature request from mm, can we have the 
> > 	if (size != 0 && n > SIZE_MAX / size)
> > check exported by itself.
> 
> What do you exactly mean by exporting?

Just make available to others so that little things like choice between
SIZE_MAX and ULONG_MAX are consistent and actually reflect the right
limit (as dictated by kmalloc/kvmalloc/vmalloc...).

> Something like the following?
> I haven't compile tested it outside of mm with different config options.
> Sticking alloc_array_check into mm_types.h is kind of gross but I do not
> have a great idea where to put it. A new header doesn't seem nice.
> ---
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 7cb17c6b97de..f908b14ffc4c 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -534,7 +534,7 @@ static inline void *kvzalloc(size_t size, gfp_t flags)
>  
>  static inline void *kvmalloc_array(size_t n, size_t size, gfp_t flags)
>  {
> -	if (size != 0 && n > SIZE_MAX / size)
> +	if (!alloc_array_check(n, size))
>  		return NULL;
>  
>  	return kvmalloc(n * size, flags);
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 45cdb27791a3..d7154b43a0d1 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -601,4 +601,10 @@ typedef struct {
>  	unsigned long val;
>  } swp_entry_t;
>  
> +static inline bool alloc_array_check(size_t n, size_t size)
> +{
> +	if (size != 0 && n > SIZE_MAX / size)
> +		return false;
> +	return true;

Just return size == 0 || n <= SIZE_MAX /size ?

Whether or not size being 0 makes for a sane user is another question.
The guideline is that size is the known constant from sizeof() or
whatever and n is the variable number to allocate.

But yes, that inline is what I want :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre