[PATCH net v2 0/3] fix xa_alloc_cyclic() return checks

[PATCH net v2 0/3] fix xa_alloc_cyclic() return checks

[Michal Swiatkowski]

Pierre Riteau <pierre@stackhpc.com> found suspicious handling an error
from xa_alloc_cyclic() in scheduler code [1]. The same is done in few
other places.

v1 --> v2: [2]
 * add fixes tags
 * fix also the same usage in dpll and phy

[1] https://lore.kernel.org/netdev/20250213223610.320278-1-pierre@stackhpc.com/
[2] https://lore.kernel.org/netdev/20250214132453.4108-1-michal.swiatkowski@linux.intel.com/

Michal Swiatkowski (3):
  devlink: fix xa_alloc_cyclic() error handling
  dpll: fix xa_alloc_cyclic() error handling
  phy: fix xa_alloc_cyclic() error handling

 drivers/dpll/dpll_core.c            | 2 +-
 drivers/net/phy/phy_link_topology.c | 2 +-
 net/devlink/core.c                  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.42.0

[PATCH net v2 1/3] devlink: fix xa_alloc_cyclic() error handling

[Michal Swiatkowski]

In case of returning 1 from xa_alloc_cyclic() (wrapping) ERR_PTR(1) will
be returned, which will cause IS_ERR() to be false. Which can lead to
dereference not allocated pointer (rel).

Fix it by checking if err is lower than zero.

This wasn't found in real usecase, only noticed. Credit to Pierre.

Fixes: c137743bce02 ("devlink: introduce object and nested devlink relationship infra")
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
---
 net/devlink/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/devlink/core.c b/net/devlink/core.c
index f49cd83f1955..7203c39532fc 100644
--- a/net/devlink/core.c
+++ b/net/devlink/core.c
@@ -117,7 +117,7 @@ static struct devlink_rel *devlink_rel_alloc(void)
 
 	err = xa_alloc_cyclic(&devlink_rels, &rel->index, rel,
 			      xa_limit_32b, &next, GFP_KERNEL);
-	if (err) {
+	if (err < 0) {
 		kfree(rel);
 		return ERR_PTR(err);
 	}
-- 
2.42.0

Re: [PATCH net v2 1/3] devlink: fix xa_alloc_cyclic() error handling

[Andrew Lunn]

On Wed, Mar 12, 2025 at 10:52:49AM +0100, Michal Swiatkowski wrote:
> In case of returning 1 from xa_alloc_cyclic() (wrapping) ERR_PTR(1) will
> be returned, which will cause IS_ERR() to be false. Which can lead to
> dereference not allocated pointer (rel).
> 
> Fix it by checking if err is lower than zero.
> 
> This wasn't found in real usecase, only noticed. Credit to Pierre.
> 
> Fixes: c137743bce02 ("devlink: introduce object and nested devlink relationship infra")
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net v2 1/3] devlink: fix xa_alloc_cyclic() error handling

[Jiri Pirko]

Wed, Mar 12, 2025 at 10:52:49AM +0100, michal.swiatkowski@linux.intel.com wrote:
>In case of returning 1 from xa_alloc_cyclic() (wrapping) ERR_PTR(1) will
>be returned, which will cause IS_ERR() to be false. Which can lead to
>dereference not allocated pointer (rel).
>
>Fix it by checking if err is lower than zero.
>
>This wasn't found in real usecase, only noticed. Credit to Pierre.
>
>Fixes: c137743bce02 ("devlink: introduce object and nested devlink relationship infra")
>Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

[PATCH net v2 2/3] dpll: fix xa_alloc_cyclic() error handling

[Michal Swiatkowski]

In case of returning 1 from xa_alloc_cyclic() (wrapping) ERR_PTR(1) will
be returned, which will cause IS_ERR() to be false. Which can lead to
dereference not allocated pointer (pin).

Fix it by checking if err is lower than zero.

This wasn't found in real usecase, only noticed. Credit to Pierre.

Fixes: 97f265ef7f5b ("dpll: allocate pin ids in cycle")
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
---
 drivers/dpll/dpll_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
index 32019dc33cca..1877201d1aa9 100644
--- a/drivers/dpll/dpll_core.c
+++ b/drivers/dpll/dpll_core.c
@@ -505,7 +505,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module,
 	xa_init_flags(&pin->parent_refs, XA_FLAGS_ALLOC);
 	ret = xa_alloc_cyclic(&dpll_pin_xa, &pin->id, pin, xa_limit_32b,
 			      &dpll_pin_xa_id, GFP_KERNEL);
-	if (ret)
+	if (ret < 0)
 		goto err_xa_alloc;
 	return pin;
 err_xa_alloc:
