[PATCH] can: etas_es58x: fix error handling

[PATCH] can: etas_es58x: fix error handling

[Pavel Skripkin]

When register_candev() fails there are 2 possible device states:
NETREG_UNINITIALIZED and NETREG_UNREGISTERED. None of them are suitable
for calling unregister_candev(), because of following checks in
unregister_netdevice_many():

	if (dev->reg_state == NETREG_UNINITIALIZED)
		WARN_ON(1);
...
	BUG_ON(dev->reg_state != NETREG_REGISTERED);

To avoid possible BUG_ON or WARN_ON let's free current netdev before
returning from es58x_init_netdev() and leave others (registered)
net devices for es58x_free_netdevs().

Fixes: 004653f0abf2 ("can: etas_es58x: add es58x_free_netdevs() to factorize code")
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/net/can/usb/etas_es58x/es58x_core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index 96a13c770e4a..41c721f2fbbe 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -2098,8 +2098,11 @@ static int es58x_init_netdev(struct es58x_device *es58x_dev, int channel_idx)
 	netdev->flags |= IFF_ECHO;	/* We support local echo */
 
 	ret = register_candev(netdev);
-	if (ret)
+	if (ret) {
+		free_candev(netdev);
+		es58x_dev->netdev[channel_idx] = NULL;
 		return ret;
+	}
 
 	netdev_queue_set_dql_min_limit(netdev_get_tx_queue(netdev, 0),
 				       es58x_dev->param->dql_min_limit);
-- 
2.33.1

Re: [PATCH] can: etas_es58x: fix error handling

[Vincent MAILHOL]

Hi Pavel,

Thanks for the patch!

On Mon. 15 Nov 2021 at 05:58, Pavel Skripkin <paskripkin@gmail.com> wrote:
> When register_candev() fails there are 2 possible device states:
> NETREG_UNINITIALIZED and NETREG_UNREGISTERED. None of them are suitable
> for calling unregister_candev(), because of following checks in
> unregister_netdevice_many():
>
>         if (dev->reg_state == NETREG_UNINITIALIZED)
>                 WARN_ON(1);
> ...
>         BUG_ON(dev->reg_state != NETREG_REGISTERED);
>
> To avoid possible BUG_ON or WARN_ON let's free current netdev before
> returning from es58x_init_netdev() and leave others (registered)
> net devices for es58x_free_netdevs().
>
> Fixes: 004653f0abf2 ("can: etas_es58x: add es58x_free_netdevs() to factorize code")

Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X
CAN USB interfaces")

The bug existed from the initial commit.  Prior to the
introduction of es58x_free_netdevs(), unregister_candev() was
called in the error handling of es58x_probe():

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/can/usb/etas_es58x/es58x_core.c?id=8537257874e949a59c834cecfd5a063e11b64b0b#n2234

> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
>  drivers/net/can/usb/etas_es58x/es58x_core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
> index 96a13c770e4a..41c721f2fbbe 100644
> --- a/drivers/net/can/usb/etas_es58x/es58x_core.c
> +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
> @@ -2098,8 +2098,11 @@ static int es58x_init_netdev(struct es58x_device *es58x_dev, int channel_idx)
>         netdev->flags |= IFF_ECHO;      /* We support local echo */
>
>         ret = register_candev(netdev);
> -       if (ret)
> +       if (ret) {
> +               free_candev(netdev);
> +               es58x_dev->netdev[channel_idx] = NULL;

A nitpick, but if you don’t mind, I would prefer to set
es58x_dev->netdev[channel_idx] after register_candev() succeeds
so that we do not have to reset it to NULL in the error handling.

diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c
b/drivers/net/can/usb/etas_es58x/es58x_core.c
index ce2b9e1ce3af..fb0daad9b9c8 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -2091,18 +2091,20 @@ static int es58x_init_netdev(struct
es58x_device *es58x_dev, int channel_idx)
                return -ENOMEM;
        }
        SET_NETDEV_DEV(netdev, dev);
-       es58x_dev->netdev[channel_idx] = netdev;
        es58x_init_priv(es58x_dev, es58x_priv(netdev), channel_idx);

        netdev->netdev_ops = &es58x_netdev_ops;
        netdev->flags |= IFF_ECHO;      /* We support local echo */

        ret = register_candev(netdev);
-       if (ret)
+       if (ret) {
+               free_candev(netdev);
                return ret;
+       }

        netdev_queue_set_dql_min_limit(netdev_get_tx_queue(netdev, 0),
                                       es58x_dev->param->dql_min_limit);
