From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Avoid triggering a `refcount_t: addition on 0; use-after-free.` warning
when registering a module clock with the same MSTOP configuration. The
issue arises when a module clock is registered but not enabled, resulting
in a `ref_cnt` of 0. Subsequent calls to `refcount_inc()` on such clocks
cause the kernel to warn about use-after-free.
[ 0.113529] ------------[ cut here ]------------
[ 0.113537] refcount_t: addition on 0; use-after-free.
[ 0.113576] WARNING: CPU: 2 PID: 1 at lib/refcount.c:25 refcount_warn_saturate+0x120/0x144
[ 0.113602] Modules linked in:
[ 0.113616] CPU: 2 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.13.0-rc3+ #446
[ 0.113629] Hardware name: Renesas RZ/V2H EVK Board based on r9a09g057h44 (DT)
[ 0.113641] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 0.113652] pc : refcount_warn_saturate+0x120/0x144
[ 0.113664] lr : refcount_warn_saturate+0x120/0x144
[ 0.113675] sp : ffff8000818aba90
[ 0.113682] x29: ffff8000818aba90 x28: ffff0000c0d96450 x27: ffff0000c0d96440
[ 0.113699] x26: 0000000000000014 x25: 0000000000051000 x24: ffff0000c0ad6480
[ 0.113714] x23: ffff0000c0d96200 x22: ffff800080fae558 x21: 00000000000001e0
[ 0.113730] x20: ffff0000c0b11c10 x19: ffff8000815ae6f0 x18: 0000000000000006
[ 0.113745] x17: ffff800081765368 x16: 0000000000000000 x15: 0765076507720766
[ 0.113760] x14: ffff8000816a3ea0 x13: 0765076507720766 x12: 072d077207650774
[ 0.113776] x11: ffff8000816a3ea0 x10: 00000000000000ce x9 : ffff8000816fbea0
[ 0.113791] x8 : 0000000000017fe8 x7 : 00000000fffff000 x6 : ffff8000816fbea0
[ 0.113806] x5 : 80000000fffff000 x4 : 0000000000000000 x3 : 0000000000000000
[ 0.113821] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000c0158000
[ 0.113837] Call trace:
[ 0.113845] refcount_warn_saturate+0x120/0x144 (P)
[ 0.113859] rzv2h_cpg_probe+0x7f8/0xa38
[ 0.113874] platform_probe+0x68/0xdc
[ 0.113890] really_probe+0xbc/0x2c0
[ 0.113901] __driver_probe_device+0x78/0x120
[ 0.113912] driver_probe_device+0x3c/0x154
[ 0.113923] __driver_attach+0x90/0x1a0
[ 0.113933] bus_for_each_dev+0x7c/0xe0
[ 0.113944] driver_attach+0x24/0x30
[ 0.113954] bus_add_driver+0xe4/0x208
[ 0.113965] driver_register+0x68/0x124
[ 0.113975] __platform_driver_probe+0x54/0xd4
[ 0.113987] rzv2h_cpg_init+0x24/0x30
[ 0.113998] do_one_initcall+0x60/0x1d4
[ 0.114013] kernel_init_freeable+0x214/0x278
[ 0.114028] kernel_init+0x20/0x140
[ 0.114041] ret_from_fork+0x10/0x20
[ 0.114052] ---[ end trace 0000000000000000 ]---
Resolve this by checking the `ref_cnt` value before calling
`refcount_inc()`. If `ref_cnt` is 0, reset it to 1 using `refcount_set()`.
Fixes: 7bd4cb3d6b7c ("clk: renesas: rzv2h: Relocate MSTOP-related macros to the family driver")
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
drivers/clk/renesas/rzv2h-cpg.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c
index 668a2880b2d3..23c89b0de38a 100644
--- a/drivers/clk/renesas/rzv2h-cpg.c
+++ b/drivers/clk/renesas/rzv2h-cpg.c
@@ -565,8 +565,12 @@ static struct rzv2h_mstop
continue;
if (BUS_MSTOP(clk->mstop->idx, clk->mstop->mask) == mstop_data) {
- if (rzv2h_mod_clock_is_enabled(&clock->hw))
- refcount_inc(&clk->mstop->ref_cnt);
+ if (rzv2h_mod_clock_is_enabled(&clock->hw)) {
+ if (refcount_read(&clk->mstop->ref_cnt))
+ refcount_inc(&clk->mstop->ref_cnt);
+ else
+ refcount_set(&clk->mstop->ref_cnt, 1);
+ }
return clk->mstop;
}
}
--
2.43.0
Hi Prabhakar,
On Wed, Dec 18, 2024 at 3:20 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Avoid triggering a `refcount_t: addition on 0; use-after-free.` warning
> when registering a module clock with the same MSTOP configuration. The
> issue arises when a module clock is registered but not enabled, resulting
> in a `ref_cnt` of 0. Subsequent calls to `refcount_inc()` on such clocks
> cause the kernel to warn about use-after-free.
>
> [ 0.113529] ------------[ cut here ]------------
> [ 0.113537] refcount_t: addition on 0; use-after-free.
> [ 0.113576] WARNING: CPU: 2 PID: 1 at lib/refcount.c:25 refcount_warn_saturate+0x120/0x144
[...]
> Resolve this by checking the `ref_cnt` value before calling
> `refcount_inc()`. If `ref_cnt` is 0, reset it to 1 using `refcount_set()`.
Thanks for your patch!
> Fixes: 7bd4cb3d6b7c ("clk: renesas: rzv2h: Relocate MSTOP-related macros to the family driver")
The description (from your [PATCH 2/5]?) does not match the commit.
Fixes: 7bd4cb3d6b7c43f0 ("clk: renesas: rzv2h: Add MSTOP support")
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> --- a/drivers/clk/renesas/rzv2h-cpg.c
> +++ b/drivers/clk/renesas/rzv2h-cpg.c
> @@ -565,8 +565,12 @@ static struct rzv2h_mstop
> continue;
>
> if (BUS_MSTOP(clk->mstop->idx, clk->mstop->mask) == mstop_data) {
> - if (rzv2h_mod_clock_is_enabled(&clock->hw))
> - refcount_inc(&clk->mstop->ref_cnt);
> + if (rzv2h_mod_clock_is_enabled(&clock->hw)) {
> + if (refcount_read(&clk->mstop->ref_cnt))
> + refcount_inc(&clk->mstop->ref_cnt);
> + else
> + refcount_set(&clk->mstop->ref_cnt, 1);
> + }
> return clk->mstop;
> }
> }
This makes me wonder if refcount is the right abstraction?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert,
On Thu, Dec 19, 2024 at 4:20 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Wed, Dec 18, 2024 at 3:20 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Avoid triggering a `refcount_t: addition on 0; use-after-free.` warning
> > when registering a module clock with the same MSTOP configuration. The
> > issue arises when a module clock is registered but not enabled, resulting
> > in a `ref_cnt` of 0. Subsequent calls to `refcount_inc()` on such clocks
> > cause the kernel to warn about use-after-free.
> >
> > [ 0.113529] ------------[ cut here ]------------
> > [ 0.113537] refcount_t: addition on 0; use-after-free.
> > [ 0.113576] WARNING: CPU: 2 PID: 1 at lib/refcount.c:25 refcount_warn_saturate+0x120/0x144
>
> [...]
>
> > Resolve this by checking the `ref_cnt` value before calling
> > `refcount_inc()`. If `ref_cnt` is 0, reset it to 1 using `refcount_set()`.
>
> Thanks for your patch!
>
> > Fixes: 7bd4cb3d6b7c ("clk: renesas: rzv2h: Relocate MSTOP-related macros to the family driver")
>
> The description (from your [PATCH 2/5]?) does not match the commit.
>
Ouch!
> Fixes: 7bd4cb3d6b7c43f0 ("clk: renesas: rzv2h: Add MSTOP support")
>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> > @@ -565,8 +565,12 @@ static struct rzv2h_mstop
> > continue;
> >
> > if (BUS_MSTOP(clk->mstop->idx, clk->mstop->mask) == mstop_data) {
> > - if (rzv2h_mod_clock_is_enabled(&clock->hw))
> > - refcount_inc(&clk->mstop->ref_cnt);
> > + if (rzv2h_mod_clock_is_enabled(&clock->hw)) {
> > + if (refcount_read(&clk->mstop->ref_cnt))
> > + refcount_inc(&clk->mstop->ref_cnt);
> > + else
> > + refcount_set(&clk->mstop->ref_cnt, 1);
> > + }
> > return clk->mstop;
> > }
> > }
>
> This makes me wonder if refcount is the right abstraction?
>
You mean as discussed on irc, refcount per mstop bit instead of groups
is not OK too? Do you have any other better approach in mind?
Cheers,
Prabhakar
Hi Prabhakar,
On Fri, Dec 20, 2024 at 9:24 AM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Thu, Dec 19, 2024 at 4:20 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Dec 18, 2024 at 3:20 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Avoid triggering a `refcount_t: addition on 0; use-after-free.` warning
> > > when registering a module clock with the same MSTOP configuration. The
> > > issue arises when a module clock is registered but not enabled, resulting
> > > in a `ref_cnt` of 0. Subsequent calls to `refcount_inc()` on such clocks
> > > cause the kernel to warn about use-after-free.
> > >
> > > [ 0.113529] ------------[ cut here ]------------
> > > [ 0.113537] refcount_t: addition on 0; use-after-free.
> > > [ 0.113576] WARNING: CPU: 2 PID: 1 at lib/refcount.c:25 refcount_warn_saturate+0x120/0x144
> >
> > [...]
> >
> > > Resolve this by checking the `ref_cnt` value before calling
> > > `refcount_inc()`. If `ref_cnt` is 0, reset it to 1 using `refcount_set()`.
> >
> > Thanks for your patch!
> >
> > > Fixes: 7bd4cb3d6b7c ("clk: renesas: rzv2h: Relocate MSTOP-related macros to the family driver")
> >
> > The description (from your [PATCH 2/5]?) does not match the commit.
> >
> Ouch!
>
> > Fixes: 7bd4cb3d6b7c43f0 ("clk: renesas: rzv2h: Add MSTOP support")
> >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> > > @@ -565,8 +565,12 @@ static struct rzv2h_mstop
> > > continue;
> > >
> > > if (BUS_MSTOP(clk->mstop->idx, clk->mstop->mask) == mstop_data) {
> > > - if (rzv2h_mod_clock_is_enabled(&clock->hw))
> > > - refcount_inc(&clk->mstop->ref_cnt);
> > > + if (rzv2h_mod_clock_is_enabled(&clock->hw)) {
> > > + if (refcount_read(&clk->mstop->ref_cnt))
> > > + refcount_inc(&clk->mstop->ref_cnt);
> > > + else
> > > + refcount_set(&clk->mstop->ref_cnt, 1);
> > > + }
Or simply
do refcount_set(&clk->mstop->ref_cnt,
refcount_read(&clk->mstop->ref_cnt) +1);
?
Still, you risk some janitor replacing that by refcount_inc() regardless...
> > > return clk->mstop;
> > > }
> > > }
> >
> > This makes me wonder if refcount is the right abstraction?
> >
> You mean as discussed on irc, refcount per mstop bit instead of groups
> is not OK too? Do you have any other better approach in mind?
I mean if you need such silly workarounds to do a simple increment, is
refcount_t the right abstraction, instead of a plain atomic_t?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert,
On Fri, Dec 20, 2024 at 8:42 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Fri, Dec 20, 2024 at 9:24 AM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Thu, Dec 19, 2024 at 4:20 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Wed, Dec 18, 2024 at 3:20 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Avoid triggering a `refcount_t: addition on 0; use-after-free.` warning
> > > > when registering a module clock with the same MSTOP configuration. The
> > > > issue arises when a module clock is registered but not enabled, resulting
> > > > in a `ref_cnt` of 0. Subsequent calls to `refcount_inc()` on such clocks
> > > > cause the kernel to warn about use-after-free.
> > > >
> > > > [ 0.113529] ------------[ cut here ]------------
> > > > [ 0.113537] refcount_t: addition on 0; use-after-free.
> > > > [ 0.113576] WARNING: CPU: 2 PID: 1 at lib/refcount.c:25 refcount_warn_saturate+0x120/0x144
> > >
> > > [...]
> > >
> > > > Resolve this by checking the `ref_cnt` value before calling
> > > > `refcount_inc()`. If `ref_cnt` is 0, reset it to 1 using `refcount_set()`.
> > >
> > > Thanks for your patch!
> > >
> > > > Fixes: 7bd4cb3d6b7c ("clk: renesas: rzv2h: Relocate MSTOP-related macros to the family driver")
> > >
> > > The description (from your [PATCH 2/5]?) does not match the commit.
> > >
> > Ouch!
> >
> > > Fixes: 7bd4cb3d6b7c43f0 ("clk: renesas: rzv2h: Add MSTOP support")
> > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > > > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> > > > @@ -565,8 +565,12 @@ static struct rzv2h_mstop
> > > > continue;
> > > >
> > > > if (BUS_MSTOP(clk->mstop->idx, clk->mstop->mask) == mstop_data) {
> > > > - if (rzv2h_mod_clock_is_enabled(&clock->hw))
> > > > - refcount_inc(&clk->mstop->ref_cnt);
> > > > + if (rzv2h_mod_clock_is_enabled(&clock->hw)) {
> > > > + if (refcount_read(&clk->mstop->ref_cnt))
> > > > + refcount_inc(&clk->mstop->ref_cnt);
> > > > + else
> > > > + refcount_set(&clk->mstop->ref_cnt, 1);
> > > > + }
>
> Or simply
>
> do refcount_set(&clk->mstop->ref_cnt,
> refcount_read(&clk->mstop->ref_cnt) +1);
>
> ?
>
> Still, you risk some janitor replacing that by refcount_inc() regardless...
>
Agreed.
> > > > return clk->mstop;
> > > > }
> > > > }
> > >
> > > This makes me wonder if refcount is the right abstraction?
> > >
> > You mean as discussed on irc, refcount per mstop bit instead of groups
> > is not OK too? Do you have any other better approach in mind?
>
> I mean if you need such silly workarounds to do a simple increment, is
> refcount_t the right abstraction, instead of a plain atomic_t?
>
OK, I'll switch to the atomic_t variant. For this I will still rebase
my work on [0] along with atomic_t per mstop bit. Is that OK?
[0] https://lore.kernel.org/all/CAMuHMdUEkN6Z7p=LspP+npB3xs4ui+D9oGG+Q15kQ-mYiaoK-A@mail.gmail.com/
Cheers,
Prabhakar
Hi Prabhakar,
On Fri, Dec 20, 2024 at 10:37 AM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Fri, Dec 20, 2024 at 8:42 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Dec 20, 2024 at 9:24 AM Lad, Prabhakar
> > <prabhakar.csengg@gmail.com> wrote:
> > > On Thu, Dec 19, 2024 at 4:20 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Wed, Dec 18, 2024 at 3:20 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > >
> > > > > Avoid triggering a `refcount_t: addition on 0; use-after-free.` warning
> > > > > when registering a module clock with the same MSTOP configuration. The
> > > > > issue arises when a module clock is registered but not enabled, resulting
> > > > > in a `ref_cnt` of 0. Subsequent calls to `refcount_inc()` on such clocks
> > > > > cause the kernel to warn about use-after-free.
> > > > >
> > > > > [ 0.113529] ------------[ cut here ]------------
> > > > > [ 0.113537] refcount_t: addition on 0; use-after-free.
> > > > > [ 0.113576] WARNING: CPU: 2 PID: 1 at lib/refcount.c:25 refcount_warn_saturate+0x120/0x144
> > > >
> > > > [...]
> > > >
> > > > > Resolve this by checking the `ref_cnt` value before calling
> > > > > `refcount_inc()`. If `ref_cnt` is 0, reset it to 1 using `refcount_set()`.
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > Fixes: 7bd4cb3d6b7c ("clk: renesas: rzv2h: Relocate MSTOP-related macros to the family driver")
> > > >
> > > > The description (from your [PATCH 2/5]?) does not match the commit.
> > > >
> > > Ouch!
> > >
> > > > Fixes: 7bd4cb3d6b7c43f0 ("clk: renesas: rzv2h: Add MSTOP support")
> > > >
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > > > > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> > > > > @@ -565,8 +565,12 @@ static struct rzv2h_mstop
> > > > > continue;
> > > > >
> > > > > if (BUS_MSTOP(clk->mstop->idx, clk->mstop->mask) == mstop_data) {
> > > > > - if (rzv2h_mod_clock_is_enabled(&clock->hw))
> > > > > - refcount_inc(&clk->mstop->ref_cnt);
> > > > > + if (rzv2h_mod_clock_is_enabled(&clock->hw)) {
> > > > > + if (refcount_read(&clk->mstop->ref_cnt))
> > > > > + refcount_inc(&clk->mstop->ref_cnt);
> > > > > + else
> > > > > + refcount_set(&clk->mstop->ref_cnt, 1);
> > > > > + }
> >
> > Or simply
> >
> > do refcount_set(&clk->mstop->ref_cnt,
> > refcount_read(&clk->mstop->ref_cnt) +1);
> >
> > ?
> >
> > Still, you risk some janitor replacing that by refcount_inc() regardless...
> >
> Agreed.
>
> > > > > return clk->mstop;
> > > > > }
> > > > > }
> > > >
> > > > This makes me wonder if refcount is the right abstraction?
> > > >
> > > You mean as discussed on irc, refcount per mstop bit instead of groups
> > > is not OK too? Do you have any other better approach in mind?
> >
> > I mean if you need such silly workarounds to do a simple increment, is
> > refcount_t the right abstraction, instead of a plain atomic_t?
> >
> OK, I'll switch to the atomic_t variant. For this I will still rebase
> my work on [0] along with atomic_t per mstop bit. Is that OK?
>
> [0] https://lore.kernel.org/all/CAMuHMdUEkN6Z7p=LspP+npB3xs4ui+D9oGG+Q15kQ-mYiaoK-A@mail.gmail.com/
That's fine. Once all issues are sorted out, I can still squash the fix into
the original commit, to avoid regressions while bisecting.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
© 2016 - 2025 Red Hat, Inc.