From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759639AbcIMRbB (ORCPT ); Tue, 13 Sep 2016 13:31:01 -0400 Received: from mout.web.de ([212.227.15.3]:64630 "EHLO mout.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755434AbcIMRa7 (ORCPT ); Tue, 13 Sep 2016 13:30:59 -0400 Subject: Re: virtio_blk: Less function calls in init_vq() after error detection To: =?UTF-8?Q?Christian_Borntr=c3=a4ger?= , virtualization@lists.linux-foundation.org, "Michael S. Tsirkin" , Minfei Huang , Cornelia Huck , Stefan Hajnoczi References: <566ABCD9.1060404@users.sourceforge.net> <02054675-8395-ac81-6863-e3a5cbfc9032@users.sourceforge.net> Cc: Julia Lawall , kernel-janitors@vger.kernel.org, LKML , Chao Fan From: SF Markus Elfring Message-ID: <7da823eb-939c-9ee6-32bf-db296e6a96f6@users.sourceforge.net> Date: Tue, 13 Sep 2016 19:30:27 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-Provags-ID: V03:K0:vQFYUWc6o9ZAzQ2joNg3uDGj4NBP95GZ9XK6WUYh4axUmDaXeif wCwP24LKl8kaofIVwh6yQKMS64RgxEtric9/ENkFYyN2s6q2yqV52+xa4yAwWayGrxp9Pck jykKbJlvm4fgFYRUnDHYKNsCTNvCx18+Eu69oMz4EYgVv4sBekQc5vec2zE9FlovcFDK2bP UEFlkSL0gASM+E7scdQYw== X-UI-Out-Filterresults: notjunk:1;V01:K0:IjQshRoL1UE=:ty3sCAMLUFgv4OcZSqeiKs P/67sykQhzfBIo0Dwd6rGkOCWkTbxPkeokw4pfzub55+JYTdwn9Lrm8ckIy9MpxyvyxB9sNPS e14aJb8nA/wYqoUSOeqMGqlTSe3YKyg3lhDz/1Pa6P29nhwf3oOdkOkcE43HlgLHeMRnasc9Z jWUiyNE5jzMA1DIAQ82OpWgKbn6etH5sirJJ4ECVDPdErlakxtelJ9v8ebq7h8Vt5DX911pZl EiFJVAuNnbON8LZvT3u09xmWHtgt+84VMfdt884Y69NtlmoTX3KZPzrm99fyPHIRbHPfstbEK yY5lwmYk36stZ0wjm0Tp5TtN3wdPDSXl6gVFlY15BFKR9Zp2UtV+lVxU2kDp0cHyXdwl1rPW1 Ld97XQNjStFEz61zVJquAnYKJ4iQ+f0GNVR7ZbZeBOCbdNJ1RrRF4wV2odITvViv+eAsBnpFe IHOaJLp0NKlwR4uKmqED/MqMRhh62HDU2s8LE21+u7pq+5Z6ZvVxEitkza39UsFtnQ02fRaYY RRzg6y9yxD5my4BX26svfMnvxwNObOauxv1voVWMCFaTRCyCgGY2gLMVBYqBv3tr89CfGwlbj Nmv/sj0zcblQJpQnD8PSuggD4SGWc7PHpYNcL3R2nG8Aq5Lk+irzmLDGL6Pl4RluYz17dzY1E wVMgCmADXm5MXwRn3UqxNwmwdHHO5nYJxWBQn+KPvjzwNUbv3AXlQv1mawQc0Xy+AtPWxuqVB 1cFwX6gbIng5C/yFiKclmrzOIHBYjy+i+8vQPor0lUrfyhACF57Fxzg1cFTCJv9xBTuQskelx 2aNYtok Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > In addition, please have a look at commit 347a529398e8e723338cca5d8a8ae2d9e7e93448 > virtio_blk: Fix a slient kernel panic I would like to add another view on the implementation details in this software update. > which did the opposite of your patch. This update contained a different approach for error detection and corresponding exception handling. > And in fact it fixed a bug. This is great in principle according to an information in the commit description. "… To fix this bug, we should take care of allocation failure, and return correct value to let caller know what happen. …" > Quite obviously multiple labels are harder to read and harder to get right. > For error handling with just kfree one label is just the right thing to. Unfortunately, I get an other impression here after a closer look. Can it be that the discussed commit from 2016-08-09 accepted (or tolerated) two weaknesses at least? 1. Commit title: Is the word "slient" a typo? Would you like to read "silent" there instead? 2. Source code: Why would another memory allocation be attempted if it could be determined quicker that a previous one failed and this function implementation can not succeed then? How much will it matter in general that two function calls are performed in this use case without checking their return values immediately? https://cwe.mitre.org/data/definitions/252.html if (!names || !callbacks || !vqs) { … https://cwe.mitre.org/data/definitions/754.html Was the software development attention a bit too low as it happens occasionally? I hope that my suggestions can improve the affected situation a bit more also for this software module. Regards, Markus