| Message ID | 20211115075124.17713-1-paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] can: etas_es58x: fix error handling | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
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
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
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?
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
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; }
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(-)