* Re: [Intel-wired-lan] [PATCH iwl-net v1] ice: fix bug with suspend and rebuild
2024-03-04 23:08 [PATCH iwl-net v1] ice: fix bug with suspend and rebuild Jesse Brandeburg
@ 2024-03-05 6:28 ` Paul Menzel
2024-03-05 13:17 ` Simon Horman
2024-03-06 16:46 ` Maciej Fijalkowski
2 siblings, 0 replies; 10+ messages in thread
From: Paul Menzel @ 2024-03-05 6:28 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: intel-wired-lan, Michal Swiatkowski, netdev, Eric Dumazet,
Tony Nguyen, Simon Horman, Przemek Kitszel, Jakub Kicinski,
Paolo Abeni, David S. Miller, Robert Elliott
Dear Jesse,
Thank you for your patch.
Am 05.03.24 um 00:08 schrieb Jesse Brandeburg:
> The ice driver would previously panic during suspend. This is caused
> from the driver *only* calling the ice_vsi_free_q_vectors() function by
> itself, when it is suspending. Since commit b3e7b3a6ee92 ("ice: prevent
> NULL pointer deref during reload") the driver has zeroed out
> num_q_vectors, and only restored it in ice_vsi_cfg_def().
>
> This further causes the ice_rebuild() function to allocate a zero length
> buffer, after which num_q_vectors is updated, and then the new value of
> num_q_vectors is used to index into the zero length buffer, which
> corrupts memory.
[…]
For the commit message summary I suggest to be more specific. Maybe:
ice: Fix memory corruption with suspend and rebuild
ice: Avoid 0-length buffer to fix memory corruption with suspend/rebuild
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Kind regards,
Paul
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH iwl-net v1] ice: fix bug with suspend and rebuild
2024-03-04 23:08 [PATCH iwl-net v1] ice: fix bug with suspend and rebuild Jesse Brandeburg
2024-03-05 6:28 ` [Intel-wired-lan] " Paul Menzel
@ 2024-03-05 13:17 ` Simon Horman
2024-03-05 17:35 ` Jesse Brandeburg
2024-03-06 16:46 ` Maciej Fijalkowski
2 siblings, 1 reply; 10+ messages in thread
From: Simon Horman @ 2024-03-05 13:17 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: intel-wired-lan, netdev, Robert Elliott, Tony Nguyen,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Przemek Kitszel, Michal Swiatkowski
On Mon, Mar 04, 2024 at 03:08:44PM -0800, Jesse Brandeburg wrote:
> The ice driver would previously panic during suspend. This is caused
> from the driver *only* calling the ice_vsi_free_q_vectors() function by
> itself, when it is suspending. Since commit b3e7b3a6ee92 ("ice: prevent
> NULL pointer deref during reload") the driver has zeroed out
> num_q_vectors, and only restored it in ice_vsi_cfg_def().
>
> This further causes the ice_rebuild() function to allocate a zero length
> buffer, after which num_q_vectors is updated, and then the new value of
> num_q_vectors is used to index into the zero length buffer, which
> corrupts memory.
>
> The fix entails making sure all the code referencing num_q_vectors only
> does so after it has been reset via ice_vsi_cfg_def().
>
> I didn't perform a full bisect, but I was able to test against 6.1.77
> kernel and that ice driver works fine for suspend/resume with no panic,
> so sometime since then, this problem was introduced.
>
> Also clean up an un-needed init of a local variable in the function
> being modified.
>
> PANIC from 6.8.0-rc1:
>
> [1026674.915596] PM: suspend exit
> [1026675.664697] ice 0000:17:00.1: PTP reset successful
> [1026675.664707] ice 0000:17:00.1: 2755 msecs passed between update to cached PHC time
> [1026675.667660] ice 0000:b1:00.0: PTP reset successful
> [1026675.675944] ice 0000:b1:00.0: 2832 msecs passed between update to cached PHC time
> [1026677.137733] ixgbe 0000:31:00.0 ens787: NIC Link is Up 1 Gbps, Flow Control: None
> [1026677.190201] BUG: kernel NULL pointer dereference, address: 0000000000000010
> [1026677.192753] ice 0000:17:00.0: PTP reset successful
> [1026677.192764] ice 0000:17:00.0: 4548 msecs passed between update to cached PHC time
> [1026677.197928] #PF: supervisor read access in kernel mode
> [1026677.197933] #PF: error_code(0x0000) - not-present page
> [1026677.197937] PGD 1557a7067 P4D 0
> [1026677.212133] ice 0000:b1:00.1: PTP reset successful
> [1026677.212143] ice 0000:b1:00.1: 4344 msecs passed between update to cached PHC time
> [1026677.212575]
> [1026677.243142] Oops: 0000 [#1] PREEMPT SMP NOPTI
> [1026677.247918] CPU: 23 PID: 42790 Comm: kworker/23:0 Kdump: loaded Tainted: G W 6.8.0-rc1+ #1
> [1026677.257989] Hardware name: Intel Corporation M50CYP2SBSTD/M50CYP2SBSTD, BIOS SE5C620.86B.01.01.0005.2202160810 02/16/2022
> [1026677.269367] Workqueue: ice ice_service_task [ice]
> [1026677.274592] RIP: 0010:ice_vsi_rebuild_set_coalesce+0x130/0x1e0 [ice]
> [1026677.281421] Code: 0f 84 3a ff ff ff 41 0f b7 74 ec 02 66 89 b0 22 02 00 00 81 e6 ff 1f 00 00 e8 ec fd ff ff e9 35 ff ff ff 48 8b 43 30 49 63 ed <41> 0f b7 34 24 41 83 c5 01 48 8b 3c e8 66 89 b7 aa 02 00 00 81 e6
> [1026677.300877] RSP: 0018:ff3be62a6399bcc0 EFLAGS: 00010202
> [1026677.306556] RAX: ff28691e28980828 RBX: ff28691e41099828 RCX: 0000000000188000
> [1026677.314148] RDX: 0000000000000000 RSI: 0000000000000010 RDI: ff28691e41099828
> [1026677.321730] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
> [1026677.329311] R10: 0000000000000007 R11: ffffffffffffffc0 R12: 0000000000000010
> [1026677.336896] R13: 0000000000000000 R14: 0000000000000000 R15: ff28691e0eaa81a0
> [1026677.344472] FS: 0000000000000000(0000) GS:ff28693cbffc0000(0000) knlGS:0000000000000000
> [1026677.353000] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [1026677.359195] CR2: 0000000000000010 CR3: 0000000128df4001 CR4: 0000000000771ef0
> [1026677.366779] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [1026677.374369] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [1026677.381952] PKRU: 55555554
> [1026677.385116] Call Trace:
> [1026677.388023] <TASK>
> [1026677.390589] ? __die+0x20/0x70
> [1026677.394105] ? page_fault_oops+0x82/0x160
> [1026677.398576] ? do_user_addr_fault+0x65/0x6a0
> [1026677.403307] ? exc_page_fault+0x6a/0x150
> [1026677.407694] ? asm_exc_page_fault+0x22/0x30
> [1026677.412349] ? ice_vsi_rebuild_set_coalesce+0x130/0x1e0 [ice]
> [1026677.418614] ice_vsi_rebuild+0x34b/0x3c0 [ice]
> [1026677.423583] ice_vsi_rebuild_by_type+0x76/0x180 [ice]
> [1026677.429147] ice_rebuild+0x18b/0x520 [ice]
> [1026677.433746] ? delay_tsc+0x8f/0xc0
> [1026677.437630] ice_do_reset+0xa3/0x190 [ice]
> [1026677.442231] ice_service_task+0x26/0x440 [ice]
> [1026677.447180] process_one_work+0x174/0x340
> [1026677.451669] worker_thread+0x27e/0x390
> [1026677.455890] ? __pfx_worker_thread+0x10/0x10
> [1026677.460627] kthread+0xee/0x120
> [1026677.464235] ? __pfx_kthread+0x10/0x10
> [1026677.468445] ret_from_fork+0x2d/0x50
> [1026677.472476] ? __pfx_kthread+0x10/0x10
> [1026677.476671] ret_from_fork_asm+0x1b/0x30
> [1026677.481050] </TASK>
>
> Fixes: b3e7b3a6ee92 ("ice: prevent NULL pointer deref during reload")
> Reported-by: Robert Elliott <elliott@hpe.com>
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
> drivers/net/ethernet/intel/ice/ice_lib.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index 097bf8fd6bf0..0f5a92a6b1e6 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -3238,7 +3238,7 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags)
> {
> struct ice_vsi_cfg_params params = {};
> struct ice_coalesce_stored *coalesce;
> - int prev_num_q_vectors = 0;
> + int prev_num_q_vectors;
> struct ice_pf *pf;
> int ret;
>
> @@ -3252,13 +3252,6 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags)
> if (WARN_ON(vsi->type == ICE_VSI_VF && !vsi->vf))
> return -EINVAL;
>
> - coalesce = kcalloc(vsi->num_q_vectors,
> - sizeof(struct ice_coalesce_stored), GFP_KERNEL);
> - if (!coalesce)
> - return -ENOMEM;
> -
> - prev_num_q_vectors = ice_vsi_rebuild_get_coalesce(vsi, coalesce);
> -
> ret = ice_vsi_realloc_stat_arrays(vsi);
> if (ret)
> goto err_vsi_cfg;
> @@ -3268,6 +3261,13 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags)
> if (ret)
> goto err_vsi_cfg;
Hi Jesse,
the label above will result in a call to kfree(coalesce).
However, coalesce is now uninitialised until the following line executes.
>
> + coalesce = kcalloc(vsi->num_q_vectors,
> + sizeof(struct ice_coalesce_stored), GFP_KERNEL);
> + if (!coalesce)
> + return -ENOMEM;
> +
> + prev_num_q_vectors = ice_vsi_rebuild_get_coalesce(vsi, coalesce);
> +
> ret = ice_vsi_cfg_tc_lan(pf, vsi);
> if (ret) {
> if (vsi_flags & ICE_VSI_FLAG_INIT) {
>
> base-commit: 6923134fc6b62d7909169b3ad913ab72ee04233a
--
pw-bot: changes-requested
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH iwl-net v1] ice: fix bug with suspend and rebuild
2024-03-05 13:17 ` Simon Horman
@ 2024-03-05 17:35 ` Jesse Brandeburg
0 siblings, 0 replies; 10+ messages in thread
From: Jesse Brandeburg @ 2024-03-05 17:35 UTC (permalink / raw)
To: Simon Horman
Cc: intel-wired-lan, netdev, Robert Elliott, Tony Nguyen,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Przemek Kitszel, Michal Swiatkowski
On 3/5/2024 5:17 AM, Simon Horman wrote:
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
>> @@ -3238,7 +3238,7 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags)
>> {
>> struct ice_vsi_cfg_params params = {};
>> struct ice_coalesce_stored *coalesce;
>> - int prev_num_q_vectors = 0;
>> + int prev_num_q_vectors;
>> struct ice_pf *pf;
>> int ret;
>>
>> @@ -3252,13 +3252,6 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags)
>> if (WARN_ON(vsi->type == ICE_VSI_VF && !vsi->vf))
>> return -EINVAL;
>>
>> - coalesce = kcalloc(vsi->num_q_vectors,
>> - sizeof(struct ice_coalesce_stored), GFP_KERNEL);
>> - if (!coalesce)
>> - return -ENOMEM;
>> -
>> - prev_num_q_vectors = ice_vsi_rebuild_get_coalesce(vsi, coalesce);
>> -
>> ret = ice_vsi_realloc_stat_arrays(vsi);
>> if (ret)
>> goto err_vsi_cfg;
>> @@ -3268,6 +3261,13 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags)
>> if (ret)
>> goto err_vsi_cfg;
>
> Hi Jesse,
>
> the label above will result in a call to kfree(coalesce).
> However, coalesce is now uninitialised until the following line executes.
Thanks Simon, you're right! And it figures, The internal static analysis
runs caught this only after I sent the patch to the list.
One fix is to NULL the initialization of coalesce, which solves the
problem with a one-liner, but I'll look more today at whether I should
just move the label or something else.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH iwl-net v1] ice: fix bug with suspend and rebuild
2024-03-04 23:08 [PATCH iwl-net v1] ice: fix bug with suspend and rebuild Jesse Brandeburg
2024-03-05 6:28 ` [Intel-wired-lan] " Paul Menzel
2024-03-05 13:17 ` Simon Horman
@ 2024-03-06 16:46 ` Maciej Fijalkowski
2024-03-06 16:56 ` Maciej Fijalkowski
2 siblings, 1 reply; 10+ messages in thread
From: Maciej Fijalkowski @ 2024-03-06 16:46 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: intel-wired-lan, netdev, Robert Elliott, Tony Nguyen,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Przemek Kitszel, Michal Swiatkowski, Simon Horman
On Mon, Mar 04, 2024 at 03:08:44PM -0800, Jesse Brandeburg wrote:
> The ice driver would previously panic during suspend. This is caused
> from the driver *only* calling the ice_vsi_free_q_vectors() function by
> itself, when it is suspending. Since commit b3e7b3a6ee92 ("ice: prevent
> NULL pointer deref during reload") the driver has zeroed out
> num_q_vectors, and only restored it in ice_vsi_cfg_def().
>
> This further causes the ice_rebuild() function to allocate a zero length
> buffer, after which num_q_vectors is updated, and then the new value of
> num_q_vectors is used to index into the zero length buffer, which
> corrupts memory.
>
> The fix entails making sure all the code referencing num_q_vectors only
> does so after it has been reset via ice_vsi_cfg_def().
>
> I didn't perform a full bisect, but I was able to test against 6.1.77
> kernel and that ice driver works fine for suspend/resume with no panic,
> so sometime since then, this problem was introduced.
>
> Also clean up an un-needed init of a local variable in the function
> being modified.
>
> PANIC from 6.8.0-rc1:
>
> [1026674.915596] PM: suspend exit
> [1026675.664697] ice 0000:17:00.1: PTP reset successful
> [1026675.664707] ice 0000:17:00.1: 2755 msecs passed between update to cached PHC time
> [1026675.667660] ice 0000:b1:00.0: PTP reset successful
> [1026675.675944] ice 0000:b1:00.0: 2832 msecs passed between update to cached PHC time
> [1026677.137733] ixgbe 0000:31:00.0 ens787: NIC Link is Up 1 Gbps, Flow Control: None
> [1026677.190201] BUG: kernel NULL pointer dereference, address: 0000000000000010
> [1026677.192753] ice 0000:17:00.0: PTP reset successful
> [1026677.192764] ice 0000:17:00.0: 4548 msecs passed between update to cached PHC time
> [1026677.197928] #PF: supervisor read access in kernel mode
> [1026677.197933] #PF: error_code(0x0000) - not-present page
> [1026677.197937] PGD 1557a7067 P4D 0
> [1026677.212133] ice 0000:b1:00.1: PTP reset successful
> [1026677.212143] ice 0000:b1:00.1: 4344 msecs passed between update to cached PHC time
> [1026677.212575]
> [1026677.243142] Oops: 0000 [#1] PREEMPT SMP NOPTI
> [1026677.247918] CPU: 23 PID: 42790 Comm: kworker/23:0 Kdump: loaded Tainted: G W 6.8.0-rc1+ #1
> [1026677.257989] Hardware name: Intel Corporation M50CYP2SBSTD/M50CYP2SBSTD, BIOS SE5C620.86B.01.01.0005.2202160810 02/16/2022
> [1026677.269367] Workqueue: ice ice_service_task [ice]
> [1026677.274592] RIP: 0010:ice_vsi_rebuild_set_coalesce+0x130/0x1e0 [ice]
> [1026677.281421] Code: 0f 84 3a ff ff ff 41 0f b7 74 ec 02 66 89 b0 22 02 00 00 81 e6 ff 1f 00 00 e8 ec fd ff ff e9 35 ff ff ff 48 8b 43 30 49 63 ed <41> 0f b7 34 24 41 83 c5 01 48 8b 3c e8 66 89 b7 aa 02 00 00 81 e6
> [1026677.300877] RSP: 0018:ff3be62a6399bcc0 EFLAGS: 00010202
> [1026677.306556] RAX: ff28691e28980828 RBX: ff28691e41099828 RCX: 0000000000188000
> [1026677.314148] RDX: 0000000000000000 RSI: 0000000000000010 RDI: ff28691e41099828
> [1026677.321730] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
> [1026677.329311] R10: 0000000000000007 R11: ffffffffffffffc0 R12: 0000000000000010
> [1026677.336896] R13: 0000000000000000 R14: 0000000000000000 R15: ff28691e0eaa81a0
> [1026677.344472] FS: 0000000000000000(0000) GS:ff28693cbffc0000(0000) knlGS:0000000000000000
> [1026677.353000] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [1026677.359195] CR2: 0000000000000010 CR3: 0000000128df4001 CR4: 0000000000771ef0
> [1026677.366779] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [1026677.374369] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [1026677.381952] PKRU: 55555554
> [1026677.385116] Call Trace:
> [1026677.388023] <TASK>
> [1026677.390589] ? __die+0x20/0x70
> [1026677.394105] ? page_fault_oops+0x82/0x160
> [1026677.398576] ? do_user_addr_fault+0x65/0x6a0
> [1026677.403307] ? exc_page_fault+0x6a/0x150
> [1026677.407694] ? asm_exc_page_fault+0x22/0x30
> [1026677.412349] ? ice_vsi_rebuild_set_coalesce+0x130/0x1e0 [ice]
> [1026677.418614] ice_vsi_rebuild+0x34b/0x3c0 [ice]
> [1026677.423583] ice_vsi_rebuild_by_type+0x76/0x180 [ice]
> [1026677.429147] ice_rebuild+0x18b/0x520 [ice]
> [1026677.433746] ? delay_tsc+0x8f/0xc0
> [1026677.437630] ice_do_reset+0xa3/0x190 [ice]
> [1026677.442231] ice_service_task+0x26/0x440 [ice]
> [1026677.447180] process_one_work+0x174/0x340
> [1026677.451669] worker_thread+0x27e/0x390
> [1026677.455890] ? __pfx_worker_thread+0x10/0x10
> [1026677.460627] kthread+0xee/0x120
> [1026677.464235] ? __pfx_kthread+0x10/0x10
> [1026677.468445] ret_from_fork+0x2d/0x50
> [1026677.472476] ? __pfx_kthread+0x10/0x10
> [1026677.476671] ret_from_fork_asm+0x1b/0x30
> [1026677.481050] </TASK>
>
> Fixes: b3e7b3a6ee92 ("ice: prevent NULL pointer deref during reload")
> Reported-by: Robert Elliott <elliott@hpe.com>
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Well, that refactor of config path introduced lots of issues. Could
validation folks include a short list of tests they tried out against
tested patch?
> ---
> drivers/net/ethernet/intel/ice/ice_lib.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index 097bf8fd6bf0..0f5a92a6b1e6 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -3238,7 +3238,7 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags)
> {
> struct ice_vsi_cfg_params params = {};
> struct ice_coalesce_stored *coalesce;
> - int prev_num_q_vectors = 0;
> + int prev_num_q_vectors;
> struct ice_pf *pf;
> int ret;
>
> @@ -3252,13 +3252,6 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags)
> if (WARN_ON(vsi->type == ICE_VSI_VF && !vsi->vf))
> return -EINVAL;
>
> - coalesce = kcalloc(vsi->num_q_vectors,
> - sizeof(struct ice_coalesce_stored), GFP_KERNEL);
> - if (!coalesce)
> - return -ENOMEM;
> -
> - prev_num_q_vectors = ice_vsi_rebuild_get_coalesce(vsi, coalesce);
> -
> ret = ice_vsi_realloc_stat_arrays(vsi);
> if (ret)
> goto err_vsi_cfg;
> @@ -3268,6 +3261,13 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags)
> if (ret)
> goto err_vsi_cfg;
>
> + coalesce = kcalloc(vsi->num_q_vectors,
> + sizeof(struct ice_coalesce_stored), GFP_KERNEL);
> + if (!coalesce)
> + return -ENOMEM;
> +
> + prev_num_q_vectors = ice_vsi_rebuild_get_coalesce(vsi, coalesce);
> +
> ret = ice_vsi_cfg_tc_lan(pf, vsi);
> if (ret) {
> if (vsi_flags & ICE_VSI_FLAG_INIT) {
>
> base-commit: 6923134fc6b62d7909169b3ad913ab72ee04233a
> --
> 2.39.3
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH iwl-net v1] ice: fix bug with suspend and rebuild
2024-03-06 16:46 ` Maciej Fijalkowski
@ 2024-03-06 16:56 ` Maciej Fijalkowski
2024-03-07 1:33 ` Jesse Brandeburg
0 siblings, 1 reply; 10+ messages in thread
From: Maciej Fijalkowski @ 2024-03-06 16:56 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: intel-wired-lan, netdev, Robert Elliott, Tony Nguyen,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Przemek Kitszel, Michal Swiatkowski, Simon Horman
On Wed, Mar 06, 2024 at 05:46:02PM +0100, Maciej Fijalkowski wrote:
> On Mon, Mar 04, 2024 at 03:08:44PM -0800, Jesse Brandeburg wrote:
> > The ice driver would previously panic during suspend. This is caused
> > from the driver *only* calling the ice_vsi_free_q_vectors() function by
> > itself, when it is suspending. Since commit b3e7b3a6ee92 ("ice: prevent
> > NULL pointer deref during reload") the driver has zeroed out
> > num_q_vectors, and only restored it in ice_vsi_cfg_def().
> >
> > This further causes the ice_rebuild() function to allocate a zero length
> > buffer, after which num_q_vectors is updated, and then the new value of
> > num_q_vectors is used to index into the zero length buffer, which
> > corrupts memory.
> >
> > The fix entails making sure all the code referencing num_q_vectors only
> > does so after it has been reset via ice_vsi_cfg_def().
> >
> > I didn't perform a full bisect, but I was able to test against 6.1.77
> > kernel and that ice driver works fine for suspend/resume with no panic,
> > so sometime since then, this problem was introduced.
> >
> > Also clean up an un-needed init of a local variable in the function
> > being modified.
> >
> > PANIC from 6.8.0-rc1:
> >
> > [1026674.915596] PM: suspend exit
> > [1026675.664697] ice 0000:17:00.1: PTP reset successful
> > [1026675.664707] ice 0000:17:00.1: 2755 msecs passed between update to cached PHC time
> > [1026675.667660] ice 0000:b1:00.0: PTP reset successful
> > [1026675.675944] ice 0000:b1:00.0: 2832 msecs passed between update to cached PHC time
> > [1026677.137733] ixgbe 0000:31:00.0 ens787: NIC Link is Up 1 Gbps, Flow Control: None
> > [1026677.190201] BUG: kernel NULL pointer dereference, address: 0000000000000010
> > [1026677.192753] ice 0000:17:00.0: PTP reset successful
> > [1026677.192764] ice 0000:17:00.0: 4548 msecs passed between update to cached PHC time
> > [1026677.197928] #PF: supervisor read access in kernel mode
> > [1026677.197933] #PF: error_code(0x0000) - not-present page
> > [1026677.197937] PGD 1557a7067 P4D 0
> > [1026677.212133] ice 0000:b1:00.1: PTP reset successful
> > [1026677.212143] ice 0000:b1:00.1: 4344 msecs passed between update to cached PHC time
> > [1026677.212575]
> > [1026677.243142] Oops: 0000 [#1] PREEMPT SMP NOPTI
> > [1026677.247918] CPU: 23 PID: 42790 Comm: kworker/23:0 Kdump: loaded Tainted: G W 6.8.0-rc1+ #1
> > [1026677.257989] Hardware name: Intel Corporation M50CYP2SBSTD/M50CYP2SBSTD, BIOS SE5C620.86B.01.01.0005.2202160810 02/16/2022
> > [1026677.269367] Workqueue: ice ice_service_task [ice]
> > [1026677.274592] RIP: 0010:ice_vsi_rebuild_set_coalesce+0x130/0x1e0 [ice]
> > [1026677.281421] Code: 0f 84 3a ff ff ff 41 0f b7 74 ec 02 66 89 b0 22 02 00 00 81 e6 ff 1f 00 00 e8 ec fd ff ff e9 35 ff ff ff 48 8b 43 30 49 63 ed <41> 0f b7 34 24 41 83 c5 01 48 8b 3c e8 66 89 b7 aa 02 00 00 81 e6
> > [1026677.300877] RSP: 0018:ff3be62a6399bcc0 EFLAGS: 00010202
> > [1026677.306556] RAX: ff28691e28980828 RBX: ff28691e41099828 RCX: 0000000000188000
> > [1026677.314148] RDX: 0000000000000000 RSI: 0000000000000010 RDI: ff28691e41099828
> > [1026677.321730] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
> > [1026677.329311] R10: 0000000000000007 R11: ffffffffffffffc0 R12: 0000000000000010
> > [1026677.336896] R13: 0000000000000000 R14: 0000000000000000 R15: ff28691e0eaa81a0
> > [1026677.344472] FS: 0000000000000000(0000) GS:ff28693cbffc0000(0000) knlGS:0000000000000000
> > [1026677.353000] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [1026677.359195] CR2: 0000000000000010 CR3: 0000000128df4001 CR4: 0000000000771ef0
> > [1026677.366779] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > [1026677.374369] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > [1026677.381952] PKRU: 55555554
> > [1026677.385116] Call Trace:
> > [1026677.388023] <TASK>
> > [1026677.390589] ? __die+0x20/0x70
> > [1026677.394105] ? page_fault_oops+0x82/0x160
> > [1026677.398576] ? do_user_addr_fault+0x65/0x6a0
> > [1026677.403307] ? exc_page_fault+0x6a/0x150
> > [1026677.407694] ? asm_exc_page_fault+0x22/0x30
> > [1026677.412349] ? ice_vsi_rebuild_set_coalesce+0x130/0x1e0 [ice]
> > [1026677.418614] ice_vsi_rebuild+0x34b/0x3c0 [ice]
> > [1026677.423583] ice_vsi_rebuild_by_type+0x76/0x180 [ice]
> > [1026677.429147] ice_rebuild+0x18b/0x520 [ice]
> > [1026677.433746] ? delay_tsc+0x8f/0xc0
> > [1026677.437630] ice_do_reset+0xa3/0x190 [ice]
> > [1026677.442231] ice_service_task+0x26/0x440 [ice]
> > [1026677.447180] process_one_work+0x174/0x340
> > [1026677.451669] worker_thread+0x27e/0x390
> > [1026677.455890] ? __pfx_worker_thread+0x10/0x10
> > [1026677.460627] kthread+0xee/0x120
> > [1026677.464235] ? __pfx_kthread+0x10/0x10
> > [1026677.468445] ret_from_fork+0x2d/0x50
> > [1026677.472476] ? __pfx_kthread+0x10/0x10
> > [1026677.476671] ret_from_fork_asm+0x1b/0x30
> > [1026677.481050] </TASK>
> >
> > Fixes: b3e7b3a6ee92 ("ice: prevent NULL pointer deref during reload")
> > Reported-by: Robert Elliott <elliott@hpe.com>
> > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>
> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
>
> Well, that refactor of config path introduced lots of issues. Could
> validation folks include a short list of tests they tried out against
> tested patch?
Sorry, I got confused and now I saw the same thing Simon pointed out.
>
> > ---
> > drivers/net/ethernet/intel/ice/ice_lib.c | 16 ++++++++--------
> > 1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> > index 097bf8fd6bf0..0f5a92a6b1e6 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> > @@ -3238,7 +3238,7 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags)
> > {
> > struct ice_vsi_cfg_params params = {};
> > struct ice_coalesce_stored *coalesce;
struct ice_coalesce_stored *coalesce __free(kfree);
?
and drop explicit kfree()s altogether?
> > - int prev_num_q_vectors = 0;
> > + int prev_num_q_vectors;
> > struct ice_pf *pf;
> > int ret;
> >
> > @@ -3252,13 +3252,6 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags)
> > if (WARN_ON(vsi->type == ICE_VSI_VF && !vsi->vf))
> > return -EINVAL;
> >
> > - coalesce = kcalloc(vsi->num_q_vectors,
> > - sizeof(struct ice_coalesce_stored), GFP_KERNEL);
> > - if (!coalesce)
> > - return -ENOMEM;
> > -
> > - prev_num_q_vectors = ice_vsi_rebuild_get_coalesce(vsi, coalesce);
> > -
> > ret = ice_vsi_realloc_stat_arrays(vsi);
> > if (ret)
> > goto err_vsi_cfg;
> > @@ -3268,6 +3261,13 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags)
> > if (ret)
> > goto err_vsi_cfg;
> >
> > + coalesce = kcalloc(vsi->num_q_vectors,
> > + sizeof(struct ice_coalesce_stored), GFP_KERNEL);
> > + if (!coalesce)
> > + return -ENOMEM;
> > +
> > + prev_num_q_vectors = ice_vsi_rebuild_get_coalesce(vsi, coalesce);
> > +
> > ret = ice_vsi_cfg_tc_lan(pf, vsi);
> > if (ret) {
> > if (vsi_flags & ICE_VSI_FLAG_INIT) {
> >
> > base-commit: 6923134fc6b62d7909169b3ad913ab72ee04233a
> > --
> > 2.39.3
> >
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH iwl-net v1] ice: fix bug with suspend and rebuild
2024-03-06 16:56 ` Maciej Fijalkowski
@ 2024-03-07 1:33 ` Jesse Brandeburg
2024-03-07 1:41 ` Jesse Brandeburg
2024-03-07 16:11 ` Maciej Fijalkowski
0 siblings, 2 replies; 10+ messages in thread
From: Jesse Brandeburg @ 2024-03-07 1:33 UTC (permalink / raw)
To: Maciej Fijalkowski
Cc: intel-wired-lan, netdev, Robert Elliott, Tony Nguyen,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Przemek Kitszel, Michal Swiatkowski, Simon Horman
On 3/6/2024 8:56 AM, Maciej Fijalkowski wrote:
>>> Fixes: b3e7b3a6ee92 ("ice: prevent NULL pointer deref during reload")
>>> Reported-by: Robert Elliott <elliott@hpe.com>
>>> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>>
>> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
>>
>> Well, that refactor of config path introduced lots of issues. Could
>> validation folks include a short list of tests they tried out against
>> tested patch?
>
> Sorry, I got confused and now I saw the same thing Simon pointed out.
Thanks, NP! this thread is for v1, and v2 is already posted with the
refactor just moving the CONFIG_FOO in patch 1, and then the real final
version of the fix in v2 patch two which switches all the intel drivers
over to the new macros.
>
>>
>>> ---
>>> drivers/net/ethernet/intel/ice/ice_lib.c | 16 ++++++++--------
>>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
>>> index 097bf8fd6bf0..0f5a92a6b1e6 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
>>> @@ -3238,7 +3238,7 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags)
>>> {
>>> struct ice_vsi_cfg_params params = {};
>>> struct ice_coalesce_stored *coalesce;
>
> struct ice_coalesce_stored *coalesce __free(kfree);
Yes, but not in a "fix" patch for -net. I figure this is more of a
refactor kind of thing, so will not include it in this change.
>
> ?
>
> and drop explicit kfree()s altogether?
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH iwl-net v1] ice: fix bug with suspend and rebuild
2024-03-07 1:33 ` Jesse Brandeburg
@ 2024-03-07 1:41 ` Jesse Brandeburg
2024-03-07 16:10 ` Maciej Fijalkowski
2024-03-07 16:11 ` Maciej Fijalkowski
1 sibling, 1 reply; 10+ messages in thread
From: Jesse Brandeburg @ 2024-03-07 1:41 UTC (permalink / raw)
To: Maciej Fijalkowski
Cc: intel-wired-lan, netdev, Robert Elliott, Tony Nguyen,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Przemek Kitszel, Michal Swiatkowski, Simon Horman
On 3/6/2024 5:33 PM, Jesse Brandeburg wrote:
> On 3/6/2024 8:56 AM, Maciej Fijalkowski wrote:
>>>> Fixes: b3e7b3a6ee92 ("ice: prevent NULL pointer deref during reload")
>>>> Reported-by: Robert Elliott <elliott@hpe.com>
>>>> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>>>
>>> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
>>>
>>> Well, that refactor of config path introduced lots of issues. Could
>>> validation folks include a short list of tests they tried out against
>>> tested patch?
That's a good question which I'll follow up on with our team, but please
don't let it block anything for this patch.
>>
>> Sorry, I got confused and now I saw the same thing Simon pointed out.
>
> Thanks, NP! this thread is for v1, and v2 is already posted with the
> refactor just moving the CONFIG_FOO in patch 1, and then the real final
> version of the fix in v2 patch two which switches all the intel drivers
> over to the new macros.
oops!
FYI v2 of this change was posted at
https://lore.kernel.org/netdev/20240305230204.448724-1-jesse.brandeburg@intel.com/
Thanks for the review!
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH iwl-net v1] ice: fix bug with suspend and rebuild
2024-03-07 1:41 ` Jesse Brandeburg
@ 2024-03-07 16:10 ` Maciej Fijalkowski
0 siblings, 0 replies; 10+ messages in thread
From: Maciej Fijalkowski @ 2024-03-07 16:10 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: intel-wired-lan, netdev, Robert Elliott, Tony Nguyen,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Przemek Kitszel, Michal Swiatkowski, Simon Horman
On Wed, Mar 06, 2024 at 05:41:10PM -0800, Jesse Brandeburg wrote:
> On 3/6/2024 5:33 PM, Jesse Brandeburg wrote:
> > On 3/6/2024 8:56 AM, Maciej Fijalkowski wrote:
> > > > > Fixes: b3e7b3a6ee92 ("ice: prevent NULL pointer deref during reload")
> > > > > Reported-by: Robert Elliott <elliott@hpe.com>
> > > > > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > > >
> > > > Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > >
> > > > Well, that refactor of config path introduced lots of issues. Could
> > > > validation folks include a short list of tests they tried out against
> > > > tested patch?
>
> That's a good question which I'll follow up on with our team, but please
> don't let it block anything for this patch.
Blocking was not my intent at all, just raising a concern regarding this
ever-lasting issue.
>
> > >
> > > Sorry, I got confused and now I saw the same thing Simon pointed out.
> >
> > Thanks, NP! this thread is for v1, and v2 is already posted with the
> > refactor just moving the CONFIG_FOO in patch 1, and then the real final
> > version of the fix in v2 patch two which switches all the intel drivers
> > over to the new macros.
>
> oops!
>
> FYI v2 of this change was posted at https://lore.kernel.org/netdev/20240305230204.448724-1-jesse.brandeburg@intel.com/
>
> Thanks for the review!
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH iwl-net v1] ice: fix bug with suspend and rebuild
2024-03-07 1:33 ` Jesse Brandeburg
2024-03-07 1:41 ` Jesse Brandeburg
@ 2024-03-07 16:11 ` Maciej Fijalkowski
1 sibling, 0 replies; 10+ messages in thread
From: Maciej Fijalkowski @ 2024-03-07 16:11 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: intel-wired-lan, netdev, Robert Elliott, Tony Nguyen,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Przemek Kitszel, Michal Swiatkowski, Simon Horman
On Wed, Mar 06, 2024 at 05:33:50PM -0800, Jesse Brandeburg wrote:
> On 3/6/2024 8:56 AM, Maciej Fijalkowski wrote:
> > > > Fixes: b3e7b3a6ee92 ("ice: prevent NULL pointer deref during reload")
> > > > Reported-by: Robert Elliott <elliott@hpe.com>
> > > > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > >
> > > Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > >
> > > Well, that refactor of config path introduced lots of issues. Could
> > > validation folks include a short list of tests they tried out against
> > > tested patch?
> >
> > Sorry, I got confused and now I saw the same thing Simon pointed out.
>
> Thanks, NP! this thread is for v1, and v2 is already posted with the
> refactor just moving the CONFIG_FOO in patch 1, and then the real final
> version of the fix in v2 patch two which switches all the intel drivers over
> to the new macros.
>
> >
> > >
> > > > ---
> > > > drivers/net/ethernet/intel/ice/ice_lib.c | 16 ++++++++--------
> > > > 1 file changed, 8 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> > > > index 097bf8fd6bf0..0f5a92a6b1e6 100644
> > > > --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> > > > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> > > > @@ -3238,7 +3238,7 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags)
> > > > {
> > > > struct ice_vsi_cfg_params params = {};
> > > > struct ice_coalesce_stored *coalesce;
> >
> > struct ice_coalesce_stored *coalesce __free(kfree);
>
> Yes, but not in a "fix" patch for -net. I figure this is more of a refactor
> kind of thing, so will not include it in this change.
I suppose that's because older kernels might not have this construct
backported, so yeah your v2 is good as-is.
>
> >
> > ?
> >
> > and drop explicit kfree()s altogether?
> >
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread