Received-SPF: pass (google.com: domain of linux-wireless+bounces-3518-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <9779efb62cec04926e3bb2575a8aecc8bbf0e7ff.camel@sipsolutions.net>
Subject: Re: iwlwifi RFC related to iwl_mvm_tx_reclaim
From: Johannes Berg <johannes@sipsolutions.net>
To: Ben Greear <greearb@candelatech.com>, linux-wireless
	 <linux-wireless@vger.kernel.org>
Date: Tue, 13 Feb 2024 10:37:14 +0100
In-Reply-To: <c9b0c01e-acac-9f15-730f-a0ba991a68dc@candelatech.com>
References: <c9b0c01e-acac-9f15-730f-a0ba991a68dc@candelatech.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.3 (3.50.3-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

On Mon, 2024-02-12 at 15:22 -0800, Ben Greear wrote:
> I'm seeing a crash due to accessing an 'sta' object in this method that i=
s '-2' as integer.
>=20
> It fails the initial check for null STA, but I'm thinking it might should=
 check for IS_ERR(sta)
> as well.
>=20
> (I have my own patch that references sta before the IS_ERR check later in=
 the code, and this
> causes the crash I'm seeing.  I guess upstream will not crash in this sit=
uation.).

Indeed.

> My question:  Is the patch below a preferred approach, or should I add sp=
ecial checks to where I
> access sta and only exit the method lower where it already has the IS_ERR=
(sta) check?

You can do whatever you want in your tree, but I guess generally I'd
advocate you assume that the code does what it should ;-)

In this case, ERR_PTR(-ENOENT) is used to indicate the station is being
deleted, but has not yet been fully removed, and so indeed we still want
to reclaim the frames here correctly, which the code does.

The comment below even kind of explains that?

johannes