* [patch net-next] devlink: remove reload failed checks in params get/set callbacks
@ 2023-07-12 11:37 Jiri Pirko
2023-07-12 13:47 ` Ido Schimmel
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Pirko @ 2023-07-12 11:37 UTC (permalink / raw)
To: netdev; +Cc: kuba, pabeni, davem, edumazet, moshe, idosch
From: Jiri Pirko <jiri@nvidia.com>
The checks in question were introduced by
commit 6b4db2e528f6 ("devlink: Fix use-after-free after a failed reload").
Back then, it was a possible fix. Alternative way to fix this was to
make sure drivers register/unregister params in the code where it is
ensured that the data accessed by params callbacks are available.
But that was problematic as the list of params wes static durint
devlink instance being registered.
Eventually this limitation was lifted and also the alternative fix
(which also fixed another issue) was done for mlxsw by
commit 74cbc3c03c82 ("mlxsw: spectrum_acl_tcam: Move devlink param to TCAM code").
The checks are no longer relevant, each driver should make sure to
register/unregister params alongside with the data it touches. Remove
the checks.
Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
net/devlink/leftover.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index 1f00f874471f..5128b9c7eea8 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -3946,7 +3946,7 @@ static int devlink_param_get(struct devlink *devlink,
const struct devlink_param *param,
struct devlink_param_gset_ctx *ctx)
{
- if (!param->get || devlink->reload_failed)
+ if (!param->get)
return -EOPNOTSUPP;
return param->get(devlink, param->id, ctx);
}
@@ -3955,7 +3955,7 @@ static int devlink_param_set(struct devlink *devlink,
const struct devlink_param *param,
struct devlink_param_gset_ctx *ctx)
{
- if (!param->set || devlink->reload_failed)
+ if (!param->set)
return -EOPNOTSUPP;
return param->set(devlink, param->id, ctx);
}
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [patch net-next] devlink: remove reload failed checks in params get/set callbacks
2023-07-12 11:37 [patch net-next] devlink: remove reload failed checks in params get/set callbacks Jiri Pirko
@ 2023-07-12 13:47 ` Ido Schimmel
2023-07-12 15:20 ` Jiri Pirko
0 siblings, 1 reply; 6+ messages in thread
From: Ido Schimmel @ 2023-07-12 13:47 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, kuba, pabeni, davem, edumazet, moshe
On Wed, Jul 12, 2023 at 01:37:10PM +0200, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
>
> The checks in question were introduced by
> commit 6b4db2e528f6 ("devlink: Fix use-after-free after a failed reload").
>
> Back then, it was a possible fix. Alternative way to fix this was to
> make sure drivers register/unregister params in the code where it is
> ensured that the data accessed by params callbacks are available.
> But that was problematic as the list of params wes static durint
s/wes/was/
s/durint/during/
> devlink instance being registered.
>
> Eventually this limitation was lifted and also the alternative fix
> (which also fixed another issue) was done for mlxsw by
> commit 74cbc3c03c82 ("mlxsw: spectrum_acl_tcam: Move devlink param to TCAM code").
>
> The checks are no longer relevant, each driver should make sure to
> register/unregister params alongside with the data it touches. Remove
> the checks.
>
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
I don't see how we can hit the issue after 74cbc3c03c82 and any driver
that suffers from this issue should have already seen it after
7d7e9169a3ec, so this patch looks reasonable to me.
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch net-next] devlink: remove reload failed checks in params get/set callbacks
2023-07-12 13:47 ` Ido Schimmel
@ 2023-07-12 15:20 ` Jiri Pirko
2023-07-12 19:21 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Pirko @ 2023-07-12 15:20 UTC (permalink / raw)
To: Ido Schimmel; +Cc: netdev, kuba, pabeni, davem, edumazet, moshe
Wed, Jul 12, 2023 at 03:47:29PM CEST, idosch@nvidia.com wrote:
>On Wed, Jul 12, 2023 at 01:37:10PM +0200, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>>
>> The checks in question were introduced by
>> commit 6b4db2e528f6 ("devlink: Fix use-after-free after a failed reload").
>>
>> Back then, it was a possible fix. Alternative way to fix this was to
>> make sure drivers register/unregister params in the code where it is
>> ensured that the data accessed by params callbacks are available.
>> But that was problematic as the list of params wes static durint
>
>s/wes/was/
>s/durint/during/
Maintainers, I will send v2 with these typos fixed tomorrow, if these
are not any other comments.
>
>> devlink instance being registered.
>>
>> Eventually this limitation was lifted and also the alternative fix
>> (which also fixed another issue) was done for mlxsw by
>> commit 74cbc3c03c82 ("mlxsw: spectrum_acl_tcam: Move devlink param to TCAM code").
>>
>> The checks are no longer relevant, each driver should make sure to
>> register/unregister params alongside with the data it touches. Remove
>> the checks.
>>
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>
>I don't see how we can hit the issue after 74cbc3c03c82 and any driver
>that suffers from this issue should have already seen it after
>7d7e9169a3ec, so this patch looks reasonable to me.
>
>Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch net-next] devlink: remove reload failed checks in params get/set callbacks
2023-07-12 15:20 ` Jiri Pirko
@ 2023-07-12 19:21 ` Jakub Kicinski
2023-07-13 9:00 ` Jiri Pirko
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2023-07-12 19:21 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Ido Schimmel, netdev, pabeni, davem, edumazet, moshe
On Wed, 12 Jul 2023 17:20:40 +0200 Jiri Pirko wrote:
> >> Back then, it was a possible fix. Alternative way to fix this was to
> >> make sure drivers register/unregister params in the code where it is
> >> ensured that the data accessed by params callbacks are available.
> >> But that was problematic as the list of params wes static durint
> >
> >s/wes/was/
> >s/durint/during/
>
> Maintainers, I will send v2 with these typos fixed tomorrow, if these
> are not any other comments.
Feel free to toss in
pw-bot: changes-requested
so we don't have to update the status manually.
The commit message would benefit from a rewrite, TBH I don't understand
half of it, specially:
Alternative way to fix this was to make sure drivers
register/unregister params in the code where it is ensured that
the data accessed by params callbacks are available.
Can't parse.
list of params [was] static [during] devlink instance being
registered.
You mean that list of params can't change after the instance was
registered?
register/unregister params alongside with the data it touches
Meaning params for a sub-object are registered when the sub-object
is registered? An example could help clarify the meaning.
> >> devlink instance being registered.
> >>
> >> Eventually this limitation was lifted and also the alternative fix
> >> (which also fixed another issue) was done for mlxsw by
> >> commit 74cbc3c03c82 ("mlxsw: spectrum_acl_tcam: Move devlink param to TCAM code").
> >>
> >> The checks are no longer relevant, each driver should make sure to
> >> register/unregister params alongside with the data it touches. Remove
> >> the checks.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch net-next] devlink: remove reload failed checks in params get/set callbacks
2023-07-12 19:21 ` Jakub Kicinski
@ 2023-07-13 9:00 ` Jiri Pirko
2023-07-13 15:55 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Pirko @ 2023-07-13 9:00 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Ido Schimmel, netdev, pabeni, davem, edumazet, moshe
Wed, Jul 12, 2023 at 09:21:03PM CEST, kuba@kernel.org wrote:
>On Wed, 12 Jul 2023 17:20:40 +0200 Jiri Pirko wrote:
>> >> Back then, it was a possible fix. Alternative way to fix this was to
>> >> make sure drivers register/unregister params in the code where it is
>> >> ensured that the data accessed by params callbacks are available.
>> >> But that was problematic as the list of params wes static durint
>> >
>> >s/wes/was/
>> >s/durint/during/
>>
>> Maintainers, I will send v2 with these typos fixed tomorrow, if these
>> are not any other comments.
>
>Feel free to toss in
>
>pw-bot: changes-requested
I see, is this documented somewhere?
>
>so we don't have to update the status manually.
>
>The commit message would benefit from a rewrite, TBH I don't understand
>half of it, specially:
Will do.
>
> Alternative way to fix this was to make sure drivers
> register/unregister params in the code where it is ensured that
> the data accessed by params callbacks are available.
>
>Can't parse.
>
> list of params [was] static [during] devlink instance being
> registered.
>
>You mean that list of params can't change after the instance was
>registered?
Yeah, that was a limitation in history IIRC.
>
> register/unregister params alongside with the data it touches
>
>Meaning params for a sub-object are registered when the sub-object
>is registered? An example could help clarify the meaning.
>
>> >> devlink instance being registered.
>> >>
>> >> Eventually this limitation was lifted and also the alternative fix
>> >> (which also fixed another issue) was done for mlxsw by
>> >> commit 74cbc3c03c82 ("mlxsw: spectrum_acl_tcam: Move devlink param to TCAM code").
>> >>
>> >> The checks are no longer relevant, each driver should make sure to
>> >> register/unregister params alongside with the data it touches. Remove
>> >> the checks.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch net-next] devlink: remove reload failed checks in params get/set callbacks
2023-07-13 9:00 ` Jiri Pirko
@ 2023-07-13 15:55 ` Jakub Kicinski
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2023-07-13 15:55 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Ido Schimmel, netdev, pabeni, davem, edumazet, moshe
On Thu, 13 Jul 2023 11:00:51 +0200 Jiri Pirko wrote:
> >Feel free to toss in
> >
> >pw-bot: changes-requested
>
> I see, is this documented somewhere?
Aha, recently. I try to mark emails with important stuff with [ANN]
maybe we need a better form of broadcast :S
Quoting documentation:
Updating patch status
~~~~~~~~~~~~~~~~~~~~~
Contributors and reviewers do not have the permissions to update patch
state directly in patchwork. Patchwork doesn't expose much information
about the history of the state of patches, therefore having multiple
people update the state leads to confusion.
Instead of delegating patchwork permissions netdev uses a simple mail
bot which looks for special commands/lines within the emails sent to
the mailing list. For example to mark a series as Changes Requested
one needs to send the following line anywhere in the email thread::
pw-bot: changes-requested
As a result the bot will set the entire series to Changes Requested.
This may be useful when author discovers a bug in their own series
and wants to prevent it from getting applied.
The use of the bot is entirely optional, if in doubt ignore its existence
completely. Maintainers will classify and update the state of the patches
themselves. No email should ever be sent to the list with the main purpose
of communicating with the bot, the bot commands should be seen as metadata.
The use of the bot is restricted to authors of the patches (the ``From:``
header on patch submission and command must match!), maintainers of
the modified code according to the MAINTAINERS file (again, ``From:``
must match the MAINTAINERS entry) and a handful of senior reviewers.
Bot records its activity here:
https://patchwork.hopto.org/pw-bot.html
See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#updating-patch-status
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-07-13 15:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-12 11:37 [patch net-next] devlink: remove reload failed checks in params get/set callbacks Jiri Pirko
2023-07-12 13:47 ` Ido Schimmel
2023-07-12 15:20 ` Jiri Pirko
2023-07-12 19:21 ` Jakub Kicinski
2023-07-13 9:00 ` Jiri Pirko
2023-07-13 15:55 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).