On 9/10/24 23:30, Jacob Keller wrote:
On 9/10/2024 6:57 AM, Przemek Kitszel wrote:Fix leak of the FW blob (DDP pkg). Make ice_cfg_tx_topo() const-correct, so ice_init_tx_topology() can avoid copying whole FW blob. Copy just the topology section, and only when needed. Reuse the buffer allocated for the read of the current topology. This was found by kmemleak, with the following trace for each PF: [<ffffffff8761044d>] kmemdup_noprof+0x1d/0x50 [<ffffffffc0a0a480>] ice_init_ddp_config+0x100/0x220 [ice] [<ffffffffc0a0da7f>] ice_init_dev+0x6f/0x200 [ice] [<ffffffffc0a0dc49>] ice_init+0x29/0x560 [ice] [<ffffffffc0a10c1d>] ice_probe+0x21d/0x310 [ice] Constify ice_cfg_tx_topo() @buf parameter. This cascades further down to few more functions.Nice! Reviewed-by: Jacob Keller <jacob.e.kel...@intel.com>
Thanks!
Fixes: cc5776fe1832 ("ice: Enable switching default Tx scheduler topology") CC: Larysa Zaremba <larysa.zare...@intel.com> CC: Jacob Keller <jacob.e.kel...@intel.com> CC: Pucha Himasekhar Reddy <himasekharx.reddy.pu...@intel.com> CC: Mateusz Polchlopek <mateusz.polchlo...@intel.com> Signed-off-by: Przemek Kitszel <przemyslaw.kits...@intel.com> --- this patch obsoletes two other, so I'm dropping RB tags v1, iwl-net: https://lore.kernel.org/netdev/20240904123306.14629-2-przemyslaw.kits...@intel.com/ wrong assumption that ice_get_set_tx_topo() does not modify the buffer on adminq SET variant, it sometimes does, to fill the response, thanks to Pucha Himasekhar Reddy for finding it out; almost-const-correct iwl-next patch: https://lore.kernel.org/intel-wired-lan/20240904093135.8795-2-przemyslaw.kits...@intel.com it was just some of this patch, now it is const-correct ---Right. So now we're doing the const correctness in this patch along with the fix?
yes
Would it make sense to fix the copy issue but leave const updates to the next tree? I think I'm fine with this, but wonder if it will make backporting a bit more difficult? Probably not, given that this code is rarely modified.
hard to say, but I think one commit will make it a little bit easier, as there will be smaller number of possible sets of commits applied (at least in this case)
The const fixes are also relatively smaller than I anticipated :D
just adding kfree(), knowing the proper solution is to make code const-correct, is just a workaround, not a proper fix change is still rather small, and splitting into two would require postponing -next one to be after -net (as it will just remove the added kfree())
Thanks, Jake
