From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2DC1232B9A9 for ; Wed, 15 Apr 2026 12:49:26 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776257367; cv=none; b=EiVBucksA3VTPSisoF6uZlyTuBRkGVYS+S9/mJ/ja2RUwjHXvt7hA7uXFQtnLkDFwJZTgiYxvSSPRedGGz85kAD6+3GaGpYsjZURsql3bFgZMF48HDQRHn5cjTUkmDkBVVJVlhbB10Dp+G9STNpLMndLYIj8qF65Cqwx/5/DAKw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776257367; c=relaxed/simple; bh=GOwZ5doA46qVcWz3RPILsKEjPzaIRsf8e9HxV08nzL4=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=BJ/kaKc2etOxYb0aclosD1v5m/JLybsAkUuNizo3cvdtKaeF6o9u2/zThu4HMOH5AUKNM2IXu2aYKlASeIHhlQApUvGoIbgePSoDPWTPfuqh4giQ8KHy8iXfD9NEXJDeeq78yC7a8QNF/5kdpBZIoL7BcwSEw0tvv9hVKVGvFkU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=J8Z7E+ea; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="J8Z7E+ea" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2879FC2BCB4; Wed, 15 Apr 2026 12:49:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1776257366; bh=GOwZ5doA46qVcWz3RPILsKEjPzaIRsf8e9HxV08nzL4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=J8Z7E+eaoXU5E36Ci2dtxMk1/QFNhGsdxvTszyuFWgzvwVnwmMB419YrtxS0qiImi O82QO67KWosKyuRvX72/MLD6BaW9Ml6DQd4N6ltTvgkugqfmtmdj//Fg/vMasEQiIq ydquxxeH3qwC5Rl/NCS1BXmi+T0VQkCTwUdRcte63xVwtTZxXJP9sG8jbLwHbpWiJZ i5zLGGJGAbFQ6njoeJtyPy+Tvo4cDmA3LzVWDS/nv/Wu7tqY4wJv1cVwPeGOb39BTO Dqhxe6lCosXo/yQhmG/DvxTbmm//HOfMBdk8ciMyXabCcRlLgX4cdQ9zTMSgrCg+N1 UeeU+Q825PSYg== From: Simon Horman To: aleksandr.loktionov@intel.com Cc: Simon Horman , intel-wired-lan@lists.osuosl.org, kiran.patil@intel.com, sylwesterx.dziedziuch@intel.com, netdev@vger.kernel.org, anthony.l.nguyen@intel.com, avinash.dayanand@intel.com Subject: Re: [PATCH iwl-net 1/5] iavf: fix null pointer dereference in iavf_detect_recover_hung Date: Wed, 15 Apr 2026 13:48:45 +0100 Message-ID: <20260415124845.797759-1-horms@kernel.org> X-Mailer: git-send-email 2.53.0 In-Reply-To: <20260413073035.4082204-2-aleksandr.loktionov@intel.com> References: <20260413073035.4082204-2-aleksandr.loktionov@intel.com> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html --- iavf: fix null pointer dereference in iavf_detect_recover_hung During a concurrent reset, q_vectors are freed and re-allocated while the watchdog task may still be iterating rings in iavf_detect_recover_hung(). Dereferencing a NULL q_vector inside iavf_force_wb() results in a crash. Guard against this by skipping rings whose q_vector is NULL. > Fixes: 9c6c12595b73 ("i40e: Detection and recovery of TX queue hung logic moved to service_task from tx_timeout") The Fixes tag points to commit 9c6c12595b73, but that commit only modified the i40e driver and did not introduce the iavf_detect_recover_hung function. The bug was actually introduced by commit 07d44190a389 ("i40e/i40evf: Detect and recover hung queue scenario"), which added iavf_detect_recover_hung with the code pattern 'if (tx_ring && tx_ring->desc)' that calls iavf_force_wb(vsi, tx_ring->q_vector) without checking if q_vector is NULL. Should the Fixes tag be: Fixes: 07d44190a389 ("i40e/i40evf: Detect and recover hung queue scenario") > diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.c b/drivers/net/ethernet/intel/iavf/iavf_txrx.c > index 363c42bf3dcf..e7e7fc9a5c68 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_txrx.c > +++ b/drivers/net/ethernet/intel/iavf/iavf_txrx.c [ ... ] > @@ -195,8 +194,11 @@ void iavf_detect_recover_hung(struct iavf_vsi *vsi) > return; > > for (i = 0; i < vsi->back->num_active_queues; i++) { > - tx_ring = &vsi->back->tx_rings[i]; > - if (tx_ring && tx_ring->desc) { > + struct iavf_ring *tx_ring = &vsi->back->tx_rings[i]; > + > + if (!tx_ring || !tx_ring->q_vector) > + continue; The check `if (!tx_ring || ...)` tests whether tx_ring is NULL, but tx_ring is assigned as `&vsi->back->tx_rings[i]`, which is the address of an array element. This address is always non-NULL by C language rules, as noted in the commit message itself: "the compiler can never observe since an array-element address is always non-NULL." This creates misleading logic. Other similar code in the driver correctly omits such checks (for example, in iavf_virtchnl.c and elsewhere in iavf_txrx.c). Should this be simplified to just check the q_vector: if (!tx_ring->q_vector) continue; > + if (tx_ring->desc) { > /* If packet counter has not changed the queue is > * likely stalled, so force an interrupt for this > * queue. [ ... ]