On 06/03/2020 11:20, Alexandru Elisei wrote:
Hi,
replying here after reviewing the v3 patch, and still seeing the problem.
> On 1/30/20 2:51 PM, Andre Przywara wrote:
>> On Thu, 23 Jan 2020 13:47:50 +0000
>> Alexandru Elisei <alexandru.elisei@xxxxxxx> wrote:
>>
>> Hi,
>>
>>> Don't ignore an error in the bus specific initialization function in
>>> virtio_init; don't ignore the result of virtio_init; and don't return 0
>>> in virtio_blk__init and virtio_scsi__init when we encounter an error.
>>> Hopefully this will save some developer's time debugging faulty virtio
>>> devices in a guest.
>> Seems like the right thing to do, but I was wondering how you triggered this? AFAICS virtio_init only fails when calloc() fails or you pass an illegal transport, with the latter looking like being hard coded to one of the two supported.
>
> I haven't triggered it. I found it by inspection. The transport-specific
> initialization functions can fail for various reasons (ioport_register or
> kvm__register_mmio can fail because some device emulation claimed all the MMIO
> space or the MMIO space was configured incorrectly in the kvm-arch.h header file;
> or memory allocation failed, etc) and this is the reason they return an int.
> Because of this, virtio_init can fail and this is the reason it too returns an
> int. It makes sense to check that the protocol that your device uses is actually
> working.
>
>>
>> One minor thing below ...
>
> [..]
>
>>> diff --git a/virtio/net.c b/virtio/net.c
>>> index 091406912a24..425c13ba1136 100644
>>> --- a/virtio/net.c
>>> +++ b/virtio/net.c
>>> @@ -910,7 +910,7 @@ done:
>>>
>>> static int virtio_net__init_one(struct virtio_net_params *params)
>>> {
>>> - int i, err;
>>> + int i, r;
>>> struct net_dev *ndev;
>>> struct virtio_ops *ops;
>>> enum virtio_trans trans = VIRTIO_DEFAULT_TRANS(params->kvm);
>>> @@ -920,10 +920,8 @@ static int virtio_net__init_one(struct virtio_net_params *params)
>>> return -ENOMEM;
>>>
>>> ops = malloc(sizeof(*ops));
>>> - if (ops == NULL) {
>>> - err = -ENOMEM;
>>> - goto err_free_ndev;
>>> - }
>>> + if (ops == NULL)
>>> + return -ENOMEM;
>> Doesn't that leave struct net_dev allocated? I am happy with removing the goto, but we should free(ndev) before we return, I think.
>
> Nope, the cleanup routine in virtio_net__exit takes care of deallocating it (you
> get there from virtio_net__init if virtio_net__init_one fails).
First, I don't see where we actually deallocate the struct net_dev
storage for each network device in __exit() - it seems to only call the
downscript, if needed, but frees nothing.
But more importantly, even that would only happen if this structure
would be already part of the list, which happens only *after* the check
for the ops malloc() return value. If we return prematurely due to this
malloc() failing, the ndev pointer is lost on the stack.
So I guess you need to free this here. As mentioned, you should still
drop the goto, since there is only one user.
Cheers,
Andre.
