[PATCH] can: etas_es58x: fix error handling

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 <redacted>
---
 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

Hi Pavel,

Thanks for the patch!

On Mon. 15 Nov 2021 at 05:58, Pavel Skripkin [off-list ref] 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

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);

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 [off-list ref] wrote:

quoted

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

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





With regards,
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 <redacted>
---

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

On Mon, Nov 15, 2021 at 10:51:24AM +0300, Pavel Skripkin 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.

 	return ret;
 }

Johan

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

On Mon, Nov 15, 2021 at 11:15:07AM +0300, Pavel Skripkin wrote:

On 11/15/21 11:11, Johan Hovold wrote:

quoted

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

On 11/15/21 11:16, Johan Hovold wrote:

On Mon, Nov 15, 2021 at 11:15:07AM +0300, Pavel Skripkin wrote:

quoted

On 11/15/21 11:11, Johan Hovold wrote:

quoted

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

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 <redacted>
---

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 [off-list ref] 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

On Mon. 15 Nov 2021 at 17:37, Pavel Skripkin [off-list ref] 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 <redacted>

Acked-by: Vincent Mailhol <redacted>

---

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 [off-list ref] for spotting
          it

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

On Mon. 15 Nov 2021 at 17:30, Pavel Skripkin [off-list ref] wrote:

On 11/15/21 11:16, Johan Hovold wrote:

quoted

On Mon, Nov 15, 2021 at 11:15:07AM +0300, Pavel Skripkin wrote:

quoted

On 11/15/21 11:11, Johan Hovold wrote:

quoted

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?

On 11/15/21 12:24, Vincent MAILHOL wrote:

quoted

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.

quoted

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