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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
