| Message ID | 20240910135814.35693-2-przemyslaw.kitszel@intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-net,v2] ice: fix memleak in ice_init_tx_topology() | expand |
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.keller@intel.com> > Fixes: cc5776fe1832 ("ice: Enable switching default Tx scheduler topology") > CC: Larysa Zaremba <larysa.zaremba@intel.com> > CC: Jacob Keller <jacob.e.keller@intel.com> > CC: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> > CC: Mateusz Polchlopek <mateusz.polchlopek@intel.com> > Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@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.kitszel@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.kitszel@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? 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. The const fixes are also relatively smaller than I anticipated :D Thanks, Jake
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.keller@intel.com> Thanks! > >> Fixes: cc5776fe1832 ("ice: Enable switching default Tx scheduler topology") >> CC: Larysa Zaremba <larysa.zaremba@intel.com> >> CC: Jacob Keller <jacob.e.keller@intel.com> >> CC: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> >> CC: Mateusz Polchlopek <mateusz.polchlopek@intel.com> >> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@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.kitszel@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.kitszel@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
On 9/11/2024 1:37 AM, Przemek Kitszel wrote: > 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.keller@intel.com> > > Thanks! > >> >>> Fixes: cc5776fe1832 ("ice: Enable switching default Tx scheduler topology") >>> CC: Larysa Zaremba <larysa.zaremba@intel.com> >>> CC: Jacob Keller <jacob.e.keller@intel.com> >>> CC: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> >>> CC: Mateusz Polchlopek <mateusz.polchlopek@intel.com> >>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@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.kitszel@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.kitszel@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()) > Well I was thinking of splitting it as the change in this patch where you move the buffer copy, and change how we allocate things but with a forced cast, instead of changing all the function prototypes to const. However, I think this is small enough its fine as-is. >> >> Thanks, >> Jake >
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Przemek Kitszel > Sent: Tuesday, September 10, 2024 7:27 PM > To: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com> > Cc: Pucha, HimasekharX Reddy <himasekharx.reddy.pucha@intel.com>; Zaremba, Larysa <larysa.zaremba@intel.com>; netdev@vger.kernel.org; Polchlopek, Mateusz <mateusz.polchlopek@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com> > Subject: [Intel-wired-lan] [PATCH iwl-net v2] ice: fix memleak in ice_init_tx_topology() > > 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. > > Fixes: cc5776fe1832 ("ice: Enable switching default Tx scheduler topology") > CC: Larysa Zaremba <larysa.zaremba@intel.com> > CC: Jacob Keller <jacob.e.keller@intel.com> > CC: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> > CC: Mateusz Polchlopek <mateusz.polchlopek@intel.com> > Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@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.kitszel@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.kitszel@intel.com > it was just some of this patch, now it is const-correct > --- > drivers/net/ethernet/intel/ice/ice_ddp.h | 4 +- drivers/net/ethernet/intel/ice/ice_ddp.c | 58 +++++++++++------------ drivers/net/ethernet/intel/ice/ice_main.c | 8 +--- > 3 files changed, 31 insertions(+), 39 deletions(-) > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.h b/drivers/net/ethernet/intel/ice/ice_ddp.h index 622543f08b43..00840e5a1077 100644 --- a/drivers/net/ethernet/intel/ice/ice_ddp.h +++ b/drivers/net/ethernet/intel/ice/ice_ddp.h @@ -430,7 +430,7 @@ struct ice_pkg_enum { u32 buf_idx; u32 type; - struct ice_buf_hdr *buf; + const struct ice_buf_hdr *buf; u32 sect_idx; void *sect; u32 sect_type; @@ -454,6 +454,6 @@ u16 ice_pkg_buf_get_active_sections(struct ice_buf_build *bld); void *ice_pkg_enum_section(struct ice_seg *ice_seg, struct ice_pkg_enum *state, u32 sect_type); -int ice_cfg_tx_topo(struct ice_hw *hw, u8 *buf, u32 len); +int ice_cfg_tx_topo(struct ice_hw *hw, const void *buf, u32 len); #endif diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c index f182179529b7..6b60b7c4de09 100644 --- a/drivers/net/ethernet/intel/ice/ice_ddp.c +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c @@ -31,7 +31,7 @@ static const struct ice_tunnel_type_scan tnls[] = { * Verifies various attributes of the package file, including length, format * version, and the requirement of at least one segment. */ -static enum ice_ddp_state ice_verify_pkg(struct ice_pkg_hdr *pkg, u32 len) +static enum ice_ddp_state ice_verify_pkg(const struct ice_pkg_hdr *pkg, u32 len) { u32 seg_count; u32 i; @@ -57,13 +57,13 @@ static enum ice_ddp_state ice_verify_pkg(struct ice_pkg_hdr *pkg, u32 len) /* all segments must fit within length */ for (i = 0; i < seg_count; i++) { u32 off = le32_to_cpu(pkg->seg_offset[i]); - struct ice_generic_seg_hdr *seg; + const struct ice_generic_seg_hdr *seg; /* segment header must fit */ if (len < off + sizeof(*seg)) return ICE_DDP_PKG_INVALID_FILE; - seg = (struct ice_generic_seg_hdr *)((u8 *)pkg + off); + seg = (void *)pkg + off; /* segment body must fit */ if (len < off + le32_to_cpu(seg->seg_size)) @@ -119,13 +119,13 @@ static enum ice_ddp_state ice_chk_pkg_version(struct ice_pkg_ver *pkg_ver) * * This helper function validates a buffer's header. */ -static struct ice_buf_hdr *ice_pkg_val_buf(struct ice_buf *buf) +static const struct ice_buf_hdr *ice_pkg_val_buf(const struct ice_buf *buf) { - struct ice_buf_hdr *hdr; + const struct ice_buf_hdr *hdr; u16 section_count; u16 data_end; - hdr = (struct ice_buf_hdr *)buf->buf; + hdr = (const struct ice_buf_hdr *)buf->buf; /* verify data */ section_count = le16_to_cpu(hdr->section_count); if (section_count < ICE_MIN_S_COUNT || section_count > ICE_MAX_S_COUNT) @@ -165,8 +165,8 @@ static struct ice_buf_table *ice_find_buf_table(struct ice_seg *ice_seg) * unexpected value has been detected (for example an invalid section count or * an invalid buffer end value). */ -static struct ice_buf_hdr *ice_pkg_enum_buf(struct ice_seg *ice_seg, - struct ice_pkg_enum *state) +static const struct ice_buf_hdr *ice_pkg_enum_buf(struct ice_seg *ice_seg, + struct ice_pkg_enum *state) { if (ice_seg) { state->buf_table = ice_find_buf_table(ice_seg); @@ -1800,9 +1800,9 @@ int ice_update_pkg(struct ice_hw *hw, struct ice_buf *bufs, u32 count) * success it returns a pointer to the segment header, otherwise it will * return NULL. */ -static struct ice_generic_seg_hdr * +static const struct ice_generic_seg_hdr * ice_find_seg_in_pkg(struct ice_hw *hw, u32 seg_type, - struct ice_pkg_hdr *pkg_hdr) + const struct ice_pkg_hdr *pkg_hdr) { u32 i; @@ -1813,11 +1813,9 @@ ice_find_seg_in_pkg(struct ice_hw *hw, u32 seg_type, /* Search all package segments for the requested segment type */ for (i = 0; i < le32_to_cpu(pkg_hdr->seg_count); i++) { - struct ice_generic_seg_hdr *seg; + const struct ice_generic_seg_hdr *seg; - seg = (struct ice_generic_seg_hdr - *)((u8 *)pkg_hdr + - le32_to_cpu(pkg_hdr->seg_offset[i])); + seg = (void *)pkg_hdr + le32_to_cpu(pkg_hdr->seg_offset[i]); if (le32_to_cpu(seg->seg_type) == seg_type) return seg; @@ -2354,12 +2352,12 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 buf_size, * * Return: zero when update was successful, negative values otherwise. */ -int ice_cfg_tx_topo(struct ice_hw *hw, u8 *buf, u32 len) +int ice_cfg_tx_topo(struct ice_hw *hw, const void *buf, u32 len) { - u8 *current_topo, *new_topo = NULL; - struct ice_run_time_cfg_seg *seg; - struct ice_buf_hdr *section; - struct ice_pkg_hdr *pkg_hdr; + u8 *new_topo = NULL, *topo __free(kfree) = NULL; + const struct ice_run_time_cfg_seg *seg; + const struct ice_buf_hdr *section; + const struct ice_pkg_hdr *pkg_hdr; enum ice_ddp_state state; u16 offset, size = 0; u32 reg = 0; @@ -2375,15 +2373,13 @@ int ice_cfg_tx_topo(struct ice_hw *hw, u8 *buf, u32 len) return -EOPNOTSUPP; } - current_topo = kzalloc(ICE_AQ_MAX_BUF_LEN, GFP_KERNEL); - if (!current_topo) + topo = kzalloc(ICE_AQ_MAX_BUF_LEN, GFP_KERNEL); + if (!topo) return -ENOMEM; - /* Get the current Tx topology */ - status = ice_get_set_tx_topo(hw, current_topo, ICE_AQ_MAX_BUF_LEN, NULL, - &flags, false); - - kfree(current_topo); + /* Get the current Tx topology flags */ + status = ice_get_set_tx_topo(hw, topo, ICE_AQ_MAX_BUF_LEN, NULL, &flags, + false); if (status) { ice_debug(hw, ICE_DBG_INIT, "Get current topology is failed\n"); @@ -2419,16 +2415,16 @@ int ice_cfg_tx_topo(struct ice_hw *hw, u8 *buf, u32 len) goto update_topo; } - pkg_hdr = (struct ice_pkg_hdr *)buf; + pkg_hdr = (const struct ice_pkg_hdr *)buf; state = ice_verify_pkg(pkg_hdr, len); if (state) { ice_debug(hw, ICE_DBG_INIT, "Failed to verify pkg (err: %d)\n", state); return -EIO; } /* Find runtime configuration segment */ - seg = (struct ice_run_time_cfg_seg *) + seg = (const struct ice_run_time_cfg_seg *) ice_find_seg_in_pkg(hw, SEGMENT_TYPE_ICE_RUN_TIME_CFG, pkg_hdr); if (!seg) { ice_debug(hw, ICE_DBG_INIT, "5 layer topology segment is missing\n"); @@ -2461,8 +2457,10 @@ int ice_cfg_tx_topo(struct ice_hw *hw, u8 *buf, u32 len) return -EIO; } - /* Get the new topology buffer */ - new_topo = ((u8 *)section) + offset; + /* Get the new topology buffer, reuse current topo copy mem */ + static_assert(ICE_PKG_BUF_SIZE == ICE_AQ_MAX_BUF_LEN); + new_topo = topo; + memcpy(new_topo, (u8 *)section + offset, size); update_topo: /* Acquire global lock to make sure that set topology issued diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index c7db88b517da..30b94eca32b1 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -4548,16 +4548,10 @@ ice_init_tx_topology(struct ice_hw *hw, const struct firmware *firmware) u8 num_tx_sched_layers = hw->num_tx_sched_layers; struct ice_pf *pf = hw->back; struct device *dev; - u8 *buf_copy; int err; dev = ice_pf_to_dev(pf); - /* ice_cfg_tx_topo buf argument is not a constant, - * so we have to make a copy - */ - buf_copy = kmemdup(firmware->data, firmware->size, GFP_KERNEL); - - err = ice_cfg_tx_topo(hw, buf_copy, firmware->size); + err = ice_cfg_tx_topo(hw, firmware->data, firmware->size); if (!err) { if (hw->num_tx_sched_layers > num_tx_sched_layers) dev_info(dev, "Tx scheduling layers switching feature disabled\n");
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. Fixes: cc5776fe1832 ("ice: Enable switching default Tx scheduler topology") CC: Larysa Zaremba <larysa.zaremba@intel.com> CC: Jacob Keller <jacob.e.keller@intel.com> CC: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> CC: Mateusz Polchlopek <mateusz.polchlopek@intel.com> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@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.kitszel@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.kitszel@intel.com it was just some of this patch, now it is const-correct --- drivers/net/ethernet/intel/ice/ice_ddp.h | 4 +- drivers/net/ethernet/intel/ice/ice_ddp.c | 58 +++++++++++------------ drivers/net/ethernet/intel/ice/ice_main.c | 8 +--- 3 files changed, 31 insertions(+), 39 deletions(-) base-commit: b3c9e65eb227269ed72a115ba22f4f51b4e62b4d