+       es58x_dev->netdev[channel_idx] = netdev;

        return ret;
 }

>                 return ret;
> +       }
>
>         netdev_queue_set_dql_min_limit(netdev_get_tx_queue(netdev, 0),
>                                        es58x_dev->param->dql_min_limit);

Re: [PATCH] can: etas_es58x: fix error handling

[Pavel Skripkin]

On 11/15/21 08:27, Vincent MAILHOL wrote:
> Hi Pavel,
> 
> Thanks for the patch!
> 
> On Mon. 15 Nov 2021 at 05:58, Pavel Skripkin <paskripkin@gmail.com> wrote:
>> When register_candev() fails there are 2 possible device states:
>> NETREG_UNINITIALIZED and NETREG_UNREGISTERED. None of them are suitable
>> for calling unregister_candev(), because of following checks in
>> unregister_netdevice_many():
>>
>>         if (dev->reg_state == NETREG_UNINITIALIZED)
>>                 WARN_ON(1);
>> ...
>>         BUG_ON(dev->reg_state != NETREG_REGISTERED);
>>
>> To avoid possible BUG_ON or WARN_ON let's free current netdev before
>> returning from es58x_init_netdev() and leave others (registered)
>> net devices for es58x_free_netdevs().
>>
>> Fixes: 004653f0abf2 ("can: etas_es58x: add es58x_free_netdevs() to factorize code")
> 

Hi, Vincent!

> Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X
> CAN USB interfaces")
> 
> The bug existed from the initial commit.  Prior to the
> introduction of es58x_free_netdevs(), unregister_candev() was
> called in the error handling of es58x_probe():
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/can/usb/etas_es58x/es58x_core.c?id=8537257874e949a59c834cecfd5a063e11b64b0b#n2234
> 

I see, will fix in v2

>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
>> ---
>>  drivers/net/can/usb/etas_es58x/es58x_core.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
>> index 96a13c770e4a..41c721f2fbbe 100644
>> --- a/drivers/net/can/usb/etas_es58x/es58x_core.c
>> +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
>> @@ -2098,8 +2098,11 @@ static int es58x_init_netdev(struct es58x_device *es58x_dev, int channel_idx)
>>         netdev->flags |= IFF_ECHO;      /* We support local echo */
>>
>>         ret = register_candev(netdev);
>> -       if (ret)
>> +       if (ret) {
>> +               free_candev(netdev);
>> +               es58x_dev->netdev[channel_idx] = NULL;
> 
> A nitpick, but if you don’t mind, I would prefer to set
> es58x_dev->netdev[channel_idx] after register_candev() succeeds
> so that we do not have to reset it to NULL in the error handling.
> 
> diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c
> b/drivers/net/can/usb/etas_es58x/es58x_core.c
> index ce2b9e1ce3af..fb0daad9b9c8 100644
> --- a/drivers/net/can/usb/etas_es58x/es58x_core.c
> +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
> @@ -2091,18 +2091,20 @@ static int es58x_init_netdev(struct
> es58x_device *es58x_dev, int channel_idx)
>                  return -ENOMEM;
>          }
>          SET_NETDEV_DEV(netdev, dev);
> -       es58x_dev->netdev[channel_idx] = netdev;
>          es58x_init_priv(es58x_dev, es58x_priv(netdev), channel_idx);
> 
>          netdev->netdev_ops = &es58x_netdev_ops;
>          netdev->flags |= IFF_ECHO;      /* We support local echo */
> 
>          ret = register_candev(netdev);
> -       if (ret)
> +       if (ret) {
> +               free_candev(netdev);
>                  return ret;
> +       }
> 
>          netdev_queue_set_dql_min_limit(netdev_get_tx_queue(netdev, 0),
>                                         es58x_dev->param->dql_min_limit);
> +       es58x_dev->netdev[channel_idx] = netdev;
> 
>          return ret;
>   }
> 

Also will do in v2. Thank you for your review :)





With regards,
Pavel Skripkin

[PATCH v2] can: etas_es58x: fix error handling

[Pavel Skripkin]

When register_candev() fails there are 2 possible device states:
NETREG_UNINITIALIZED and NETREG_UNREGISTERED. None of them are suitable
for calling unregister_candev(), because of following checks in
unregister_netdevice_many():

	if (dev->reg_state == NETREG_UNINITIALIZED)
		WARN_ON(1);
...
	BUG_ON(dev->reg_state != NETREG_REGISTERED);

To avoid possible BUG_ON or WARN_ON let's free current netdev before
returning from es58x_init_netdev() and leave others (registered)
net devices for es58x_free_netdevs().

Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces")
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---

