Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 24 Feb 2022 13:03:30 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Vishnu Dasa <vdasa@vmware.com>
Cc:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
        Jorgen Hansen <jhansen@vmware.com>,
        "arnd@arndb.de" <arnd@arndb.de>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "acking@vmware.com" <acking@vmware.com>,
        "dtor@vmware.com" <dtor@vmware.com>,
        Pv-drivers <Pv-drivers@vmware.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kernel-janitors@vger.kernel.org" <kernel-janitors@vger.kernel.org>
Subject: Re: [PATCH 3/3] VMCI: Fix some error handling paths in
 vmci_guest_probe_device()
Message-ID: <20220224100330.GO3943@kadam>
References: <cover.1644531317.git.christophe.jaillet@wanadoo.fr>
 <c5c1a5f3547bea3c4a1cfb8474db683d83c0ca1d.1644531317.git.christophe.jaillet@wanadoo.fr>
 <20220211140950.GN1951@kadam>
 <1bc15512-d811-b26a-d4f3-e14b30730c00@wanadoo.fr>
 <85DB8130-D600-464E-9717-1A0AFE795EA7@vmware.com>
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <85DB8130-D600-464E-9717-1A0AFE795EA7@vmware.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?JtO466hcjrpkwKHmNAvSRacObAqMzRhA/zF1XwHDQg1HulNdT29ibuZ6PR?=
 =?iso-8859-1?Q?ydfTY7SgzoOXKrVSNWqFsT/Rkl4z+HMxVJ+IqG4bXDAZtcjrZDhklI5RyV?=
 =?iso-8859-1?Q?hnl9jB4PQoYZ8uPUbtsTb6qwC+victS3rhUjud2ePPZaHsQ13GaQDk9GEM?=
 =?iso-8859-1?Q?nm3iz3wdGaaUXSDiXWb6F2e8OHV79bTx/iintwof4LZwvQFEzQa7chK1VM?=
 =?iso-8859-1?Q?wekQYJU1Aos9fP8SOsyeh9GOociSq9CPwDx0Ee8zcx3MzvWG657Cqx1Y/j?=
 =?iso-8859-1?Q?BbfK20vjGCmb60VW0sNzRMTISEzxpSJNQrRxN8TxwSS5YA310XIOqYkAuy?=
 =?iso-8859-1?Q?k3OdV9IWKX+L54J6H97KmV4efHz9VlD2Ok+TGXcP/FAapBfXOZBFK4Q4Ub?=
 =?iso-8859-1?Q?zmq2KioA2LbXlXp+cJsrPPtLq3CnUDUVe17SYNwq5X9iIcMnCxw8nQ6YzG?=
 =?iso-8859-1?Q?89rK/QsZPjkH8kUbF7KfyL2sblBEadp4ANrxmQlssDvyzKh+8cu8uFhMav?=
 =?iso-8859-1?Q?N6k7I6twATEZX8kdXAJ2tVpziUrtXCkCpUlNhM30sDblBxhU2vEyo7ApFY?=
 =?iso-8859-1?Q?roSnZwdnpyP1E6hexChH9D8TCQYbyc6WYBhvFHz9pt6D1Aaq6RfMatsvPj?=
 =?iso-8859-1?Q?RhE8lNMCUzkCpPF9UCsZJOFHxnCTh9SfSJ1PpPgUbXz5fBQKjCyd8muYB1?=
 =?iso-8859-1?Q?eaaMs9SUMmRMhP1zCrKWgtC+GopfQ2HM0PzbSArcwSSRN8BJmUEdFecq7W?=
 =?iso-8859-1?Q?x37nqUYSfHm9e+dBPQZjGX2tkmyA0cM5fkZAuIcB5LrlWhlAzXRpkg4CFM?=
 =?iso-8859-1?Q?RGZKPLBr6GAy3Gg9d8SUJL7/JAFn5gpjTodk8oNk4ODBQHW0TQqPpVIH5p?=
 =?iso-8859-1?Q?jNQNi+eSirTjkkkj+h4JzDXIpmXj1ly3wdOEkbAo1tYfincRXkhRl4N7+R?=
 =?iso-8859-1?Q?52QeS+abj1eivtKi+WsK5rdT8QhApD5hBwqLG6iOGE/WrqNKBUg0X58VLQ?=
 =?iso-8859-1?Q?fUOKFeomd3vi9xv6w962owsB7jCylsYLE5+HtiuJKOzMCqOLaTWd7HlJSD?=
 =?iso-8859-1?Q?3Edzymir3VzmcjS+PCCRe9YoXe5b6lXM6LXp9WsPXeqxT+q9eYfJpmS8rZ?=
 =?iso-8859-1?Q?ouJhgk8nW8HAQuGiWtZ4nfJ4HfcFwpjw3OX44NlP6RVaLUusK2UIPVN/YU?=
 =?iso-8859-1?Q?zjOkZ0JYa5GbSWyenfxewwDd0YTeRXYoNYRyOb4j9AI9R+HOepbkQ8jPf4?=
 =?iso-8859-1?Q?T0j1vUSf9SqEfZBWtixNHIzDTbaYLH/UnA7IOW+612orNr7D3IDmD7rlg3?=
 =?iso-8859-1?Q?dx5VuImYnogQPnY9EBdZn+eWw1mFZQwNY82bixYyueu6lVsGkI9Wkduy6r?=
 =?iso-8859-1?Q?zteGpl4GK4jFkHRzUxGBCk8EzhRRrdiZcwxzlOVCWyZ9fj5Isicimv3q1v?=
 =?iso-8859-1?Q?5YqQJlw4VizJw6D+ZDiF/vLxN0vuV05TAi5lvM8BNHf6/D8Szi15GQst9s?=
 =?iso-8859-1?Q?rV8Ft3Zc/PPQq7/mbyHj+UMWbIvLelGHmSFwcv5SzfpUeu4eS1qw1bd+f1?=
 =?iso-8859-1?Q?WZ+dPK8llAr5BWwwrzX4+ONl51M5SiiGXyqyrdZ0KXzPitXU1cg0I5ERBO?=
 =?iso-8859-1?Q?5igfgEVy9SnINeO9u9hFMLCS6dvJChxf2xcbYeaVUqvsc23PoSg+C/aN98?=
 =?iso-8859-1?Q?/iIgaLuW9AmHjV8epMzohLwbTaPwZtx2LRXhWU+3bR3jRLOL5IWtnZJ9S5?=
 =?iso-8859-1?Q?cskg=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 08a8a405-bacb-456e-42b6-08d9f77cf5e7
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Feb 2022 10:03:52.8839
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: hByILNIjjQHdUXgYVjWl7uBvi0XshXsdAHz1vTeGS9ECiD7LDhmPsKioxwYsDsGL2hAz+IfL+9Ix/SfmPaXNJZW8wubqSl3ZVWvsaKDOEzU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB5468
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10267 signatures=681306
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 spamscore=0
 mlxscore=0 adultscore=0 mlxlogscore=999 suspectscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2201110000
 definitions=main-2202240059
