[PATCH] atm/fore200e: Fix possible data race in fore200e_open()

[PATCH] atm/fore200e: Fix possible data race in fore200e_open()

[Gui-Dong Han]

Protect access to fore200e->available_cell_rate with rate_mtx lock to
prevent potential data race.

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

Re: [PATCH] atm/fore200e: Fix possible data race in fore200e_open()

[Jakub Kicinski]

On Wed, 15 Jan 2025 13:10:06 +0000 Gui-Dong Han wrote:
> Protect access to fore200e->available_cell_rate with rate_mtx lock to
> prevent potential data race.
> 
> 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.

That's not sufficient in terms of analysis.

You need to be able to articulate what can go wrong.
-- 
pw-bot: reject

Re: [PATCH] atm/fore200e: Fix possible data race in fore200e_open()

[Gui-Dong Han]

> On Wed, 15 Jan 2025 13:10:06 +0000 Gui-Dong Han wrote:
> > Protect access to fore200e->available_cell_rate with rate_mtx lock to
> > prevent potential data race.
> >
> > 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.
>
> That's not sufficient in terms of analysis.
>
> You need to be able to articulate what can go wrong.

fore200e->available_cell_rate += vcc->qos.txtp.max_pcr;
In this case, since the update depends on a prior read, a data race
could lead to a wrong fore200e.available_cell_rate value.

Regards,
Han

Re: [PATCH] atm/fore200e: Fix possible data race in fore200e_open()

[Simon Horman]

On Fri, Jan 17, 2025 at 10:28:59AM +0800, Gui-Dong Han wrote:
> > On Wed, 15 Jan 2025 13:10:06 +0000 Gui-Dong Han wrote:
> > > Protect access to fore200e->available_cell_rate with rate_mtx lock to
> > > prevent potential data race.
> > >
> > > 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.
> >
> > That's not sufficient in terms of analysis.
> >
> > You need to be able to articulate what can go wrong.
> 
> fore200e->available_cell_rate += vcc->qos.txtp.max_pcr;
> In this case, since the update depends on a prior read, a data race
> could lead to a wrong fore200e.available_cell_rate value.

Hi Gui-Dong Han,

I think it would be good to post a v2 of this patch with
an explanation along the lines of the above included in
the patch description.

Re: [PATCH] atm/fore200e: Fix possible data race in fore200e_open()

[Gui-Dong Han]

> On Fri, Jan 17, 2025 at 10:28:59AM +0800, Gui-Dong Han wrote:
> > > On Wed, 15 Jan 2025 13:10:06 +0000 Gui-Dong Han wrote:
> > > > Protect access to fore200e->available_cell_rate with rate_mtx lock to
> > > > prevent potential data race.
> > > >
> > > > 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.
> > >
> > > That's not sufficient in terms of analysis.
> > >
> > > You need to be able to articulate what can go wrong.
> >
> > fore200e->available_cell_rate += vcc->qos.txtp.max_pcr;
> > In this case, since the update depends on a prior read, a data race
> > could lead to a wrong fore200e.available_cell_rate value.
>
> Hi Gui-Dong Han,
>
> I think it would be good to post a v2 of this patch with
> an explanation along the lines of the above included in
> the patch description.

Hi Simon Horman,

Thank you for your feedback. I have submitted a v2 version of the
patch with an added description of the data race hazard, as suggested.