| Message ID | 20210306134718.17566-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | net: mellanox: mlx5: fix error return code of mlx5e_stats_flower() | expand |
On 2021-03-06 3:47 PM, Jia-Ju Bai wrote: > When mlx5e_tc_get_counter() returns NULL to counter or > mlx5_devcom_get_peer_data() returns NULL to peer_esw, no error return > code of mlx5e_stats_flower() is assigned. > To fix this bug, err is assigned with -EINVAL in these cases. > > Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> > Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> > --- > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > index 0da69b98f38f..1f2c9da7bd35 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > @@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct net_device *dev, struct mlx5e_priv *priv, > > if (mlx5e_is_offloaded_flow(flow) || flow_flag_test(flow, CT)) { > counter = mlx5e_tc_get_counter(flow); > - if (!counter) > + if (!counter) { > + err = -EINVAL; > goto errout; > + } > > mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse); > } > @@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct net_device *dev, struct mlx5e_priv *priv, > * un-offloaded while the other rule is offloaded. > */ > peer_esw = mlx5_devcom_get_peer_data(devcom, MLX5_DEVCOM_ESW_OFFLOADS); > - if (!peer_esw) > + if (!peer_esw) { > + err = -EINVAL; note here it's not an error. it could be there is no peer esw so just continue with the stats update. > goto out; > + } > > if (flow_flag_test(flow, DUP) && > flow_flag_test(flow->peer_flow, OFFLOADED)) { > @@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct net_device *dev, struct mlx5e_priv *priv, > u64 lastuse2; > > counter = mlx5e_tc_get_counter(flow->peer_flow); > - if (!counter) > + if (!counter) { > + err = -EINVAL; > goto no_peer_counter; > + } > mlx5_fc_query_cached(counter, &bytes2, &packets2, &lastuse2); > > bytes += bytes2; >
On 2021-03-09 10:20 AM, Roi Dayan wrote: > > > On 2021-03-06 3:47 PM, Jia-Ju Bai wrote: >> When mlx5e_tc_get_counter() returns NULL to counter or >> mlx5_devcom_get_peer_data() returns NULL to peer_esw, no error return >> code of mlx5e_stats_flower() is assigned. >> To fix this bug, err is assigned with -EINVAL in these cases. >> >> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> >> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> >> --- >> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >> b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >> index 0da69b98f38f..1f2c9da7bd35 100644 >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >> @@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct net_device *dev, >> struct mlx5e_priv *priv, >> if (mlx5e_is_offloaded_flow(flow) || flow_flag_test(flow, CT)) { >> counter = mlx5e_tc_get_counter(flow); >> - if (!counter) >> + if (!counter) { >> + err = -EINVAL; >> goto errout; >> + } >> mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse); >> } >> @@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct net_device *dev, >> struct mlx5e_priv *priv, >> * un-offloaded while the other rule is offloaded. >> */ >> peer_esw = mlx5_devcom_get_peer_data(devcom, >> MLX5_DEVCOM_ESW_OFFLOADS); >> - if (!peer_esw) >> + if (!peer_esw) { >> + err = -EINVAL; > > note here it's not an error. it could be there is no peer esw > so just continue with the stats update. > >> goto out; >> + } >> if (flow_flag_test(flow, DUP) && >> flow_flag_test(flow->peer_flow, OFFLOADED)) { >> @@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct net_device *dev, >> struct mlx5e_priv *priv, >> u64 lastuse2; >> counter = mlx5e_tc_get_counter(flow->peer_flow); >> - if (!counter) >> + if (!counter) { >> + err = -EINVAL; this change is problematic. the current goto is to do stats update with the first counter stats we got but if you now want to return an error then you probably should not do any update at all. >> goto no_peer_counter; >> + } >> mlx5_fc_query_cached(counter, &bytes2, &packets2, &lastuse2); >> bytes += bytes2; >> > >
On 2021/3/9 16:24, Roi Dayan wrote: > > > On 2021-03-09 10:20 AM, Roi Dayan wrote: >> >> >> On 2021-03-06 3:47 PM, Jia-Ju Bai wrote: >>> When mlx5e_tc_get_counter() returns NULL to counter or >>> mlx5_devcom_get_peer_data() returns NULL to peer_esw, no error return >>> code of mlx5e_stats_flower() is assigned. >>> To fix this bug, err is assigned with -EINVAL in these cases. >>> >>> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> >>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> >>> --- >>> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 +++++++++--- >>> 1 file changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>> b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>> index 0da69b98f38f..1f2c9da7bd35 100644 >>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>> @@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct net_device >>> *dev, struct mlx5e_priv *priv, >>> if (mlx5e_is_offloaded_flow(flow) || flow_flag_test(flow, CT)) { >>> counter = mlx5e_tc_get_counter(flow); >>> - if (!counter) >>> + if (!counter) { >>> + err = -EINVAL; >>> goto errout; >>> + } >>> mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse); >>> } >>> @@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct net_device >>> *dev, struct mlx5e_priv *priv, >>> * un-offloaded while the other rule is offloaded. >>> */ >>> peer_esw = mlx5_devcom_get_peer_data(devcom, >>> MLX5_DEVCOM_ESW_OFFLOADS); >>> - if (!peer_esw) >>> + if (!peer_esw) { >>> + err = -EINVAL; >> >> note here it's not an error. it could be there is no peer esw >> so just continue with the stats update. >> >>> goto out; >>> + } >>> if (flow_flag_test(flow, DUP) && >>> flow_flag_test(flow->peer_flow, OFFLOADED)) { >>> @@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct net_device >>> *dev, struct mlx5e_priv *priv, >>> u64 lastuse2; >>> counter = mlx5e_tc_get_counter(flow->peer_flow); >>> - if (!counter) >>> + if (!counter) { >>> + err = -EINVAL; > > this change is problematic. the current goto is to do stats update with > the first counter stats we got but if you now want to return an error > then you probably should not do any update at all. Thanks for your reply :) I am not sure whether an error code should be returned here? If so, flow_stats_update(...) should not be called here? Best wishes, Jia-Ju Bai > >>> goto no_peer_counter; >>> + } >>> mlx5_fc_query_cached(counter, &bytes2, &packets2, &lastuse2); >>> bytes += bytes2; >>> >> >>
On 2021-03-09 10:32 AM, Jia-Ju Bai wrote: > > > On 2021/3/9 16:24, Roi Dayan wrote: >> >> >> On 2021-03-09 10:20 AM, Roi Dayan wrote: >>> >>> >>> On 2021-03-06 3:47 PM, Jia-Ju Bai wrote: >>>> When mlx5e_tc_get_counter() returns NULL to counter or >>>> mlx5_devcom_get_peer_data() returns NULL to peer_esw, no error return >>>> code of mlx5e_stats_flower() is assigned. >>>> To fix this bug, err is assigned with -EINVAL in these cases. >>>> >>>> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> >>>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> >>>> --- >>>> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 +++++++++--- >>>> 1 file changed, 9 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>>> b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>>> index 0da69b98f38f..1f2c9da7bd35 100644 >>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>>> @@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct net_device >>>> *dev, struct mlx5e_priv *priv, >>>> if (mlx5e_is_offloaded_flow(flow) || flow_flag_test(flow, CT)) { >>>> counter = mlx5e_tc_get_counter(flow); >>>> - if (!counter) >>>> + if (!counter) { >>>> + err = -EINVAL; >>>> goto errout; >>>> + } >>>> mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse); >>>> } >>>> @@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct net_device >>>> *dev, struct mlx5e_priv *priv, >>>> * un-offloaded while the other rule is offloaded. >>>> */ >>>> peer_esw = mlx5_devcom_get_peer_data(devcom, >>>> MLX5_DEVCOM_ESW_OFFLOADS); >>>> - if (!peer_esw) >>>> + if (!peer_esw) { >>>> + err = -EINVAL; >>> >>> note here it's not an error. it could be there is no peer esw >>> so just continue with the stats update. >>> >>>> goto out; >>>> + } >>>> if (flow_flag_test(flow, DUP) && >>>> flow_flag_test(flow->peer_flow, OFFLOADED)) { >>>> @@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct net_device >>>> *dev, struct mlx5e_priv *priv, >>>> u64 lastuse2; >>>> counter = mlx5e_tc_get_counter(flow->peer_flow); >>>> - if (!counter) >>>> + if (!counter) { >>>> + err = -EINVAL; >> >> this change is problematic. the current goto is to do stats update with >> the first counter stats we got but if you now want to return an error >> then you probably should not do any update at all. > > Thanks for your reply :) > I am not sure whether an error code should be returned here? > If so, flow_stats_update(...) should not be called here? > > > Best wishes, > Jia-Ju Bai > basically flow and peer_flow should be valid and protected from changes, and counter should be valid. it looks like the check here is more of a sanity check if something goes wrong but shouldn't. you can just let it be, update the stats from the first queried counter. >> >>>> goto no_peer_counter; >>>> + } >>>> mlx5_fc_query_cached(counter, &bytes2, &packets2, &lastuse2); >>>> bytes += bytes2; >>>> >>> >>> >
On Tue, 2021-03-09 at 11:44 +0200, Roi Dayan wrote: > > > On 2021-03-09 10:32 AM, Jia-Ju Bai wrote: > > > > > > On 2021/3/9 16:24, Roi Dayan wrote: > > > > > > > > > On 2021-03-09 10:20 AM, Roi Dayan wrote: > > > > > > > > > > > > On 2021-03-06 3:47 PM, Jia-Ju Bai wrote: > > > > > When mlx5e_tc_get_counter() returns NULL to counter or > > > > > mlx5_devcom_get_peer_data() returns NULL to peer_esw, no > > > > > error return > > > > > code of mlx5e_stats_flower() is assigned. > > > > > To fix this bug, err is assigned with -EINVAL in these cases. > > > > > > > > > > Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> Hey Jia-Ju, What are the conditions for this robot to raise a flag? sometimes it is totally normal to abort a function and return 0.. i am just curious to know ? > > > > > Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> > > > > > --- > > > > > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 > > > > > +++++++++--- > > > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > > > > b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > > > > index 0da69b98f38f..1f2c9da7bd35 100644 > > > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > > > > @@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct > > > > > net_device > > > > > *dev, struct mlx5e_priv *priv, > > > > > if (mlx5e_is_offloaded_flow(flow) || > > > > > flow_flag_test(flow, CT)) { > > > > > counter = mlx5e_tc_get_counter(flow); > > > > > - if (!counter) > > > > > + if (!counter) { > > > > > + err = -EINVAL; > > > > > goto errout; > > > > > + } > > > > > mlx5_fc_query_cached(counter, &bytes, &packets, > > > > > &lastuse); > > > > > } > > > > > @@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct > > > > > net_device > > > > > *dev, struct mlx5e_priv *priv, > > > > > * un-offloaded while the other rule is offloaded. > > > > > */ > > > > > peer_esw = mlx5_devcom_get_peer_data(devcom, > > > > > MLX5_DEVCOM_ESW_OFFLOADS); > > > > > - if (!peer_esw) > > > > > + if (!peer_esw) { > > > > > + err = -EINVAL; > > > > This is not an error flow, i am curious what are the thresholds of this robot ? > > > > note here it's not an error. it could be there is no peer esw > > > > so just continue with the stats update. > > > > > > > > > goto out; > > > > > + } > > > > > if (flow_flag_test(flow, DUP) && > > > > > flow_flag_test(flow->peer_flow, OFFLOADED)) { > > > > > @@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct > > > > > net_device > > > > > *dev, struct mlx5e_priv *priv, > > > > > u64 lastuse2; > > > > > counter = mlx5e_tc_get_counter(flow->peer_flow); > > > > > - if (!counter) > > > > > + if (!counter) { > > > > > + err = -EINVAL; > > > > > > this change is problematic. the current goto is to do stats > > > update with > > > the first counter stats we got but if you now want to return an > > > error > > > then you probably should not do any update at all. > > > > Thanks for your reply :) > > I am not sure whether an error code should be returned here? > > If so, flow_stats_update(...) should not be called here? > > > > > > Best wishes, > > Jia-Ju Bai > > > > basically flow and peer_flow should be valid and protected from > changes, > and counter should be valid. > it looks like the check here is more of a sanity check if something > goes > wrong but shouldn't. you can just let it be, update the stats from > the > first queried counter. > Roi, let's consider returning an error code here, we shouldn't be silently returning if we are not expecting these errors, why would mlx5e_stats_flower() be called if stats are not offloaded ? Thanks, Saeed.
On 2021-03-12 12:47 AM, Saeed Mahameed wrote: > On Tue, 2021-03-09 at 11:44 +0200, Roi Dayan wrote: >> >> >> On 2021-03-09 10:32 AM, Jia-Ju Bai wrote: >>> >>> >>> On 2021/3/9 16:24, Roi Dayan wrote: >>>> >>>> >>>> On 2021-03-09 10:20 AM, Roi Dayan wrote: >>>>> >>>>> >>>>> On 2021-03-06 3:47 PM, Jia-Ju Bai wrote: >>>>>> When mlx5e_tc_get_counter() returns NULL to counter or >>>>>> mlx5_devcom_get_peer_data() returns NULL to peer_esw, no >>>>>> error return >>>>>> code of mlx5e_stats_flower() is assigned. >>>>>> To fix this bug, err is assigned with -EINVAL in these cases. >>>>>> >>>>>> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> > > Hey Jia-Ju, What are the conditions for this robot to raise a flag? > sometimes it is totally normal to abort a function and return 0.. i am > just curious to know ? > > >>>>>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> >>>>>> --- >>>>>> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 >>>>>> +++++++++--- >>>>>> 1 file changed, 9 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>>>>> b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>>>>> index 0da69b98f38f..1f2c9da7bd35 100644 >>>>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>>>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c >>>>>> @@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct >>>>>> net_device >>>>>> *dev, struct mlx5e_priv *priv, >>>>>> if (mlx5e_is_offloaded_flow(flow) || >>>>>> flow_flag_test(flow, CT)) { >>>>>> counter = mlx5e_tc_get_counter(flow); >>>>>> - if (!counter) >>>>>> + if (!counter) { >>>>>> + err = -EINVAL; >>>>>> goto errout; >>>>>> + } >>>>>> mlx5_fc_query_cached(counter, &bytes, &packets, >>>>>> &lastuse); >>>>>> } >>>>>> @@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct >>>>>> net_device >>>>>> *dev, struct mlx5e_priv *priv, >>>>>> * un-offloaded while the other rule is offloaded. >>>>>> */ >>>>>> peer_esw = mlx5_devcom_get_peer_data(devcom, >>>>>> MLX5_DEVCOM_ESW_OFFLOADS); >>>>>> - if (!peer_esw) >>>>>> + if (!peer_esw) { >>>>>> + err = -EINVAL; >>>>> > > This is not an error flow, i am curious what are the thresholds of this > robot ? > >>>>> note here it's not an error. it could be there is no peer esw >>>>> so just continue with the stats update. >>>>> >>>>>> goto out; >>>>>> + } >>>>>> if (flow_flag_test(flow, DUP) && >>>>>> flow_flag_test(flow->peer_flow, OFFLOADED)) { >>>>>> @@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct >>>>>> net_device >>>>>> *dev, struct mlx5e_priv *priv, >>>>>> u64 lastuse2; >>>>>> counter = mlx5e_tc_get_counter(flow->peer_flow); >>>>>> - if (!counter) >>>>>> + if (!counter) { >>>>>> + err = -EINVAL; >>>> >>>> this change is problematic. the current goto is to do stats >>>> update with >>>> the first counter stats we got but if you now want to return an >>>> error >>>> then you probably should not do any update at all. >>> >>> Thanks for your reply :) >>> I am not sure whether an error code should be returned here? >>> If so, flow_stats_update(...) should not be called here? >>> >>> >>> Best wishes, >>> Jia-Ju Bai >>> >> >> basically flow and peer_flow should be valid and protected from >> changes, >> and counter should be valid. >> it looks like the check here is more of a sanity check if something >> goes >> wrong but shouldn't. you can just let it be, update the stats from >> the >> first queried counter. >> > > Roi, let's consider returning an error code here, we shouldn't be > silently returning if we are not expecting these errors, > > why would mlx5e_stats_flower() be called if stats are not offloaded ? > > Thanks, > Saeed. > > yes we can return an error if peer counter missing. I just pointed out we should probably not call flow_stats_update() if we do return an error. the other option, as today, is updating the stats with first counter stats and because of that we didn't return an error.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c index 0da69b98f38f..1f2c9da7bd35 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c @@ -4380,8 +4380,10 @@ int mlx5e_stats_flower(struct net_device *dev, struct mlx5e_priv *priv, if (mlx5e_is_offloaded_flow(flow) || flow_flag_test(flow, CT)) { counter = mlx5e_tc_get_counter(flow); - if (!counter) + if (!counter) { + err = -EINVAL; goto errout; + } mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse); } @@ -4390,8 +4392,10 @@ int mlx5e_stats_flower(struct net_device *dev, struct mlx5e_priv *priv, * un-offloaded while the other rule is offloaded. */ peer_esw = mlx5_devcom_get_peer_data(devcom, MLX5_DEVCOM_ESW_OFFLOADS); - if (!peer_esw) + if (!peer_esw) { + err = -EINVAL; goto out; + } if (flow_flag_test(flow, DUP) && flow_flag_test(flow->peer_flow, OFFLOADED)) { @@ -4400,8 +4404,10 @@ int mlx5e_stats_flower(struct net_device *dev, struct mlx5e_priv *priv, u64 lastuse2; counter = mlx5e_tc_get_counter(flow->peer_flow); - if (!counter) + if (!counter) { + err = -EINVAL; goto no_peer_counter; + } mlx5_fc_query_cached(counter, &bytes2, &packets2, &lastuse2); bytes += bytes2;
When mlx5e_tc_get_counter() returns NULL to counter or mlx5_devcom_get_peer_data() returns NULL to peer_esw, no error return code of mlx5e_stats_flower() is assigned. To fix this bug, err is assigned with -EINVAL in these cases. Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> --- drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)