-- 
2.42.0

Re: [PATCH net v2 2/3] dpll: fix xa_alloc_cyclic() error handling

[Vadim Fedorenko]

On 12/03/2025 09:52, Michal Swiatkowski wrote:
> In case of returning 1 from xa_alloc_cyclic() (wrapping) ERR_PTR(1) will
> be returned, which will cause IS_ERR() to be false. Which can lead to
> dereference not allocated pointer (pin).
> 
> Fix it by checking if err is lower than zero.
> 
> This wasn't found in real usecase, only noticed. Credit to Pierre.
> 
> Fixes: 97f265ef7f5b ("dpll: allocate pin ids in cycle")
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> ---
>   drivers/dpll/dpll_core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
> index 32019dc33cca..1877201d1aa9 100644
> --- a/drivers/dpll/dpll_core.c
> +++ b/drivers/dpll/dpll_core.c
> @@ -505,7 +505,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module,
>   	xa_init_flags(&pin->parent_refs, XA_FLAGS_ALLOC);
>   	ret = xa_alloc_cyclic(&dpll_pin_xa, &pin->id, pin, xa_limit_32b,
>   			      &dpll_pin_xa_id, GFP_KERNEL);
> -	if (ret)
> +	if (ret < 0)
>   		goto err_xa_alloc;
>   	return pin;
>   err_xa_alloc:

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

RE: [PATCH net v2 2/3] dpll: fix xa_alloc_cyclic() error handling

[Kubalewski, Arkadiusz]

>From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
>Sent: Wednesday, March 12, 2025 10:53 AM
>
>In case of returning 1 from xa_alloc_cyclic() (wrapping) ERR_PTR(1) will
>be returned, which will cause IS_ERR() to be false. Which can lead to
>dereference not allocated pointer (pin).
>
>Fix it by checking if err is lower than zero.
>
>This wasn't found in real usecase, only noticed. Credit to Pierre.
>
>Fixes: 97f265ef7f5b ("dpll: allocate pin ids in cycle")
>Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
>---
> drivers/dpll/dpll_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>index 32019dc33cca..1877201d1aa9 100644
>--- a/drivers/dpll/dpll_core.c
>+++ b/drivers/dpll/dpll_core.c
>@@ -505,7 +505,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module
>*module,
> 	xa_init_flags(&pin->parent_refs, XA_FLAGS_ALLOC);
> 	ret = xa_alloc_cyclic(&dpll_pin_xa, &pin->id, pin, xa_limit_32b,
> 			      &dpll_pin_xa_id, GFP_KERNEL);
>-	if (ret)
>+	if (ret < 0)
> 		goto err_xa_alloc;
> 	return pin;
> err_xa_alloc:
>--
>2.42.0

LGTM,
Thanks!

Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

Re: [PATCH net v2 2/3] dpll: fix xa_alloc_cyclic() error handling

[Jiri Pirko]

Wed, Mar 12, 2025 at 10:52:50AM +0100, michal.swiatkowski@linux.intel.com wrote:
>In case of returning 1 from xa_alloc_cyclic() (wrapping) ERR_PTR(1) will
>be returned, which will cause IS_ERR() to be false. Which can lead to
>dereference not allocated pointer (pin).
>
>Fix it by checking if err is lower than zero.
>
>This wasn't found in real usecase, only noticed. Credit to Pierre.
>
>Fixes: 97f265ef7f5b ("dpll: allocate pin ids in cycle")
>Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

[PATCH net v2 3/3] phy: fix xa_alloc_cyclic() error handling

[Michal Swiatkowski]

xa_alloc_cyclic() can return 1, which isn't an error. To prevent
situation when the caller of this function will treat it as no error do
a check only for negative here.

Fixes: 384968786909 ("net: phy: Introduce ethernet link topology representation")
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
---
 drivers/net/phy/phy_link_topology.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
index 4a5d73002a1a..0e9e987f37dd 100644
--- a/drivers/net/phy/phy_link_topology.c
+++ b/drivers/net/phy/phy_link_topology.c
@@ -73,7 +73,7 @@ int phy_link_topo_add_phy(struct net_device *dev,
 				      xa_limit_32b, &topo->next_phy_index,
 				      GFP_KERNEL);
 
-	if (ret)
+	if (ret < 0)
 		goto err;
 
 	return 0;
-- 
2.42.0

Re: [PATCH net v2 3/3] phy: fix xa_alloc_cyclic() error handling

[Maxime Chevallier]

On Wed, 12 Mar 2025 10:52:51 +0100
Michal Swiatkowski <michal.swiatkowski@linux.intel.com> wrote:

