On Thu, Apr 6, 2023 at 8:08 PM Dan Carpenter <error27@xxxxxxxxx> wrote:
>
> On Tue, Apr 04, 2023 at 03:20:44PM +0800, Dongliang Mu wrote:
> > 3. I added a paired function - request_firmware vs release_firmware
> > with the following. Is this patch fine to check the unmatch state?
> >
> > diff --git a/check_unwind.c b/check_unwind.c
> > index f689181b..830dcfc7 100644
> > --- a/check_unwind.c
> > +++ b/check_unwind.c
> > @@ -94,6 +94,9 @@ static struct ref_func_info func_table[] = {
> >
> > { "ieee80211_alloc_hw", ALLOC, -1, "$", &valid_ptr_min_sval,
> > &valid_ptr_max_sval },
> > { "ieee80211_free_hw", RELEASE, 0, "$" },
> > +
> > + { "request_firmware", ALLOC, 0, "$", &int_zero, &int_zero},
>
> It should be "*$" instead of "$".
>
> > + { "release_firmware", RELEASE, 0, "$"}
> > };
>
> I had some result for that but when I looked through them I saw some
> false positives and fixed those bugs. Here are the results:
>
> False positives:
> drivers/crypto/qat/qat_common/adf_accel_engine.c:105 adf_ae_fw_load() warn: 'loader_data->mmp_fw' from request_firmware() not released on lines: 91,105.
> Smatch isn't connecting adf_ae_fw_release() to the correct firmware.
>
> drivers/net/wireless/st/cw1200/sta.c:1161 cw1200_setup_mac() warn: 'priv->sdd' from request_firmware() not released on lines: 1156.
> Firmware isn't supposed to be released on error.
>
> drivers/net/wireless/intersil/orinoco/fw.c:329 symbol_dl_firmware() warn: 'fw_entry' from request_firmware() not released on lines: 310,316,329.
> drivers/net/wireless/intersil/orinoco/fw.c:329 symbol_dl_firmware() warn: 'fw_entry' from request_firmware() not released on lines: 329.
> drivers/net/wireless/intersil/p54/p54spi.c:171 p54spi_request_firmware() warn: 'priv->firmware' from request_firmware() not released on lines: 168.
> Smatch gets confused by orinoco_cached_fw_get().
>
> drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c:2166 qlcnic_83xx_run_post() warn: 'fw_info->fw' from request_firmware() not released on lines: 2106,2132,2166.
> The firmware is released in qlcnic_83xx_copy_fw_file() but smatch isn't
> able to connect adapter->ahw->fw_info->fw to &fw_info->fw properly.
> This is a feature that needs to be added to smatch_ssa.c. It's not
> super complicated to do, but it's not a one liner either.
>
> drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c:2187 qlcnic_83xx_load_fw_image_from_host() warn: 'fw_info->fw' from request_firmware() not released on lines: 2182.
>
> I can't promise that the others are not false positives as well. This
> is just to give you an idea.
Thanks, Dan. This is helpful.
In the future, I want to add more paired operations which can help
detect more unmatched states in the kernel. This seems a simple way to
extend Smatch.
There are some academic works which try to find more paired functions.
I can use them to extract some paired functions and add them to
Smatch.
At the same time, I also can add some paired operations with similar
names, by code review.
To be honest, it's not easy for me to understand the code of Smatch in
a short time. I need some time to write new rules with Smatch checkers
to find more bugs.
>
> regards,
> dan carpenter
