linux-kernel - Re: [PATCH v3 2/3] resource: Add release_mem_region

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: <20130424084229.GB29191@ram.oc3035372033.ibm.com>
Date:	Wed, 24 Apr 2013 16:42:29 +0800
From:	Ram Pai <linuxram@...ibm.com>
To:	Toshi Kani <toshi.kani@...com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	David Rientjes <rientjes@...gle.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, guz.fnst@...fujitsu.com, tmac@...com,
	isimatu.yasuaki@...fujitsu.com, wency@...fujitsu.com,
	tangchen@...fujitsu.com, jiang.liu@...wei.com
Subject: Re: [PATCH v3 2/3] resource: Add release_mem_region_adjustable()

On Thu, Apr 11, 2013 at 10:30:02AM -0600, Toshi Kani wrote:
> On Wed, 2013-04-10 at 15:24 -0700, Andrew Morton wrote:
> > On Wed, 10 Apr 2013 15:08:29 -0700 (PDT) David Rientjes <rientjes@...gle.com> wrote:
> > 
> > > On Wed, 10 Apr 2013, Toshi Kani wrote:
> > > 
> > > > > I'll switch it to GFP_ATOMIC.  Which is horridly lame but the
> > > > > allocation is small and alternatives are unobvious.
> > > > 
> > > > Great!  Again, thanks for the update!
> > > 
> > > release_mem_region_adjustable() allocates at most one struct resource, so 
> > > why not do kmalloc(sizeof(struct resource), GFP_KERNEL) before taking 
> > > resource_lock and then testing whether it's NULL or not when splitting?  
> > > It unnecessarily allocates memory when there's no split, but 
> > > __remove_pages() shouldn't be a hotpath.
> > 
> > yup.
> > 
> > --- a/kernel/resource.c~resource-add-release_mem_region_adjustable-fix-fix
> > +++ a/kernel/resource.c
> > @@ -1046,7 +1046,8 @@ int release_mem_region_adjustable(struct
> >  			resource_size_t start, resource_size_t size)
> >  {
> >  	struct resource **p;
> > -	struct resource *res, *new;
> > +	struct resource *res;
> > +	struct resource *new_res;
> >  	resource_size_t end;
> >  	int ret = -EINVAL;
> >  
> > @@ -1054,6 +1055,9 @@ int release_mem_region_adjustable(struct
> >  	if ((start < parent->start) || (end > parent->end))
> >  		return ret;
> >  
> > +	/* The kzalloc() result gets checked later */
> > +	new_res = kzalloc(sizeof(struct resource), GFP_KERNEL);
> > +
> >  	p = &parent->child;
> >  	write_lock(&resource_lock);
> >  
> > @@ -1091,32 +1095,33 @@ int release_mem_region_adjustable(struct
> >  						start - res->start);
> >  		} else {
> >  			/* split into two entries */
> > -			new = kzalloc(sizeof(struct resource), GFP_ATOMIC);
> > -			if (!new) {
> > +			if (!new_res) {
> >  				ret = -ENOMEM;
> >  				break;
> >  			}
> > -			new->name = res->name;
> > -			new->start = end + 1;
> > -			new->end = res->end;
> > -			new->flags = res->flags;
> > -			new->parent = res->parent;
> > -			new->sibling = res->sibling;
> > -			new->child = NULL;
> > +			new_res->name = res->name;
> > +			new_res->start = end + 1;
> > +			new_res->end = res->end;
> > +			new_res->flags = res->flags;
> > +			new_res->parent = res->parent;
> > +			new_res->sibling = res->sibling;
> > +			new_res->child = NULL;
> >  
> >  			ret = __adjust_resource(res, res->start,
> >  						start - res->start);
> >  			if (ret) {
> > -				kfree(new);
> > +				kfree(new_res);
> >  				break;
> >  			}
> 
> The kfree() in the if-statement above is not necessary since kfree() is
> called before the return at the end.  That is, the if-statement needs to
> be:
> 	if (ret)
> 		break;
> 
> With this change, I confirmed that all my test cases passed (with all
> the config debug options this time :).  With the change:
> 
> Reviewed-by: Toshi Kani <toshi.kani@...com>

I am not confortable witht the assumption, that when a split takes
place, the children are assumed to be in the lower entry. Probably a
warning to that effect,  would help quickly
nail down the problem, if such a case does encounter ?

Otherwise this looks fine. Sorry for the delayed reply. Was out.

Reviewed-by: Ram Pai <linuxram@...ibm.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/