Changes in v2:
	- Fixed Fixes: tag
	- Moved es58x_dev->netdev[channel_idx] initialization at the end
	  of the function

---
 drivers/net/can/usb/etas_es58x/es58x_core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index 96a13c770e4a..b3af8f2e32ac 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -2091,19 +2091,22 @@ static int es58x_init_netdev(struct es58x_device *es58x_dev, int channel_idx)
 		return -ENOMEM;
 	}
 	SET_NETDEV_DEV(netdev, dev);
-	es58x_dev->netdev[channel_idx] = netdev;
 	es58x_init_priv(es58x_dev, es58x_priv(netdev), channel_idx);
 
 	netdev->netdev_ops = &es58x_netdev_ops;
 	netdev->flags |= IFF_ECHO;	/* We support local echo */
 
 	ret = register_candev(netdev);
-	if (ret)
+	if (ret) {
+		free_candev(netdev);
 		return ret;
+	}
 
 	netdev_queue_set_dql_min_limit(netdev_get_tx_queue(netdev, 0),
 				       es58x_dev->param->dql_min_limit);
 
+	es58x_dev->netdev[channel_idx] = netdev;
+
 	return ret;
 }
 
-- 
2.33.1

Re: [PATCH v2] can: etas_es58x: fix error handling

[Johan Hovold]

On Mon, Nov 15, 2021 at 10:51:24AM +0300, Pavel Skripkin wrote:
> When register_candev() fails there are 2 possible device states:
> NETREG_UNINITIALIZED and NETREG_UNREGISTERED. None of them are suitable
> for calling unregister_candev(), because of following checks in
> unregister_netdevice_many():
> 
> 	if (dev->reg_state == NETREG_UNINITIALIZED)
> 		WARN_ON(1);
> ...
> 	BUG_ON(dev->reg_state != NETREG_REGISTERED);
> 
> To avoid possible BUG_ON or WARN_ON let's free current netdev before
> returning from es58x_init_netdev() and leave others (registered)
> net devices for es58x_free_netdevs().
> 
> Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces")
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
> 
> Changes in v2:
> 	- Fixed Fixes: tag
> 	- Moved es58x_dev->netdev[channel_idx] initialization at the end
> 	  of the function
> 
> ---
>  drivers/net/can/usb/etas_es58x/es58x_core.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
> index 96a13c770e4a..b3af8f2e32ac 100644
> --- a/drivers/net/can/usb/etas_es58x/es58x_core.c
> +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
> @@ -2091,19 +2091,22 @@ static int es58x_init_netdev(struct es58x_device *es58x_dev, int channel_idx)
>  		return -ENOMEM;
>  	}
>  	SET_NETDEV_DEV(netdev, dev);
> -	es58x_dev->netdev[channel_idx] = netdev;
>  	es58x_init_priv(es58x_dev, es58x_priv(netdev), channel_idx);
>  
>  	netdev->netdev_ops = &es58x_netdev_ops;
>  	netdev->flags |= IFF_ECHO;	/* We support local echo */
>  
>  	ret = register_candev(netdev);
> -	if (ret)
> +	if (ret) {
> +		free_candev(netdev);
>  		return ret;
> +	}
>  
>  	netdev_queue_set_dql_min_limit(netdev_get_tx_queue(netdev, 0),
>  				       es58x_dev->param->dql_min_limit);
>  
> +	es58x_dev->netdev[channel_idx] = netdev;
> +

Just a drive-by comment: 

Are you sure about this move of the netdev[channel_idx] initialisation?
What happens if the registered can device is opened before you
initialise the pointer? NULL-deref in es58x_send_msg()?

You generally want the driver data fully initialised before you register
the device so this looks broken.

And either way it is arguably an unrelated change that should go in a
separate patch explaining why it is needed and safe.

>  	return ret;
>  }

Johan

Re: [PATCH v2] can: etas_es58x: fix error handling

[Pavel Skripkin]

On 11/15/21 11:11, Johan Hovold wrote:
> Just a drive-by comment:
> 
> Are you sure about this move of the netdev[channel_idx] initialisation?
> What happens if the registered can device is opened before you
> initialise the pointer? NULL-deref in es58x_send_msg()?
> 
> You generally want the driver data fully initialised before you register
> the device so this looks broken.
> 
> And either way it is arguably an unrelated change that should go in a
> separate patch explaining why it is needed and safe.
> 


It was suggested by Vincent who is the maintainer of this driver [1].



