[PATCH 32/44] staging: unisys: Move channel creation up the stack

[Don Zickus]

On Sat, May 16, 2015 at 02:38:57PM +0300, Dan Carpenter wrote:
> On Wed, May 13, 2015 at 01:22:26PM -0400, Benjamin Romer wrote:
> > --- a/drivers/staging/unisys/visorbus/visorchipset.c
> > +++ b/drivers/staging/unisys/visorbus/visorchipset.c
> > @@ -1197,6 +1197,7 @@ bus_create(struct controlvm_message *inmsg)
> >  	u32 bus_no = cmd->create_bus.bus_no;
> >  	int rc = CONTROLVM_RESP_SUCCESS;
> >  	struct visorchipset_bus_info *bus_info;
> > +	struct visorchannel *visorchannel;
> >  
> >  	bus_info = bus_find(&bus_info_list, bus_no);
> >  	if (bus_info && (bus_info->state.created == 1)) {
> > @@ -1218,18 +1219,21 @@ bus_create(struct controlvm_message *inmsg)
> >  
> >  	POSTCODE_LINUX_3(BUS_CREATE_ENTRY_PC, bus_no, POSTCODE_SEVERITY_INFO);
> >  
> > -	if (inmsg->hdr.flags.test_message == 1)
> > -		bus_info->chan_info.addr_type = ADDRTYPE_LOCALTEST;
> > -	else
> > -		bus_info->chan_info.addr_type = ADDRTYPE_LOCALPHYSICAL;
> > -
> >  	bus_info->flags.server = inmsg->hdr.flags.server;
> > -	bus_info->chan_info.channel_addr = cmd->create_bus.channel_addr;
> > -	bus_info->chan_info.n_channel_bytes = cmd->create_bus.channel_bytes;
> > -	bus_info->chan_info.channel_type_uuid =
> > -			cmd->create_bus.bus_data_type_uuid;
> > -	bus_info->chan_info.channel_inst_uuid = cmd->create_bus.bus_inst_uuid;
> >  
> > +	visorchannel = visorchannel_create(cmd->create_bus.channel_addr,
> > +					   cmd->create_bus.channel_bytes,
> > +					   GFP_KERNEL,
> > +					   cmd->create_bus.bus_data_type_uuid);
> > +
> > +	if (!visorchannel) {
> > +		POSTCODE_LINUX_3(BUS_CREATE_FAILURE_PC, bus_no,
> > +				 POSTCODE_SEVERITY_ERR);
> > +		rc = -CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
> > +		kfree(bus_info);
> 
> 
> I'm in a very lazy review mood but I can't immediately see how this is
> correct.  We're calling kfree(bus_info), but the pointer is still there
> and bus_find() will return it to someone else.  Actually it's worse than
> that, because bus_find() will dereference it iterating through the
> &bus_info_list.

Thanks for the review, I am going to half disagree, see lines below. :-)

> 
> > +		goto cleanup;
> > +	}
> > +	bus_info->visorchannel = visorchannel;
> >  	list_add(&bus_info->entry, &bus_info_list);

bus_info gets attached to bus_info_list here, only on success.  So you
concern should never happen on the failure case.

What might be confusing you is the bus_info = bus_find() command above.
That is a sanity check to see if bus_info already exists.  If it does, the
function fails.  If not, bus_info is kzalloc'd and not attached to anything
yet.

That is my half disagreement. ;-)

The half that agrees with you, does show an error in this path.  What happens
is the 'goto cleanup' part calls bus_epilog.  But because bus_info is still
non-NULL, bus-epilog tries to treat it like valid data, which it isn't
anymore.

So I need to set bus_info to NULL after kfree'ing it.  I will respin for
that.

Cheers,
Don

> >  
> >  	POSTCODE_LINUX_3(BUS_CREATE_EXIT_PC, bus_no, POSTCODE_SEVERITY_INFO);
> 
> regards,
> dan carpenter