> xa_alloc_cyclic() can return 1, which isn't an error. To prevent
> situation when the caller of this function will treat it as no error do
> a check only for negative here.
> 
> Fixes: 384968786909 ("net: phy: Introduce ethernet link topology representation")
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> ---
>  drivers/net/phy/phy_link_topology.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> index 4a5d73002a1a..0e9e987f37dd 100644
> --- a/drivers/net/phy/phy_link_topology.c
> +++ b/drivers/net/phy/phy_link_topology.c
> @@ -73,7 +73,7 @@ int phy_link_topo_add_phy(struct net_device *dev,
>  				      xa_limit_32b, &topo->next_phy_index,
>  				      GFP_KERNEL);
>  
> -	if (ret)
> +	if (ret < 0)
>  		goto err;
>  
>  	return 0;

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thank you,

Maxime

Re: [PATCH net v2 0/3] fix xa_alloc_cyclic() return checks

[Dan Carpenter]

On Wed, Mar 12, 2025 at 10:52:48AM +0100, Michal Swiatkowski wrote:
> Pierre Riteau <pierre@stackhpc.com> found suspicious handling an error
> from xa_alloc_cyclic() in scheduler code [1]. The same is done in few
> other places.
> 
> v1 --> v2: [2]
>  * add fixes tags
>  * fix also the same usage in dpll and phy
> 
> [1] https://lore.kernel.org/netdev/20250213223610.320278-1-pierre@stackhpc.com/
> [2] https://lore.kernel.org/netdev/20250214132453.4108-1-michal.swiatkowski@linux.intel.com/
> 
> Michal Swiatkowski (3):
>   devlink: fix xa_alloc_cyclic() error handling
>   dpll: fix xa_alloc_cyclic() error handling
>   phy: fix xa_alloc_cyclic() error handling

Maybe there should be a wrapper around xa_alloc_cyclic() for people who
don't care about the 1 return?

int wrapper()
{
        ret = xa_alloc_cyclic();
        if (ret < 0)
                return ret;
        rerturn 0;
}

regards,
dan carpenter

Re: [PATCH net v2 0/3] fix xa_alloc_cyclic() return checks

[Przemek Kitszel]

On 3/14/25 11:23, Dan Carpenter wrote:
> On Wed, Mar 12, 2025 at 10:52:48AM +0100, Michal Swiatkowski wrote:
>> Pierre Riteau <pierre@stackhpc.com> found suspicious handling an error
>> from xa_alloc_cyclic() in scheduler code [1]. The same is done in few
>> other places.
>>
>> v1 --> v2: [2]
>>   * add fixes tags
>>   * fix also the same usage in dpll and phy
>>
>> [1] https://lore.kernel.org/netdev/20250213223610.320278-1-pierre@stackhpc.com/
>> [2] https://lore.kernel.org/netdev/20250214132453.4108-1-michal.swiatkowski@linux.intel.com/
>>
>> Michal Swiatkowski (3):
>>    devlink: fix xa_alloc_cyclic() error handling
>>    dpll: fix xa_alloc_cyclic() error handling
>>    phy: fix xa_alloc_cyclic() error handling
> 
> Maybe there should be a wrapper around xa_alloc_cyclic() for people who
> don't care about the 1 return?

What about changing init flags instead, and add a new one for this
purpose?, say:
XA_FLAGS_ALLOC_RET0

> 
> int wrapper()
> {
>          ret = xa_alloc_cyclic();
>          if (ret < 0)
>                  return ret;
>          rerturn 0;
> }
> 
> regards,
> dan carpenter
> 

Re: [PATCH net v2 0/3] fix xa_alloc_cyclic() return checks

[Dan Carpenter]

On Fri, Mar 14, 2025 at 01:52:58PM +0100, Przemek Kitszel wrote:
> On 3/14/25 11:23, Dan Carpenter wrote:
> > On Wed, Mar 12, 2025 at 10:52:48AM +0100, Michal Swiatkowski wrote:
> > > Pierre Riteau <pierre@stackhpc.com> found suspicious handling an error
> > > from xa_alloc_cyclic() in scheduler code [1]. The same is done in few
> > > other places.
> > > 
> > > v1 --> v2: [2]
> > >   * add fixes tags
> > >   * fix also the same usage in dpll and phy
> > > 
> > > [1] https://lore.kernel.org/netdev/20250213223610.320278-1-pierre@stackhpc.com/
> > > [2] https://lore.kernel.org/netdev/20250214132453.4108-1-michal.swiatkowski@linux.intel.com/
> > > 
> > > Michal Swiatkowski (3):
> > >    devlink: fix xa_alloc_cyclic() error handling
> > >    dpll: fix xa_alloc_cyclic() error handling
> > >    phy: fix xa_alloc_cyclic() error handling
> > 
> > Maybe there should be a wrapper around xa_alloc_cyclic() for people who
> > don't care about the 1 return?
> 
> What about changing init flags instead, and add a new one for this
> purpose?, say:
> XA_FLAGS_ALLOC_RET0

