| Message ID | 20241218142045.77269-2-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add SYS and GIC clock entries for RZ/V2H(P) SoC | expand |
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
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
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
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; } }