[1] 
https://lore.kernel.org/linux-can/CAMZ6Rq+orfUuUCCgeWyGc7P0vp3t-yjf_g9H=Jhk43f1zXGfDQ@mail.gmail.com/



With regards,
Pavel Skripkin

Re: [PATCH v2] can: etas_es58x: fix error handling

[Johan Hovold]

On Mon, Nov 15, 2021 at 11:15:07AM +0300, Pavel Skripkin wrote:
> On 11/15/21 11:11, Johan Hovold wrote:
> > Just a drive-by comment:
> > 
> > Are you sure about this move of the netdev[channel_idx] initialisation?
> > What happens if the registered can device is opened before you
> > initialise the pointer? NULL-deref in es58x_send_msg()?
> > 
> > You generally want the driver data fully initialised before you register
> > the device so this looks broken.
> > 
> > And either way it is arguably an unrelated change that should go in a
> > separate patch explaining why it is needed and safe.
> > 
> 
> 
> It was suggested by Vincent who is the maintainer of this driver [1].

Yeah, I saw that, but that doesn't necessarily mean it is correct.

You're still responsible for the changes you make and need to be able to
argue why they are correct.

Johan

Re: [PATCH v2] can: etas_es58x: fix error handling

[Pavel Skripkin]

On 11/15/21 11:16, Johan Hovold wrote:
> On Mon, Nov 15, 2021 at 11:15:07AM +0300, Pavel Skripkin wrote:
>> On 11/15/21 11:11, Johan Hovold wrote:
>> > Just a drive-by comment:
>> > 
>> > Are you sure about this move of the netdev[channel_idx] initialisation?
>> > What happens if the registered can device is opened before you
>> > initialise the pointer? NULL-deref in es58x_send_msg()?
>> > 
>> > You generally want the driver data fully initialised before you register
>> > the device so this looks broken.
>> > 
>> > And either way it is arguably an unrelated change that should go in a
>> > separate patch explaining why it is needed and safe.
>> > 
>> 
>> 
>> It was suggested by Vincent who is the maintainer of this driver [1].
> 
> Yeah, I saw that, but that doesn't necessarily mean it is correct.
> 
> You're still responsible for the changes you make and need to be able to
> argue why they are correct.
> 

