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")
Is this really a bug fix? This code was doing the right
thing even if the caller was not.
quoted hunk ↗ jump to hunk
Signed-off-by: Uwe Kleine-König <redacted>
---
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);
}
if (ret)
- return ret;
+ dev_err(dev, "Failed to stop modem (%pe)\n",
+ ERR_PTR(ret));
I think this is not correct, or rather, I think it is less
correct than returning early.
What's happening here is we're trying to stop the modem.
It is an external entity that might have some in-flight
activity that could include "owning" some buffers provided
by Linux, to be filled with received data. There's a
chance that cleaning up (with the call to ipa_teardown())
can do the right thing, but I'm not going to sign off on
this until I've looked at that in closer detail.
This is something that *could* happen but is not *expected*
to happen. We expect stopping the modem to succeed so if
it doesn't, something's wrong and it's not 100% clear how
to properly handle it.
For now... you know a little more about my hesitation, but
please wait to commit this change until I've had a chance
to spend more time reviewing.
-Alex
ipa_teardown(ipa);
}