Protect access to fore200e->available_cell_rate with rate_mtx lock to prevent potential data race.
In this case, since the update depends on a prior read, a data race could lead to a wrong fore200e.available_cell_rate value.
The field fore200e.available_cell_rate is generally protected by the lock fore200e.rate_mtx when accessed. In all other read and write cases, this field is consistently protected by the lock, except for this case and during initialization.
This potential bug was detected by our experimental static analysis tool, which analyzes locking APIs and paired functions to identify data races and atomicity violations.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Cc: stable@vger.kernel.org Signed-off-by: Gui-Dong Han 2045gemini@gmail.com --- v2: * Added a description of the data race hazard in fore200e_open(), as suggested by Jakub Kicinski and Simon Horman. --- drivers/atm/fore200e.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/atm/fore200e.c b/drivers/atm/fore200e.c index 4fea1149e003..f62e38571440 100644 --- a/drivers/atm/fore200e.c +++ b/drivers/atm/fore200e.c @@ -1374,7 +1374,9 @@ fore200e_open(struct atm_vcc *vcc)
vcc->dev_data = NULL;
+ mutex_lock(&fore200e->rate_mtx); fore200e->available_cell_rate += vcc->qos.txtp.max_pcr; + mutex_unlock(&fore200e->rate_mtx);
kfree(fore200e_vcc); return -EINVAL;
On Wed, Jan 22, 2025 at 02:37:45AM +0000, Gui-Dong Han wrote:
Protect access to fore200e->available_cell_rate with rate_mtx lock to prevent potential data race.
In this case, since the update depends on a prior read, a data race could lead to a wrong fore200e.available_cell_rate value.
The field fore200e.available_cell_rate is generally protected by the lock fore200e.rate_mtx when accessed. In all other read and write cases, this field is consistently protected by the lock, except for this case and during initialization.
This potential bug was detected by our experimental static analysis tool, which analyzes locking APIs and paired functions to identify data races and atomicity violations.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Cc: stable@vger.kernel.org Signed-off-by: Gui-Dong Han 2045gemini@gmail.com
v2:
- Added a description of the data race hazard in fore200e_open(), as
suggested by Jakub Kicinski and Simon Horman.
Reviewed-by: Simon Horman horms@kernel.org
On Wed, 22 Jan 2025 02:37:45 +0000 Gui-Dong Han wrote:
Protect access to fore200e->available_cell_rate with rate_mtx lock to prevent potential data race.
In this case, since the update depends on a prior read, a data race could lead to a wrong fore200e.available_cell_rate value.
The field fore200e.available_cell_rate is generally protected by the lock fore200e.rate_mtx when accessed. In all other read and write cases, this field is consistently protected by the lock, except for this case and during initialization.
Please describe the call paths which interact to cause the race.
On Thu, Jan 23, 2025 at 11:12 PM Jakub Kicinski kuba@kernel.org wrote:
On Wed, 22 Jan 2025 02:37:45 +0000 Gui-Dong Han wrote:
Protect access to fore200e->available_cell_rate with rate_mtx lock to prevent potential data race.
In this case, since the update depends on a prior read, a data race could lead to a wrong fore200e.available_cell_rate value.
The field fore200e.available_cell_rate is generally protected by the lock fore200e.rate_mtx when accessed. In all other read and write cases, this field is consistently protected by the lock, except for this case and during initialization.
Please describe the call paths which interact to cause the race.
The fore200e.available_cell_rate is a shared resource used to track the available bandwidth for allocation.
The functions fore200e_open(), fore200e_close(), and fore200e_change_qos(), which modify fore200e.available_cell_rate to reflect bandwidth availability, may interact and cause a race condition.
fore200e_open(struct atm_vcc *vcc) { ... /* Pseudo-CBR bandwidth requested? */ if ((vcc->qos.txtp.traffic_class == ATM_CBR) && (vcc->qos.txtp.max_pcr > 0)) {
mutex_lock(&fore200e->rate_mtx); if (fore200e->available_cell_rate < vcc->qos.txtp.max_pcr) { mutex_unlock(&fore200e->rate_mtx); ... // Error handling code return -EAGAIN; }
/* Reserve bandwidth */ fore200e->available_cell_rate -= vcc->qos.txtp.max_pcr; mutex_unlock(&fore200e->rate_mtx); } ... if (fore200e_activate_vcin(fore200e, 1, vcc, vcc->qos.rxtp.max_sdu) < 0) { ... // Error handling code fore200e->available_cell_rate += vcc->qos.txtp.max_pcr; ... // Error handling code return -EINVAL; } }
There is further evidence within fore200e_open() itself. The function correctly takes the lock when subtracting vcc->qos.txtp.max_pcr from fore200e.available_cell_rate to reserve bandwidth, but later, in the error handling path, it adds vcc->qos.txtp.max_pcr back without holding the lock. There is no justification for modifying a shared resource without the corresponding lock in this case.
Regards, Han
linux-stable-mirror@lists.linaro.org
-
Gui-Dong Han -
Jakub Kicinski -
Simon Horman