| Message ID | 20241015063651.8610-1-yuancan@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] mlxsw: spectrum_router: fix xa_store() error checking | expand |
Yuan Can <yuancan@huawei.com> writes: > It is meant to use xa_err() to extract the error encoded in the return > value of xa_store(). > > Fixes: 44c2fbebe18a ("mlxsw: spectrum_router: Share nexthop counters in resilient groups") > Signed-off-by: Yuan Can <yuancan@huawei.com> Reviewed-by: Petr Machata <petrm@nvidia.com> What's the consequence of using IS_ERR()/PTR_ERR() vs. xa_err()? From the documentation it looks like IS_ERR() might interpret some valid pointers as errors[0]. Which would then show as leaks, because we bail out early and never clean up? I.e. should this aim at net rather than net-next? It looks like it's not just semantics, but has actual observable impact. [0] "The XArray does not support storing IS_ERR() pointers as some conflict with value entries or internal entries." > --- > drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > index 800dfb64ec83..7d6d859cef3f 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > @@ -3197,7 +3197,6 @@ mlxsw_sp_nexthop_sh_counter_get(struct mlxsw_sp *mlxsw_sp, > { > struct mlxsw_sp_nexthop_group *nh_grp = nh->nhgi->nh_grp; > struct mlxsw_sp_nexthop_counter *nhct; > - void *ptr; > int err; > > nhct = xa_load(&nh_grp->nhgi->nexthop_counters, nh->id); > @@ -3210,12 +3209,10 @@ mlxsw_sp_nexthop_sh_counter_get(struct mlxsw_sp *mlxsw_sp, > if (IS_ERR(nhct)) > return nhct; > > - ptr = xa_store(&nh_grp->nhgi->nexthop_counters, nh->id, nhct, > - GFP_KERNEL); > - if (IS_ERR(ptr)) { > - err = PTR_ERR(ptr); > + err = xa_err(xa_store(&nh_grp->nhgi->nexthop_counters, nh->id, nhct, > + GFP_KERNEL)); > + if (err) > goto err_store; > - }
On 2024/10/15 16:06, Petr Machata wrote: > Yuan Can <yuancan@huawei.com> writes: > >> It is meant to use xa_err() to extract the error encoded in the return >> value of xa_store(). >> >> Fixes: 44c2fbebe18a ("mlxsw: spectrum_router: Share nexthop counters in resilient groups") >> Signed-off-by: Yuan Can <yuancan@huawei.com> > Reviewed-by: Petr Machata <petrm@nvidia.com> > > What's the consequence of using IS_ERR()/PTR_ERR() vs. xa_err()? From > the documentation it looks like IS_ERR() might interpret some valid > pointers as errors[0]. Which would then show as leaks, because we bail > out early and never clean up? At least the PRT_ERR() will return a wrong error number, though the error number seems not used nor printed. > > I.e. should this aim at net rather than net-next? It looks like it's not > just semantics, but has actual observable impact. Ok, do I need to send a V2 patch to net branch? > > [0] "The XArray does not support storing IS_ERR() pointers as some > conflict with value entries or internal entries." > >> --- >> drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 9 +++------ >> 1 file changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c >> index 800dfb64ec83..7d6d859cef3f 100644 >> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c >> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c >> @@ -3197,7 +3197,6 @@ mlxsw_sp_nexthop_sh_counter_get(struct mlxsw_sp *mlxsw_sp, >> { >> struct mlxsw_sp_nexthop_group *nh_grp = nh->nhgi->nh_grp; >> struct mlxsw_sp_nexthop_counter *nhct; >> - void *ptr; >> int err; >> >> nhct = xa_load(&nh_grp->nhgi->nexthop_counters, nh->id); >> @@ -3210,12 +3209,10 @@ mlxsw_sp_nexthop_sh_counter_get(struct mlxsw_sp *mlxsw_sp, >> if (IS_ERR(nhct)) >> return nhct; >> >> - ptr = xa_store(&nh_grp->nhgi->nexthop_counters, nh->id, nhct, >> - GFP_KERNEL); >> - if (IS_ERR(ptr)) { >> - err = PTR_ERR(ptr); >> + err = xa_err(xa_store(&nh_grp->nhgi->nexthop_counters, nh->id, nhct, >> + GFP_KERNEL)); >> + if (err) >> goto err_store; >> - }
Yuan Can <yuancan@huawei.com> writes: > On 2024/10/15 16:06, Petr Machata wrote: >> Yuan Can <yuancan@huawei.com> writes: >> >>> It is meant to use xa_err() to extract the error encoded in the return >>> value of xa_store(). >>> >>> Fixes: 44c2fbebe18a ("mlxsw: spectrum_router: Share nexthop counters in resilient groups") >>> Signed-off-by: Yuan Can <yuancan@huawei.com> >> >> Reviewed-by: Petr Machata <petrm@nvidia.com> >> >> What's the consequence of using IS_ERR()/PTR_ERR() vs. xa_err()? From >> the documentation it looks like IS_ERR() might interpret some valid >> pointers as errors[0]. Which would then show as leaks, because we bail >> out early and never clean up? > > At least the PRT_ERR() will return a wrong error number, though the error number > > seems not used nor printed. What I'm saying is that if IS_ERR overestimates what is an error, we bail out from mlxsw_sp_nexthop_sh_counter_get() with a failure, but xa_store() actually succeeded, and the corresponding xa_erase is never called, causing a leak. (If IS_ERR underestimates what is an error, fails to store the allocated counter, and counter sharing stops working. This will waste HW resources, though I think it should still behave correctly overall.) Anyway, it looks to me like a net material. >> >> I.e. should this aim at net rather than net-next? It looks like it's not >> just semantics, but has actual observable impact. > > Ok, do I need to send a V2 patch to net branch? Yes please.
On 10/16/24 04:19, Yuan Can wrote: > On 2024/10/15 16:06, Petr Machata wrote: >> Yuan Can <yuancan@huawei.com> writes: >> >>> It is meant to use xa_err() to extract the error encoded in the return >>> value of xa_store(). >>> >>> Fixes: 44c2fbebe18a ("mlxsw: spectrum_router: Share nexthop counters >>> in resilient groups") >>> Signed-off-by: Yuan Can <yuancan@huawei.com> >> Reviewed-by: Petr Machata <petrm@nvidia.com> >> >> What's the consequence of using IS_ERR()/PTR_ERR() vs. xa_err()? From >> the documentation it looks like IS_ERR() might interpret some valid >> pointers as errors[0]. it is an error to insert error pointers into xarray, but @nhct is not an error thanks to prior check this patch correctly checks for error returned from xarray store attempt which is later (just after the context of the patch) converted via ERR_PTR(), so: Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> >> Which would then show as leaks, because we bail >> out early and never clean up? > > At least the PRT_ERR() will return a wrong error number, though the > error number > > seems not used nor printed. > >> >> I.e. should this aim at net rather than net-next? It looks like it's not >> just semantics, but has actual observable impact. > Ok, do I need to send a V2 patch to net branch? >> >> [0] "The XArray does not support storing IS_ERR() pointers as some >> conflict with value entries or internal entries." >> >>> --- >>> drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 9 +++------ >>> 1 file changed, 3 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/ >>> drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c >>> index 800dfb64ec83..7d6d859cef3f 100644 >>> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c >>> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c >>> @@ -3197,7 +3197,6 @@ mlxsw_sp_nexthop_sh_counter_get(struct mlxsw_sp >>> *mlxsw_sp, >>> { >>> struct mlxsw_sp_nexthop_group *nh_grp = nh->nhgi->nh_grp; >>> struct mlxsw_sp_nexthop_counter *nhct; >>> - void *ptr; >>> int err; >>> nhct = xa_load(&nh_grp->nhgi->nexthop_counters, nh->id); >>> @@ -3210,12 +3209,10 @@ mlxsw_sp_nexthop_sh_counter_get(struct >>> mlxsw_sp *mlxsw_sp, >>> if (IS_ERR(nhct)) >>> return nhct; >>> - ptr = xa_store(&nh_grp->nhgi->nexthop_counters, nh->id, nhct, >>> - GFP_KERNEL); >>> - if (IS_ERR(ptr)) { >>> - err = PTR_ERR(ptr); >>> + err = xa_err(xa_store(&nh_grp->nhgi->nexthop_counters, nh->id, >>> nhct, >>> + GFP_KERNEL)); >>> + if (err) >>> goto err_store; >>> - } >
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c index 800dfb64ec83..7d6d859cef3f 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c @@ -3197,7 +3197,6 @@ mlxsw_sp_nexthop_sh_counter_get(struct mlxsw_sp *mlxsw_sp, { struct mlxsw_sp_nexthop_group *nh_grp = nh->nhgi->nh_grp; struct mlxsw_sp_nexthop_counter *nhct; - void *ptr; int err; nhct = xa_load(&nh_grp->nhgi->nexthop_counters, nh->id); @@ -3210,12 +3209,10 @@ mlxsw_sp_nexthop_sh_counter_get(struct mlxsw_sp *mlxsw_sp, if (IS_ERR(nhct)) return nhct; - ptr = xa_store(&nh_grp->nhgi->nexthop_counters, nh->id, nhct, - GFP_KERNEL); - if (IS_ERR(ptr)) { - err = PTR_ERR(ptr); + err = xa_err(xa_store(&nh_grp->nhgi->nexthop_counters, nh->id, nhct, + GFP_KERNEL)); + if (err) goto err_store; - } return nhct;
It is meant to use xa_err() to extract the error encoded in the return value of xa_store(). Fixes: 44c2fbebe18a ("mlxsw: spectrum_router: Share nexthop counters in resilient groups") Signed-off-by: Yuan Can <yuancan@huawei.com> --- drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)