From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1764568AbcINQIz (ORCPT ); Wed, 14 Sep 2016 12:08:55 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:43025 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1763232AbcINQIx (ORCPT ); Wed, 14 Sep 2016 12:08:53 -0400 X-IBM-Helo: d06dlp02.portsmouth.uk.ibm.com X-IBM-MailFrom: sebott@linux.vnet.ibm.com X-IBM-RcptTo: linux-kernel@vger.kernel.org;linux-rdma@vger.kernel.org;netdev@vger.kernel.org Date: Wed, 14 Sep 2016 18:08:44 +0200 (CEST) From: Sebastian Ott X-X-Sender: sebott@schleppi To: Tariq Toukan cc: Yishai Hadas , Tariq Toukan , netdev@vger.kernel.org, linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] net/mlx4_en: fix off by one in error handling In-Reply-To: References: User-Agent: Alpine 2.20 (LFD 67 2015-01-07) Organization: =?ISO-8859-15?Q?=22IBM_Deutschland_Research_&_Development_GmbH_=2F_Vorsitzende_des_Aufsichtsrats=3A_Martina_Koederitz_Gesch=E4ftsf=FChrung=3A_Dirk_Wittkopp_Sitz_der_Gesellschaft=3A_B=F6blingen_=2F_Registergericht?= =?ISO-8859-15?Q?=3A_Amtsgericht_Stuttgart=2C_HRB_243294=22?= MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16091416-0040-0000-0000-000002D2AFB4 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16091416-0041-0000-0000-00001CF6D9F7 Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-09-14_08:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=11 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1609020000 definitions=main-1609140217 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 14 Sep 2016, Tariq Toukan wrote: > On 14/09/2016 4:53 PM, Sebastian Ott wrote: > > On Wed, 14 Sep 2016, Tariq Toukan wrote: > > > On 14/09/2016 2:09 PM, Sebastian Ott wrote: > > > > If an error occurs in mlx4_init_eq_table the index used in the > > > > err_out_unmap label is one too big which results in a panic in > > > > mlx4_free_eq. This patch fixes the index in the error path. > > > You are right, but your change below does not cover all cases. > > > The full solution looks like this: > > > > > > @@ -1260,7 +1260,7 @@ int mlx4_init_eq_table(struct mlx4_dev *dev) > > > eq); > > > } > > > if (err) > > > - goto err_out_unmap; > > > + goto err_out_unmap_excluded; > > In this case a call to mlx4_create_eq failed. Do you really have to call > > mlx4_free_eq for this index again? > > We agree on this part, that's why here we should goto the _excluded_ label. > For all other parts, we should not exclude the eq in the highest index, and > thus we goto the _non_excluded_ label. But that's exactly what the original patch does. If the failure is within the for loop at index i, we do the cleanup starting at index i-1. If the failure is after the for loop then i == dev->caps.num_comp_vectors + 1 and we do the cleanup starting at index i == dev->caps.num_comp_vectors. In the latter case your patch would have an out of bounds array access. Regards, Sebastian