* [Intel-wired-lan] [PATCH v2] ice: ice_adapter: release xa entry on adapter allocation failure [not found] <20250929024855.2037-1-vulab@iscas.ac.cn> @ 2025-09-30 1:51 ` Haotian Zhang 0 siblings, 0 replies; 6+ messages in thread From: Haotian Zhang @ 2025-09-30 1:51 UTC (permalink / raw) To: Jacob Keller, Tony Nguyen, Przemek Kitszel Cc: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, intel-wired-lan, netdev, linux-kernel, Haotian Zhang When ice_adapter_new() fails, the reserved XArray entry created by xa_insert() is not released. This causes subsequent insertions at the same index to return -EBUSY, potentially leading to NULL pointer dereferences. Release the reserved entry with xa_release() when adapter allocation fails to prevent this issue. Fixes: 0f0023c649c7 ("ice: do not init struct ice_adapter more times than needed") Suggested-by: Jacob Keller <jacob.e.keller@intel.com> Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn> --- Changes in v2: - Instead of checking the return value of xa_store(), fix the real bug where a failed ice_adapter_new() would leave a stale entry in the XArray. - Use xa_release() to clean up the reserved entry, as suggested by Jacob Keller. --- drivers/net/ethernet/intel/ice/ice_adapter.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c index b53561c34708..9eb100b11439 100644 --- a/drivers/net/ethernet/intel/ice/ice_adapter.c +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c @@ -110,8 +110,10 @@ struct ice_adapter *ice_adapter_get(struct pci_dev *pdev) return ERR_PTR(err); adapter = ice_adapter_new(pdev); - if (!adapter) + if (!adapter) { + xa_release(&ice_adapters, index); return ERR_PTR(-ENOMEM); + } xa_store(&ice_adapters, index, adapter, GFP_KERNEL); } return adapter; -- 2.50.1.windows.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2] ice: ice_adapter: release xa entry on adapter allocation failure @ 2025-09-30 1:51 ` Haotian Zhang 0 siblings, 0 replies; 6+ messages in thread From: Haotian Zhang @ 2025-09-30 1:51 UTC (permalink / raw) To: Jacob Keller, Tony Nguyen, Przemek Kitszel Cc: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, intel-wired-lan, netdev, linux-kernel, Haotian Zhang When ice_adapter_new() fails, the reserved XArray entry created by xa_insert() is not released. This causes subsequent insertions at the same index to return -EBUSY, potentially leading to NULL pointer dereferences. Release the reserved entry with xa_release() when adapter allocation fails to prevent this issue. Fixes: 0f0023c649c7 ("ice: do not init struct ice_adapter more times than needed") Suggested-by: Jacob Keller <jacob.e.keller@intel.com> Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn> --- Changes in v2: - Instead of checking the return value of xa_store(), fix the real bug where a failed ice_adapter_new() would leave a stale entry in the XArray. - Use xa_release() to clean up the reserved entry, as suggested by Jacob Keller. --- drivers/net/ethernet/intel/ice/ice_adapter.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c index b53561c34708..9eb100b11439 100644 --- a/drivers/net/ethernet/intel/ice/ice_adapter.c +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c @@ -110,8 +110,10 @@ struct ice_adapter *ice_adapter_get(struct pci_dev *pdev) return ERR_PTR(err); adapter = ice_adapter_new(pdev); - if (!adapter) + if (!adapter) { + xa_release(&ice_adapters, index); return ERR_PTR(-ENOMEM); + } xa_store(&ice_adapters, index, adapter, GFP_KERNEL); } return adapter; -- 2.50.1.windows.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Intel-wired-lan] [PATCH v2] ice: ice_adapter: release xa entry on adapter allocation failure 2025-09-30 1:51 ` Haotian Zhang @ 2025-10-01 5:29 ` Przemek Kitszel -1 siblings, 0 replies; 6+ messages in thread From: Przemek Kitszel @ 2025-10-01 5:29 UTC (permalink / raw) To: Haotian Zhang, Jacob Keller, Matthew Wilcox Cc: Tony Nguyen, Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, intel-wired-lan, netdev, linux-kernel On 9/30/25 03:51, Haotian Zhang wrote: > When ice_adapter_new() fails, the reserved XArray entry created by > xa_insert() is not released. This causes subsequent insertions at > the same index to return -EBUSY, potentially leading to > NULL pointer dereferences. > > Release the reserved entry with xa_release() when adapter allocation > fails to prevent this issue. > > Fixes: 0f0023c649c7 ("ice: do not init struct ice_adapter more times than needed") > Suggested-by: Jacob Keller <jacob.e.keller@intel.com> > Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn> > > --- > Changes in v2: > - Instead of checking the return value of xa_store(), fix the real bug > where a failed ice_adapter_new() would leave a stale entry in the > XArray. > - Use xa_release() to clean up the reserved entry, as suggested by > Jacob Keller. this is a correct improvement, but please let me propose other options, with 2. being my favorite: 1. (just ice changes) change the call order to be: (note that it is under a mutex) xa_load() // return early if another adapter exists xa_reserve() // return early if no mem ice_adapter_new() // return early if err xa_store() 2. (xarray changes) (perhaps I'm biased as the one introducing the error on error path): change xa_insert() to return 0 or -EEXIST when used as a reserving call (IOW: caller wanted to reserve, entry is reserved, so return should be 0 (or -EEXIST if we really want to differentiate in the callers)). > --- > drivers/net/ethernet/intel/ice/ice_adapter.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c > index b53561c34708..9eb100b11439 100644 > --- a/drivers/net/ethernet/intel/ice/ice_adapter.c > +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c > @@ -110,8 +110,10 @@ struct ice_adapter *ice_adapter_get(struct pci_dev *pdev) > return ERR_PTR(err); > > adapter = ice_adapter_new(pdev); > - if (!adapter) > + if (!adapter) { > + xa_release(&ice_adapters, index); > return ERR_PTR(-ENOMEM); > + } > xa_store(&ice_adapters, index, adapter, GFP_KERNEL); > } > return adapter; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ice: ice_adapter: release xa entry on adapter allocation failure @ 2025-10-01 5:29 ` Przemek Kitszel 0 siblings, 0 replies; 6+ messages in thread From: Przemek Kitszel @ 2025-10-01 5:29 UTC (permalink / raw) To: Haotian Zhang, Jacob Keller, Matthew Wilcox Cc: Tony Nguyen, Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, intel-wired-lan, netdev, linux-kernel On 9/30/25 03:51, Haotian Zhang wrote: > When ice_adapter_new() fails, the reserved XArray entry created by > xa_insert() is not released. This causes subsequent insertions at > the same index to return -EBUSY, potentially leading to > NULL pointer dereferences. > > Release the reserved entry with xa_release() when adapter allocation > fails to prevent this issue. > > Fixes: 0f0023c649c7 ("ice: do not init struct ice_adapter more times than needed") > Suggested-by: Jacob Keller <jacob.e.keller@intel.com> > Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn> > > --- > Changes in v2: > - Instead of checking the return value of xa_store(), fix the real bug > where a failed ice_adapter_new() would leave a stale entry in the > XArray. > - Use xa_release() to clean up the reserved entry, as suggested by > Jacob Keller. this is a correct improvement, but please let me propose other options, with 2. being my favorite: 1. (just ice changes) change the call order to be: (note that it is under a mutex) xa_load() // return early if another adapter exists xa_reserve() // return early if no mem ice_adapter_new() // return early if err xa_store() 2. (xarray changes) (perhaps I'm biased as the one introducing the error on error path): change xa_insert() to return 0 or -EEXIST when used as a reserving call (IOW: caller wanted to reserve, entry is reserved, so return should be 0 (or -EEXIST if we really want to differentiate in the callers)). > --- > drivers/net/ethernet/intel/ice/ice_adapter.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c > index b53561c34708..9eb100b11439 100644 > --- a/drivers/net/ethernet/intel/ice/ice_adapter.c > +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c > @@ -110,8 +110,10 @@ struct ice_adapter *ice_adapter_get(struct pci_dev *pdev) > return ERR_PTR(err); > > adapter = ice_adapter_new(pdev); > - if (!adapter) > + if (!adapter) { > + xa_release(&ice_adapters, index); > return ERR_PTR(-ENOMEM); > + } > xa_store(&ice_adapters, index, adapter, GFP_KERNEL); > } > return adapter; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-wired-lan] [PATCH v2] ice: ice_adapter: release xa entry on adapter allocation failure 2025-10-01 5:29 ` Przemek Kitszel @ 2025-10-01 22:00 ` Jacob Keller -1 siblings, 0 replies; 6+ messages in thread From: Jacob Keller @ 2025-10-01 22:00 UTC (permalink / raw) To: Przemek Kitszel, Haotian Zhang, Matthew Wilcox Cc: Tony Nguyen, Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, intel-wired-lan, netdev, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 3249 bytes --] On 9/30/2025 10:29 PM, Przemek Kitszel wrote: > On 9/30/25 03:51, Haotian Zhang wrote: >> When ice_adapter_new() fails, the reserved XArray entry created by >> xa_insert() is not released. This causes subsequent insertions at >> the same index to return -EBUSY, potentially leading to >> NULL pointer dereferences. >> >> Release the reserved entry with xa_release() when adapter allocation >> fails to prevent this issue. >> >> Fixes: 0f0023c649c7 ("ice: do not init struct ice_adapter more times than needed") >> Suggested-by: Jacob Keller <jacob.e.keller@intel.com> >> Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn> >> >> --- >> Changes in v2: >> - Instead of checking the return value of xa_store(), fix the real bug >> where a failed ice_adapter_new() would leave a stale entry in the >> XArray. >> - Use xa_release() to clean up the reserved entry, as suggested by >> Jacob Keller. > > this is a correct improvement, but please let me propose other options, > with 2. being my favorite: > > 1. (just ice changes) > change the call order to be: > (note that it is under a mutex) > xa_load() // return early if another adapter exists > xa_reserve() // return early if no mem > ice_adapter_new() // return early if err You still have to xa_release() here if we return early, but adding the call to xa_reserve might be more expressive of the intended behavior vs using xa_insert was. > xa_store() > > > 2. (xarray changes) > (perhaps I'm biased as the one introducing the error on error path): > change xa_insert() to return 0 or -EEXIST when used as a reserving call > (IOW: caller wanted to reserve, entry is reserved, so return should be 0 > (or -EEXIST if we really want to differentiate in the callers)). > If we go this route, I think -EEXIST is the right answer, as it should only return 0 if *this* call reserved the entry. -EEXIST instead of -EBUSY could differentiate between "slot is reserved" and "slot is filled" though. That would let us fix the issue by having xa_insert differentiate and go ahead if it fines a reserved entry that was unused. Thats safe for *our* use case because we know we were under lock and the only way we'd have a stale reserved entry is if we failed to release it... I am not certain how other users or maintainer of xarray would feel about such a change, which makes me think the ice side change is the best at least initially. > >> --- >> drivers/net/ethernet/intel/ice/ice_adapter.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c >> index b53561c34708..9eb100b11439 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_adapter.c >> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c >> @@ -110,8 +110,10 @@ struct ice_adapter *ice_adapter_get(struct pci_dev *pdev) >> return ERR_PTR(err); >> >> adapter = ice_adapter_new(pdev); >> - if (!adapter) >> + if (!adapter) { >> + xa_release(&ice_adapters, index); >> return ERR_PTR(-ENOMEM); >> + } >> xa_store(&ice_adapters, index, adapter, GFP_KERNEL); >> } >> return adapter; > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 236 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ice: ice_adapter: release xa entry on adapter allocation failure @ 2025-10-01 22:00 ` Jacob Keller 0 siblings, 0 replies; 6+ messages in thread From: Jacob Keller @ 2025-10-01 22:00 UTC (permalink / raw) To: Przemek Kitszel, Haotian Zhang, Matthew Wilcox Cc: Tony Nguyen, Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, intel-wired-lan, netdev, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 3249 bytes --] On 9/30/2025 10:29 PM, Przemek Kitszel wrote: > On 9/30/25 03:51, Haotian Zhang wrote: >> When ice_adapter_new() fails, the reserved XArray entry created by >> xa_insert() is not released. This causes subsequent insertions at >> the same index to return -EBUSY, potentially leading to >> NULL pointer dereferences. >> >> Release the reserved entry with xa_release() when adapter allocation >> fails to prevent this issue. >> >> Fixes: 0f0023c649c7 ("ice: do not init struct ice_adapter more times than needed") >> Suggested-by: Jacob Keller <jacob.e.keller@intel.com> >> Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn> >> >> --- >> Changes in v2: >> - Instead of checking the return value of xa_store(), fix the real bug >> where a failed ice_adapter_new() would leave a stale entry in the >> XArray. >> - Use xa_release() to clean up the reserved entry, as suggested by >> Jacob Keller. > > this is a correct improvement, but please let me propose other options, > with 2. being my favorite: > > 1. (just ice changes) > change the call order to be: > (note that it is under a mutex) > xa_load() // return early if another adapter exists > xa_reserve() // return early if no mem > ice_adapter_new() // return early if err You still have to xa_release() here if we return early, but adding the call to xa_reserve might be more expressive of the intended behavior vs using xa_insert was. > xa_store() > > > 2. (xarray changes) > (perhaps I'm biased as the one introducing the error on error path): > change xa_insert() to return 0 or -EEXIST when used as a reserving call > (IOW: caller wanted to reserve, entry is reserved, so return should be 0 > (or -EEXIST if we really want to differentiate in the callers)). > If we go this route, I think -EEXIST is the right answer, as it should only return 0 if *this* call reserved the entry. -EEXIST instead of -EBUSY could differentiate between "slot is reserved" and "slot is filled" though. That would let us fix the issue by having xa_insert differentiate and go ahead if it fines a reserved entry that was unused. Thats safe for *our* use case because we know we were under lock and the only way we'd have a stale reserved entry is if we failed to release it... I am not certain how other users or maintainer of xarray would feel about such a change, which makes me think the ice side change is the best at least initially. > >> --- >> drivers/net/ethernet/intel/ice/ice_adapter.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c >> index b53561c34708..9eb100b11439 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_adapter.c >> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c >> @@ -110,8 +110,10 @@ struct ice_adapter *ice_adapter_get(struct pci_dev *pdev) >> return ERR_PTR(err); >> >> adapter = ice_adapter_new(pdev); >> - if (!adapter) >> + if (!adapter) { >> + xa_release(&ice_adapters, index); >> return ERR_PTR(-ENOMEM); >> + } >> xa_store(&ice_adapters, index, adapter, GFP_KERNEL); >> } >> return adapter; > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 236 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-10-01 22:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20250929024855.2037-1-vulab@iscas.ac.cn>
2025-09-30 1:51 ` [Intel-wired-lan] [PATCH v2] ice: ice_adapter: release xa entry on adapter allocation failure Haotian Zhang
2025-09-30 1:51 ` Haotian Zhang
2025-10-01 5:29 ` [Intel-wired-lan] " Przemek Kitszel
2025-10-01 5:29 ` Przemek Kitszel
2025-10-01 22:00 ` [Intel-wired-lan] " Jacob Keller
2025-10-01 22:00 ` Jacob Keller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.