X-Proofpoint-GUID: 4c9huhEOgeFYCVXcYDZ_a9Ow8fh_YF7e
X-Proofpoint-ORIG-GUID: 4c9huhEOgeFYCVXcYDZ_a9Ow8fh_YF7e
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIMWL_WL_MED,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Feb 24, 2022 at 06:53:10AM +0000, Vishnu Dasa wrote:
> 
> > On Feb 11, 2022, at 12:06 PM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> > 
> > Le 11/02/2022 ? 15:09, Dan Carpenter a ?crit :
> >> On Thu, Feb 10, 2022 at 11:27:34PM +0100, Christophe JAILLET wrote:

> >> Whatever...  But where this really
> >> hurts is with:
> >> Alloc:
> >> 	if (vmci_dev->exclusive_vectors) {
> >> 		error = request_irq(pci_irq_vector(pdev, 1), ...
> >> Free:
> >> 	free_irq(pci_irq_vector(pdev, 1), vmci_dev);
> >> No if statement.  It works because it's the last allocation but it's
> >> confusing and fragile.
> > 
> > Agreed.
> 
> Sorry, I hope I'm not missing something obvious or misunderstanding the point.
> But I don't think the problem implied here exists?
> 
> If 'request_irq(pci_irq_vector(pdev, 0), ...' fails we goto err_disable_msi and there
> is no free_irq in this path.  If 'request_irq(pci_irq_vector(pdev, 1), ...' fails then we
> goto err_free_irq and we do 'free_irq(pci_irq_vector(pdev, 0), vmci_dev)'.  Note that
> this is for the previous one without the check for vmci_dev->exclusive_vectors.

It's not a bug, but it's bad style.

Ideally the allocation code and the free code should mirror each other
as much as possible.  If there is an if statement in the allocation code
we should copy that same if statement into the free code.  Even if we
can leave the if statement out, we should still include it for
readability.

Also if we add another allocation at the end of the function then we
will have to add the if statement back.  Maybe the person who adds the
if statement will forget.  So it's fragile.

regards,
dan carpenter