| Message ID | 20230712113710.2520129-1-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] devlink: remove reload failed checks in params get/set callbacks | expand |
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>
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!
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.
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.
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
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); }