Attempting to rollback the activation of the current master when
the current master has not been activated is bad. priv->cur_chan
and priv->cur_adap are both still zeroed out and the rollback
may result in attempts to revert an of changeset that has not been
applied and do result in calls to both del and put the zeroed out
i2c_adapter. Maybe it crashes, or whatever, but it's bad in any
case.
Fixes: e9d1a0a41d44 ("i2c: mux: demux-pinctrl: Fix an error handling path in 'i2c_demux_pinctrl_probe()'")
Cc: Christophe JAILLET <[email protected]>
Signed-off-by: Peter Rosin <[email protected]>
---
drivers/i2c/muxes/i2c-demux-pinctrl.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Hi!
Either that, or I'm missing something. I found this issue when
reading code, and have made no tests to verify if my analysis
is correct.
Cheers,
Peter
diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c b/drivers/i2c/muxes/i2c-demux-pinctrl.c
index 5365199a31f4..f7a7405d4350 100644
--- a/drivers/i2c/muxes/i2c-demux-pinctrl.c
+++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
@@ -261,7 +261,7 @@ static int i2c_demux_pinctrl_probe(struct platform_device *pdev)
err = device_create_file(&pdev->dev, &dev_attr_available_masters);
if (err)
- goto err_rollback;
+ goto err_rollback_activation;
err = device_create_file(&pdev->dev, &dev_attr_current_master);
if (err)
@@ -271,8 +271,9 @@ static int i2c_demux_pinctrl_probe(struct platform_device *pdev)
err_rollback_available:
device_remove_file(&pdev->dev, &dev_attr_available_masters);
-err_rollback:
+err_rollback_activation:
i2c_demux_deactivate_master(priv);
+err_rollback:
for (j = 0; j < i; j++) {
of_node_put(priv->chan[j].parent_np);
of_changeset_destroy(&priv->chan[j].chgset);
--
2.20.1
Le 02/03/2022 à 12:22, Peter Rosin a écrit :
> Attempting to rollback the activation of the current master when
> the current master has not been activated is bad. priv->cur_chan
> and priv->cur_adap are both still zeroed out and the rollback
> may result in attempts to revert an of changeset that has not been
> applied and do result in calls to both del and put the zeroed out
> i2c_adapter. Maybe it crashes, or whatever, but it's bad in any
> case.
>
> Fixes: e9d1a0a41d44 ("i2c: mux: demux-pinctrl: Fix an error handling path in 'i2c_demux_pinctrl_probe()'")
> Cc: Christophe JAILLET <[email protected]>
> Signed-off-by: Peter Rosin <[email protected]>
> ---
> drivers/i2c/muxes/i2c-demux-pinctrl.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> Hi!
>
> Either that, or I'm missing something. I found this issue when
> reading code, and have made no tests to verify if my analysis
> is correct.
>
> Cheers,
> Peter
>
> diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> index 5365199a31f4..f7a7405d4350 100644
> --- a/drivers/i2c/muxes/i2c-demux-pinctrl.c
> +++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> @@ -261,7 +261,7 @@ static int i2c_demux_pinctrl_probe(struct platform_device *pdev)
>
> err = device_create_file(&pdev->dev, &dev_attr_available_masters);
> if (err)
> - goto err_rollback;
> + goto err_rollback_activation;
>
> err = device_create_file(&pdev->dev, &dev_attr_current_master);
> if (err)
> @@ -271,8 +271,9 @@ static int i2c_demux_pinctrl_probe(struct platform_device *pdev)
>
> err_rollback_available:
> device_remove_file(&pdev->dev, &dev_attr_available_masters);
> -err_rollback:
> +err_rollback_activation:
> i2c_demux_deactivate_master(priv);
> +err_rollback:
> for (j = 0; j < i; j++) {
> of_node_put(priv->chan[j].parent_np);
> of_changeset_destroy(&priv->chan[j].chgset);
For what it worth, LGTM.
CJ
On Wed, Mar 02, 2022 at 10:17:20PM +0100, Wolfram Sang wrote:
>
> > Attempting to rollback the activation of the current master when
> > the current master has not been activated is bad. priv->cur_chan
>
> But the master is activated?
>
> 259 /* switch to first parent as active master */
> 260 i2c_demux_activate_master(priv, 0);
>
> Or am I missing something? :)
But _this_ code is affected by the problem you describe, or?
237 if (!adap_np) {
238 dev_err(&pdev->dev, "can't get phandle for parent %d\n", i);
239 err = -ENOENT;
240 goto err_rollback;
241 }
> Attempting to rollback the activation of the current master when
> the current master has not been activated is bad. priv->cur_chan
But the master is activated?
259 /* switch to first parent as active master */
260 i2c_demux_activate_master(priv, 0);
Or am I missing something? :)
On 2022-03-02 22:22, Wolfram Sang wrote:
> On Wed, Mar 02, 2022 at 10:17:20PM +0100, Wolfram Sang wrote:
>>
>>> Attempting to rollback the activation of the current master when
>>> the current master has not been activated is bad. priv->cur_chan
>>
>> But the master is activated?
>>
>> 259 /* switch to first parent as active master */
>> 260 i2c_demux_activate_master(priv, 0);
>>
>> Or am I missing something? :)
>
> But _this_ code is affected by the problem you describe, or?
>
> 237 if (!adap_np) {
> 238 dev_err(&pdev->dev, "can't get phandle for parent %d\n", i);
> 239 err = -ENOENT;
> 240 goto err_rollback;
> 241 }
>
Yes. That is the error path that, if taken, results in the faulty
deactivation. Sorry for not spelling it out...
Cheers,
Peter
On Wed, Mar 02, 2022 at 12:22:35PM +0100, Peter Rosin wrote:
> Attempting to rollback the activation of the current master when
> the current master has not been activated is bad. priv->cur_chan
> and priv->cur_adap are both still zeroed out and the rollback
> may result in attempts to revert an of changeset that has not been
> applied and do result in calls to both del and put the zeroed out
> i2c_adapter. Maybe it crashes, or whatever, but it's bad in any
> case.
>
> Fixes: e9d1a0a41d44 ("i2c: mux: demux-pinctrl: Fix an error handling path in 'i2c_demux_pinctrl_probe()'")
> Cc: Christophe JAILLET <[email protected]>
> Signed-off-by: Peter Rosin <[email protected]>
Applied to for-next, thanks!