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 <[email protected]>
---
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 <[email protected]> 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 <[email protected]>
> ---
> 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);
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 <[email protected]> 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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
---
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:
> 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 <[email protected]>
> ---
>
> 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
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:
> > 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:
>> 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
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 <[email protected]>
---
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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
Acked-by: Vincent Mailhol <[email protected]>
> ---
>
> 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 <[email protected]> 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
>
On Mon. 15 Nov 2021 at 17:30, Pavel Skripkin <[email protected]> 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?
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