netdev - Re: [PATCH net-next 01/10] net: ipa: Don't error out in .remove()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9a0c97e7-a790-4440-9f92-0dfb077e18c5@ieee.org>
Date: Fri, 17 Nov 2023 14:50:58 -0600
From: Alex Elder <elder@...e.org>
To: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
 Alex Elder <elder@...nel.org>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, kernel@...gutronix.de
Subject: Re: [PATCH net-next 01/10] net: ipa: Don't error out in .remove()

On 11/17/23 3:59 AM, Uwe Kleine-König wrote:
> Returning early from .remove() with an error code still results in the
> driver unbinding the device. So the driver core ignores the returned error
> code and the resources that were not freed are never catched up. In
> combination with devm this also often results in use-after-free bugs.
> 
> Here even if the modem cannot be stopped, resources must be freed. So
> replace the early error return by an error message an continue to clean up.
> 
> This prepares changing ipa_remove() to return void.
> 
> Fixes: cdf2e9419dd9 ("soc: qcom: ipa: main code")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
> ---
>   drivers/net/ipa/ipa_main.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
> index da853353a5c7..60e4f590f5de 100644
> --- a/drivers/net/ipa/ipa_main.c
> +++ b/drivers/net/ipa/ipa_main.c
> @@ -960,7 +960,8 @@ static int ipa_remove(struct platform_device *pdev)
>   			ret = ipa_modem_stop(ipa);

This ipa_modem_stop() call is the second one in this function,
which is called if the first attempt returned an error.  The
only error that's returned is -EBUSY, which occurs if a request
to stop the modem arrives at a time when we're in transition.
That is, either we are in the process of starting it up, or
we are in the process of stopping it.  In either case, this
transitional state should come to an end quickly.  This second
call happens after a short delay, giving a chance for the start
or stop that's underway to complete.

If the *second* call returns an error, it's assumed we're stuck
in one of the two transitional states, in which case something is
wrong with the hardware (we've issued a request that did not
complete, for example).  And in that case, we have no way of
knowing whether the hardware will come alive and do something
with a resource that's been allocated for it.

I think I'd rather live with whatever resource leaks occur in
such an unlikely case, rather than free them without knowing
what the (broken) hardware is going to do.

>   		}
>   		if (ret)
> -			return ret;
> +			dev_err(dev, "Failed to stop modem (%pe)\n",
> +				ERR_PTR(ret));

By the above reasoning, I'd rather your patch result in the code
looking like this:

	if (ret) {
		dev_err(dev, "remove: error %d stopping modem\n", ret);
		return ret;		/* XXX Later: just return; */
	}

The message is more consistent with the way other messages in the
driver are written.  If %pe is preferred I'd rather make that change
comprehensively throughout the driver rather than bit-by-bit.

Thanks.

					-Alex

>   		ipa_teardown(ipa);
>   	}