]> git.proxmox.com Git - mirror_ubuntu-jammy-kernel.git/commit
can: etas_es58x: change opened_channel_cnt's type from atomic_t to u8
authorVincent Mailhol <mailhol.vincent@wanadoo.fr>
Sat, 12 Feb 2022 11:27:13 +0000 (20:27 +0900)
committerPaolo Pisati <paolo.pisati@canonical.com>
Wed, 9 Mar 2022 14:17:56 +0000 (15:17 +0100)
commitf99790d9bab195127830c40f8795e9dee81317fe
treea6825d88a7ec819ba6d4b2f0d02080f2a6428f32
parent7ca1affef4721837423553b2b507215ae761f074
can: etas_es58x: change opened_channel_cnt's type from atomic_t to u8

BugLink: https://bugs.launchpad.net/bugs/1964361
[ Upstream commit f4896248e9025ff744b4147e6758274a1cb8cbae ]

The driver uses an atomic_t variable: struct
es58x_device::opened_channel_cnt to keep track of the number of opened
channels in order to only allocate memory for the URBs when this count
changes from zero to one.

While the intent was to prevent race conditions, the choice of an
atomic_t turns out to be a bad idea for several reasons:

- implementation is incorrect and fails to decrement
  opened_channel_cnt when the URB allocation fails as reported in
  [1].

- even if opened_channel_cnt were to be correctly decremented,
  atomic_t is insufficient to cover edge cases: there can be a race
  condition in which 1/ a first process fails to allocate URBs
  memory 2/ a second process enters es58x_open() before the first
  process does its cleanup and decrements opened_channed_cnt. In
  which case, the second process would successfully return despite
  the URBs memory not being allocated.

- actually, any kind of locking mechanism was useless here because
  it is redundant with the network stack big kernel lock
  (a.k.a. rtnl_lock) which is being hold by all the callers of
  net_device_ops:ndo_open() and net_device_ops:ndo_close(). c.f. the
  ASSERST_RTNL() calls in __dev_open() [2] and __dev_close_many()
  [3].

The atmomic_t is thus replaced by a simple u8 type and the logic to
increment and decrement es58x_device:opened_channel_cnt is simplified
accordingly fixing the bug reported in [1]. We do not check again for
ASSERST_RTNL() as this is already done by the callers.

[1] https://lore.kernel.org/linux-can/20220201140351.GA2548@kili/T/#u
[2] https://elixir.bootlin.com/linux/v5.16/source/net/core/dev.c#L1463
[3] https://elixir.bootlin.com/linux/v5.16/source/net/core/dev.c#L1541

Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces")
Link: https://lore.kernel.org/all/20220212112713.577957-1-mailhol.vincent@wanadoo.fr
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Sasha Levin <sashal@kernel.org>
Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com>
drivers/net/can/usb/etas_es58x/es58x_core.c
drivers/net/can/usb/etas_es58x/es58x_core.h