Sure! I should have check it before sending v2 :( My bad, sorry. I see 
now, that there is possible calltrace which can hit NULL defer.

One thing I am wondering about is why in some code parts there are 
validation checks for es58x_dev->netdev[i] and in others they are missing.

Anyway, it's completely out of scope of current patch, I am going to 
resend v1 with fixed Fixes tag. Thank you for review!





With regards,
Pavel Skripkin

[PATCH v3] can: etas_es58x: fix error handling

[Pavel Skripkin]

When register_candev() fails there are 2 possible device states:
NETREG_UNINITIALIZED and NETREG_UNREGISTERED. None of them are suitable
for calling unregister_candev(), because of following checks in
unregister_netdevice_many():

	if (dev->reg_state == NETREG_UNINITIALIZED)
		WARN_ON(1);
...
	BUG_ON(dev->reg_state != NETREG_REGISTERED);

To avoid possible BUG_ON or WARN_ON let's free current netdev before
returning from es58x_init_netdev() and leave others (registered)
net devices for es58x_free_netdevs().

Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces")
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---

Changes in v3:
	- Moved back es58x_dev->netdev[channel_idx] initialization,
	  since it's unsafe to intialize it _after_ register_candev()
	  call. Thanks to Johan Hovold <johan@kernel.org> for spotting
	  it

Changes in v2:
	- Fixed Fixes: tag
	- Moved es58x_dev->netdev[channel_idx] initialization at the end
	  of the function

---
 drivers/net/can/usb/etas_es58x/es58x_core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index 96a13c770e4a..41c721f2fbbe 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -2098,8 +2098,11 @@ static int es58x_init_netdev(struct es58x_device *es58x_dev, int channel_idx)
 	netdev->flags |= IFF_ECHO;	/* We support local echo */
 
 	ret = register_candev(netdev);
-	if (ret)
+	if (ret) {
+		free_candev(netdev);
+		es58x_dev->netdev[channel_idx] = NULL;
 		return ret;
+	}
 
 	netdev_queue_set_dql_min_limit(netdev_get_tx_queue(netdev, 0),
 				       es58x_dev->param->dql_min_limit);
-- 
2.33.1

Re: [PATCH v3] can: etas_es58x: fix error handling

[Vincent MAILHOL]

On Mon. 15 Nov 2021 at 17:37, Pavel Skripkin <paskripkin@gmail.com> wrote:
> When register_candev() fails there are 2 possible device states:
> NETREG_UNINITIALIZED and NETREG_UNREGISTERED. None of them are suitable
> for calling unregister_candev(), because of following checks in
> unregister_netdevice_many():
>
>         if (dev->reg_state == NETREG_UNINITIALIZED)
>                 WARN_ON(1);
> ...
>         BUG_ON(dev->reg_state != NETREG_REGISTERED);
>
> To avoid possible BUG_ON or WARN_ON let's free current netdev before
> returning from es58x_init_netdev() and leave others (registered)
> net devices for es58x_free_netdevs().
>
> Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces")
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>

Acked-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

> ---
>
> Changes in v3:
>         - Moved back es58x_dev->netdev[channel_idx] initialization,
>           since it's unsafe to intialize it _after_ register_candev()
>           call. Thanks to Johan Hovold <johan@kernel.org> for spotting
>           it

My bad on that. I missed the fact that the netdev_ops becomes
active once register_candev() returns.

> Changes in v2:
>         - Fixed Fixes: tag
>         - Moved es58x_dev->netdev[channel_idx] initialization at the end
>           of the function
>
> ---
>  drivers/net/can/usb/etas_es58x/es58x_core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
> index 96a13c770e4a..41c721f2fbbe 100644
> --- a/drivers/net/can/usb/etas_es58x/es58x_core.c
> +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
> @@ -2098,8 +2098,11 @@ static int es58x_init_netdev(struct es58x_device *es58x_dev, int channel_idx)
>         netdev->flags |= IFF_ECHO;      /* We support local echo */
>
>         ret = register_candev(netdev);
> -       if (ret)
> +       if (ret) {
> +               free_candev(netdev);
> +               es58x_dev->netdev[channel_idx] = NULL;
>                 return ret;
> +       }
>
>         netdev_queue_set_dql_min_limit(netdev_get_tx_queue(netdev, 0),
>                                        es58x_dev->param->dql_min_limit);
> --
> 2.33.1
>

Re: [PATCH v2] can: etas_es58x: fix error handling

[Vincent MAILHOL]

On Mon. 15 Nov 2021 at 17:30, Pavel Skripkin <paskripkin@gmail.com> wrote:
> On 11/15/21 11:16, Johan Hovold wrote:
> > On Mon, Nov 15, 2021 at 11:15:07AM +0300, Pavel Skripkin wrote:
> >> On 11/15/21 11:11, Johan Hovold wrote:
> >> > Just a drive-by comment:
> >> >
> >> > Are you sure about this move of the netdev[channel_idx] initialisation?
> >> > What happens if the registered can device is opened before you
> >> > initialise the pointer? NULL-deref in es58x_send_msg()?
> >> >
> >> > You generally want the driver data fully initialised before you register
> >> > the device so this looks broken.
> >> >
> >> > And either way it is arguably an unrelated change that should go in a
> >> > separate patch explaining why it is needed and safe.
> >> >
> >>
> >>
> >> It was suggested by Vincent who is the maintainer of this driver [1].
> >
> > Yeah, I saw that, but that doesn't necessarily mean it is correct.
> >
> > You're still responsible for the changes you make and need to be able to
> > argue why they are correct.
> >
>
> Sure! I should have check it before sending v2 :( My bad, sorry. I see
> now, that there is possible calltrace which can hit NULL defer.

I should be the one apologizing here. Sorry for the confusion.

> One thing I am wondering about is why in some code parts there are
> validation checks for es58x_dev->netdev[i] and in others they are missing.

There is a validation when it is accessed in a for loop.
It is not guarded in es58x_send_msg() because this function
expects the channel_idx to be a valid index.

Does this answer your wonders?

Re: [PATCH v2] can: etas_es58x: fix error handling

[Pavel Skripkin]

On 11/15/21 12:24, Vincent MAILHOL wrote:
>> Sure! I should have check it before sending v2 :( My bad, sorry. I see
>> now, that there is possible calltrace which can hit NULL defer.
> 
> I should be the one apologizing here. Sorry for the confusion.
> 
>> One thing I am wondering about is why in some code parts there are
>> validation checks for es58x_dev->netdev[i] and in others they are missing.
> 
> There is a validation when it is accessed in a for loop.
> It is not guarded in es58x_send_msg() because this function
> expects the channel_idx to be a valid index.
> 
> Does this answer your wonders?
> 

Yeah! I have just looked at the code one more time and came up with the 
same idea.

Thank you for confirming and acking my patch :)



With regards,
Pavel Skripkin