Re: [PATCH v2] zram: fix possible use after free in zcomp_create()

[Minchan Kim <minchan@kernel.org>]

On Tue, Sep 08, 2015 at 10:20:38AM +0900, Sergey Senozhatsky wrote:
> On (09/08/15 10:07), Minchan Kim wrote:
> [..]
> > > +	int ret;
> > 
> > For the clarification, I want to call it as 'error' instead of ret.
> > 
> > >  
> > >  	backend = find_backend(compress);
> > >  	if (!backend)
> > > @@ -347,10 +348,10 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
> > >  
> > >  	comp->backend = backend;
> > >  	if (max_strm > 1)
> > > -		zcomp_strm_multi_create(comp, max_strm);
> > > +		ret = zcomp_strm_multi_create(comp, max_strm);
> > >  	else
> > > -		zcomp_strm_single_create(comp);
> > > -	if (!comp->stream) {
> > > +		ret = zcomp_strm_single_create(comp);
> > > +	if (ret) {
> > >  		kfree(comp);
> > >  		return ERR_PTR(-ENOMEM);
> > >  	}
> > 
> > And we could return ERR_PTR(error) rather than fixed -ENOMEM to propagate
> 
> yep, I thought about that; looks to be minor so I didn't insist
> on changing that. don't mind to change it to ERR_PTR(error).

Thanks.

> 
> > other errors potentially could be happen in future(ex, crypto support).
> > Of course, we should change description of the function about error return.
> 
> on the other hand, this is zcomp_strm_FOO_create(), which is mostly
> about memory allocation. but yeah, who knows. We can change this as
> a part of crypto api rework; ERR_PTR(error) does not fix anything
> per se, so may be we can avoid pushing this change into stable.

Strictly speaking, you're right. I asked two things bug fixing and
refactoring. But I thought it is really trivial enough to push
-stable.

With bonus, it's more readable(ie, error naming) and makes sense(
ie, not assume under layer function always fails with no memory).

So, hope to fix it altogether if anyone isn't strong against that.

Thank you.