Re: [PATCH v3 kvmtool 14/32] virtio: Don't ignore initialization failures

From: Alexandru Elisei <alexandru.elisei@arm.com>
To: "André Przywara" <andre.przywara@arm.com>, kvm@vger.kernel.org
Cc: will@kernel.org, julien.thierry.kdev@gmail.com,
	sami.mujawar@arm.com, lorenzo.pieralisi@arm.com
Subject: Re: [PATCH v3 kvmtool 14/32] virtio: Don't ignore initialization failures
Date: Tue, 14 Apr 2020 11:27:17 +0100	[thread overview]
Message-ID: <7a7a8443-4122-4f35-bdb1-cf1326aa2ac7@arm.com> (raw)
In-Reply-To: <9cb9e14b-ab07-48ec-819f-11fe727c88b1@arm.com>

Hi,

On 3/30/20 10:30 AM, André Przywara wrote:
> Hi,
>
> On 26/03/2020 15:24, Alexandru Elisei wrote:
>> 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.
>>
>> To take advantage of the cleanup function virtio_blk__exit, we have
>> moved appending the new device to the list before the call to
>> virtio_init.
>>
>> To safeguard against this in the future, virtio_init has been annoted
>> with the compiler attribute warn_unused_result.
> This is a good idea, but actually triggers an unrelated, long standing
> bug in vesa.c (on x86):
> hw/vesa.c: In function ‘vesa__init’:
> hw/vesa.c:77:3: error: ignoring return value of ‘ERR_PTR’, declared with
> attribute warn_unused_result [-Werror=unused-result]
>    ERR_PTR(-errno);
>    ^
> cc1: all warnings being treated as errors
>
> So could you add the missing "return" statement in that line, to fix
> that bug?
> I see that this gets rectified two patches later, but for the sake of
> bisect-ability it would be good to keep this compilable all the way through.

You're right, I'll fix it here.

>
>> 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;
>>  
>>  	list_add_tail(&ndev->list, &ndevs);
> As mentioned in the reply to the v2 version, I think this is still
> leaking memory here.

Yes, the leak is there. I didn't realize that virtio_net__exit doesn't do any
free'ing at all.

I find it a bit strange that we copy net_dev_virtio_ops for each device instance.
I suspect that passing the pointer to the static struct to virtio_init would have
been enough (this is what blk does), but I might be missing something. I don't
want to introduce a regression, so I'll keep the memory allocation and free it on
error.

Thanks,
Alex