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

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

Hi Jakub and Simon,

I was organizing my emails and noticed this v2 patch from January.
Simon kindly provided a "Reviewed-by" tag for it.

It seems this patch may have been overlooked and was not merged.
I checked the current upstream tree, and the data race in
fore200e_open() (accessing fore200e->available_cell_rate
without the rate_mtx lock in the error path) still exists.

Could you please take another look and consider this patch for merging?

Thank you,
Gui-Dong Han

On Tue, Nov 11, 2025 at 08:47:14PM +0800, Gui-Dong Han wrote:
> Hi Jakub and Simon,
> 
> I was organizing my emails and noticed this v2 patch from January.
> Simon kindly provided a "Reviewed-by" tag for it.
> 
> It seems this patch may have been overlooked and was not merged.
> I checked the current upstream tree, and the data race in
> fore200e_open() (accessing fore200e->available_cell_rate
> without the rate_mtx lock in the error path) still exists.
> 
> Could you please take another look and consider this patch for merging?

Hi,

January was a long time ago, so I guess this slipped through the cracks somehow.

I would suggest reposting it to have it considered for merging.
I'd also explicitly target the patch at net. Something like this:

Subject: [PATCH REPOST net v2] ...

And feel free to include my Reviewed-by tag.

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>