Right now I have a static checker rule for passing 1 to ERR_PTR().
It's not specific to this function but it catches the bugs here.  If we
added a XA_FLAGS_ALLOC_RET0 then I'd have to silence the checker rule for
xa_alloc_cyclic().

I was also thinking about creating another more specific rule for just this
function to warn about when callers which treat 1 and negative error
codes the same, but that wouldn't be possible.

On the other hand, people who pass XA_FLAGS_ALLOC_RET0 probably will
understand what it means and not introduce bugs so static analysis becomes
less important in that case.

regards,
dan carpenter

Re: [PATCH net v2 0/3] fix xa_alloc_cyclic() return checks

[Matthew Wilcox]

On Fri, Mar 14, 2025 at 01:52:58PM +0100, Przemek Kitszel wrote:
> What about changing init flags instead, and add a new one for this
> purpose?, say:
> XA_FLAGS_ALLOC_RET0

No.  Dan's suggestion is better.  Actually, I'd go further and
make xa_alloc_cyclic() always do that.  People who want the wrapping
information get to call __xa_alloc_cyclic themselves.

Re: [PATCH net v2 0/3] fix xa_alloc_cyclic() return checks

[Przemek Kitszel]

On 3/14/25 15:23, Matthew Wilcox wrote:
> On Fri, Mar 14, 2025 at 01:52:58PM +0100, Przemek Kitszel wrote:
>> What about changing init flags instead, and add a new one for this
>> purpose?, say:
>> XA_FLAGS_ALLOC_RET0
> 
> No.  Dan's suggestion is better.  Actually, I'd go further and
> make xa_alloc_cyclic() always do that.  People who want the wrapping
> information get to call __xa_alloc_cyclic themselves.

Even better, LGTM!

Re: [PATCH net v2 0/3] fix xa_alloc_cyclic() return checks

[Matthew Wilcox]

On Mon, Mar 17, 2025 at 09:55:44AM +0100, Przemek Kitszel wrote:
> On 3/14/25 15:23, Matthew Wilcox wrote:
> > On Fri, Mar 14, 2025 at 01:52:58PM +0100, Przemek Kitszel wrote:
> > > What about changing init flags instead, and add a new one for this
> > > purpose?, say:
> > > XA_FLAGS_ALLOC_RET0
> > 
> > No.  Dan's suggestion is better.  Actually, I'd go further and
> > make xa_alloc_cyclic() always do that.  People who want the wrapping
> > information get to call __xa_alloc_cyclic themselves.
> 
> Even better, LGTM!

Is that "I volunteer to do this"?

Re: [PATCH net v2 0/3] fix xa_alloc_cyclic() return checks

[Przemek Kitszel]

On 3/17/25 13:24, Matthew Wilcox wrote:
> On Mon, Mar 17, 2025 at 09:55:44AM +0100, Przemek Kitszel wrote:
>> On 3/14/25 15:23, Matthew Wilcox wrote:
>>> On Fri, Mar 14, 2025 at 01:52:58PM +0100, Przemek Kitszel wrote:
>>>> What about changing init flags instead, and add a new one for this
>>>> purpose?, say:
>>>> XA_FLAGS_ALLOC_RET0
>>>
>>> No.  Dan's suggestion is better.  Actually, I'd go further and
>>> make xa_alloc_cyclic() always do that.  People who want the wrapping
>>> information get to call __xa_alloc_cyclic themselves.
>>
>> Even better, LGTM!
> 
> Is that "I volunteer to do this"?

sure, why not :)

Re: [PATCH net v2 0/3] fix xa_alloc_cyclic() return checks

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed, 12 Mar 2025 10:52:48 +0100 you wrote:
> Pierre Riteau <pierre@stackhpc.com> found suspicious handling an error
> from xa_alloc_cyclic() in scheduler code [1]. The same is done in few
> other places.
> 
> v1 --> v2: [2]
>  * add fixes tags
>  * fix also the same usage in dpll and phy
> 
> [...]

Here is the summary with links:
  - [net,v2,1/3] devlink: fix xa_alloc_cyclic() error handling
    https://git.kernel.org/netdev/net/c/f3b97b7d4bf3
  - [net,v2,2/3] dpll: fix xa_alloc_cyclic() error handling
    https://git.kernel.org/netdev/net/c/3614bf90130d
  - [net,v2,3/3] phy: fix xa_alloc_cyclic() error handling
    https://git.kernel.org/netdev/net/c/3178d2b04836

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html