openipmi-developer Mailing List for Open IPMI

On 6/8/26 10:58 PM, Corey Minyard wrote:
> On Mon, Jun 08, 2026 at 11:27:54AM +0800, Rui Qi wrote:
>> Hi Corey,
>>
>> I'm following up on this patch which was originally submitted on
>> March 25 and resubmitted as v2 on May 25. I haven't received any
>> feedback so far, so I wanted to bring it back to your attention.
>>
>> To recap, this is a one-line fix for handle_read_event_rsp() where
>> rcu_read_unlock() is incorrectly called instead of srcu_read_unlock()
>> on the error path, leaving the SRCU read-side lock held.
>>
>> This patch is specifically targeted at stable branches (v6.12 and
>> earlier) that still carry the original SRCU-based locking. In
>> mainline, commit 3be997d5a64a ("ipmi:msghandler: Remove srcu from
>> the ipmi user structure") has already restructured this function to
>> use a mutex, effectively eliminating the bug. However, that commit
>> is part of a larger SRCU removal series that is not suitable for
>> stable backport.
>>
>> Since the affected code no longer exists in mainline or your
>> for-next tree, this patch cannot follow the usual path of being
>> applied there first and then cherry-picked by stable. Could you
>> please review and provide an Acked-by so the stable team can pick
>> it up directly?
> 
> I can give an:
> 
> Acked-by: Corey Minyard <co...@mi...>
> 
> on this, as it is obviously correct.  However, it might be better to
> backport the changes removing SRCU.  Using SRCU here was a mistake to
> begin with.  But that might be too big a change.
> 
> -corey
> 

Hi Corey,

Thanks for the review and the Acked-by.

Regarding backporting the SRCU removal series: I agree that removing
SRCU entirely would be the cleaner long-term solution. However, as
you noted, that series involves significant refactoring across
multiple functions and would be a relatively large change for a
stable branch. The one-line fix is minimal and addresses the
immediate SRCU imbalance without risking regressions, which seems
more appropriate for a stable backport.

With your Acked-by, I'll ask the stable team to pick this up.

Thanks again,
Rui

>>
>> No changes since v2. The patch is reproduced below for convenience.
>>
>> From: Rui Qi <qir...@by...>
>> Subject: [PATCH v2] ipmi: Fix rcu_read_unlock to srcu_read_unlock in
>>  handle_read_event_rsp
>>
>> Fix a bug where rcu_read_unlock() was used instead of srcu_read_unlock()
>> in handle_read_event_rsp() when ipmi_alloc_recv_msg() fails.
>>
>> This mismatch leads to an SRCU read-side critical section imbalance: the
>> entry uses srcu_read_lock(&intf->users_srcu) but the error path
>> incorrectly calls rcu_read_unlock(), which is a no-op for SRCU and
>> leaves the SRCU lock held.
>>
>> The offending code was restructured in mainline by commit 3be997d5a64a
>> ("ipmi:msghandler: Remove srcu from the ipmi user structure"), which
>> replaced the SRCU locking with a mutex in this function, effectively
>> eliminating the mismatch. However, that commit is part of a larger
>> SRCU removal series that is not suitable for stable backport. This
>> minimal fix addresses the SRCU imbalance for 6.12 and earlier stable
>> branches that still carry the original locking scheme.
>>
>> Fixes: e86ee2d44b44 ("ipmi: Rework locking and shutdown for hot remove")
>> Cc: st...@vg...
>> Signed-off-by: Rui Qi <qir...@by...>
>>
>>  drivers/char/ipmi/ipmi_msghandler.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
>> index 188722ec0337..41ae4dac4eeb 100644
>> --- a/drivers/char/ipmi/ipmi_msghandler.c
>> +++ b/drivers/char/ipmi/ipmi_msghandler.c
>> @@ -4395,7 +4395,7 @@ static int handle_read_event_rsp(struct ipmi_smi *intf,
>>
>>  		recv_msg = ipmi_alloc_recv_msg(user);
>>  		if (IS_ERR(recv_msg)) {
>> -			rcu_read_unlock();
>> +			srcu_read_unlock(&intf->users_srcu, index);
>>  			list_for_each_entry_safe(recv_msg, recv_msg2, &msgs,
>>  						 link) {
>>  				list_del(&recv_msg->link);

On Mon, Jun 08, 2026 at 11:27:54AM +0800, Rui Qi wrote:
> Hi Corey,
> 
> I'm following up on this patch which was originally submitted on
> March 25 and resubmitted as v2 on May 25. I haven't received any
> feedback so far, so I wanted to bring it back to your attention.
> 
> To recap, this is a one-line fix for handle_read_event_rsp() where
> rcu_read_unlock() is incorrectly called instead of srcu_read_unlock()
> on the error path, leaving the SRCU read-side lock held.
> 
> This patch is specifically targeted at stable branches (v6.12 and
> earlier) that still carry the original SRCU-based locking. In
> mainline, commit 3be997d5a64a ("ipmi:msghandler: Remove srcu from
> the ipmi user structure") has already restructured this function to
> use a mutex, effectively eliminating the bug. However, that commit
> is part of a larger SRCU removal series that is not suitable for
> stable backport.
> 
> Since the affected code no longer exists in mainline or your
> for-next tree, this patch cannot follow the usual path of being
> applied there first and then cherry-picked by stable. Could you
> please review and provide an Acked-by so the stable team can pick
> it up directly?

I can give an:

Acked-by: Corey Minyard <co...@mi...>

on this, as it is obviously correct.  However, it might be better to
backport the changes removing SRCU.  Using SRCU here was a mistake to
begin with.  But that might be too big a change.

-corey

> 
> No changes since v2. The patch is reproduced below for convenience.
> 
> From: Rui Qi <qir...@by...>
> Subject: [PATCH v2] ipmi: Fix rcu_read_unlock to srcu_read_unlock in
>  handle_read_event_rsp
> 
> Fix a bug where rcu_read_unlock() was used instead of srcu_read_unlock()
> in handle_read_event_rsp() when ipmi_alloc_recv_msg() fails.
> 
> This mismatch leads to an SRCU read-side critical section imbalance: the
> entry uses srcu_read_lock(&intf->users_srcu) but the error path
> incorrectly calls rcu_read_unlock(), which is a no-op for SRCU and
> leaves the SRCU lock held.
> 
> The offending code was restructured in mainline by commit 3be997d5a64a
> ("ipmi:msghandler: Remove srcu from the ipmi user structure"), which
> replaced the SRCU locking with a mutex in this function, effectively
> eliminating the mismatch. However, that commit is part of a larger
> SRCU removal series that is not suitable for stable backport. This
> minimal fix addresses the SRCU imbalance for 6.12 and earlier stable
> branches that still carry the original locking scheme.
> 
> Fixes: e86ee2d44b44 ("ipmi: Rework locking and shutdown for hot remove")
> Cc: st...@vg...
> Signed-off-by: Rui Qi <qir...@by...>
> 
>  drivers/char/ipmi/ipmi_msghandler.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index 188722ec0337..41ae4dac4eeb 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -4395,7 +4395,7 @@ static int handle_read_event_rsp(struct ipmi_smi *intf,
> 
>  		recv_msg = ipmi_alloc_recv_msg(user);
>  		if (IS_ERR(recv_msg)) {
> -			rcu_read_unlock();
> +			srcu_read_unlock(&intf->users_srcu, index);
>  			list_for_each_entry_safe(recv_msg, recv_msg2, &msgs,
>  						 link) {
>  				list_del(&recv_msg->link);

Hi Corey,

I'm following up on this patch which was originally submitted on
March 25 and resubmitted as v2 on May 25. I haven't received any
feedback so far, so I wanted to bring it back to your attention.

To recap, this is a one-line fix for handle_read_event_rsp() where
rcu_read_unlock() is incorrectly called instead of srcu_read_unlock()
on the error path, leaving the SRCU read-side lock held.

This patch is specifically targeted at stable branches (v6.12 and
earlier) that still carry the original SRCU-based locking. In
mainline, commit 3be997d5a64a ("ipmi:msghandler: Remove srcu from
the ipmi user structure") has already restructured this function to
use a mutex, effectively eliminating the bug. However, that commit
is part of a larger SRCU removal series that is not suitable for
stable backport.

Since the affected code no longer exists in mainline or your
for-next tree, this patch cannot follow the usual path of being
applied there first and then cherry-picked by stable. Could you
please review and provide an Acked-by so the stable team can pick
it up directly?

No changes since v2. The patch is reproduced below for convenience.

From: Rui Qi <qir...@by...>
Subject: [PATCH v2] ipmi: Fix rcu_read_unlock to srcu_read_unlock in
 handle_read_event_rsp

Fix a bug where rcu_read_unlock() was used instead of srcu_read_unlock()
in handle_read_event_rsp() when ipmi_alloc_recv_msg() fails.

This mismatch leads to an SRCU read-side critical section imbalance: the
entry uses srcu_read_lock(&intf->users_srcu) but the error path
incorrectly calls rcu_read_unlock(), which is a no-op for SRCU and
leaves the SRCU lock held.

The offending code was restructured in mainline by commit 3be997d5a64a
("ipmi:msghandler: Remove srcu from the ipmi user structure"), which
replaced the SRCU locking with a mutex in this function, effectively
eliminating the mismatch. However, that commit is part of a larger
SRCU removal series that is not suitable for stable backport. This
minimal fix addresses the SRCU imbalance for 6.12 and earlier stable
branches that still carry the original locking scheme.

Fixes: e86ee2d44b44 ("ipmi: Rework locking and shutdown for hot remove")
Cc: st...@vg...
Signed-off-by: Rui Qi <qir...@by...>

 drivers/char/ipmi/ipmi_msghandler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 188722ec0337..41ae4dac4eeb 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -4395,7 +4395,7 @@ static int handle_read_event_rsp(struct ipmi_smi *intf,

 		recv_msg = ipmi_alloc_recv_msg(user);
 		if (IS_ERR(recv_msg)) {
-			rcu_read_unlock();
+			srcu_read_unlock(&intf->users_srcu, index);
 			list_for_each_entry_safe(recv_msg, recv_msg2, &msgs,
 						 link) {
 				list_del(&recv_msg->link);

On Wed, Jun 03, 2026 at 04:18:57PM -0700, Rosen Penev wrote:
> On Wed, Jun 3, 2026 at 4:17 PM Corey Minyard <co...@mi...> wrote:
> >
> > On Wed, Jun 03, 2026 at 04:05:54PM -0700, Rosen Penev wrote:
> > > On Wed, Jun 3, 2026 at 3:57 PM Corey Minyard <co...@mi...> wrote:
> > > >
> > > > On Wed, Jun 03, 2026 at 05:53:56PM -0500, Corey Minyard wrote:
> > > > > On Wed, Jun 03, 2026 at 12:25:11PM -0700, Rosen Penev wrote:
> > > > > > Use platform_get_irq() to retrieve the interrupt resource instead of
> > > > > > directly parsing and mapping the OF node via irq_of_parse_and_map().
> > > > > > This is the standard pattern for platform devices.
> > > > > > irq_of_parse_and_map() requires ire_dispose_mapping(), which is missing.
> > > > > >
> > > > > > Assisted-by: Antigravity:Gemini-3.5-Flash
> > > > > > Signed-off-by: Rosen Penev <ro...@gm...>
> > > > > > ---
> > > > > >  drivers/char/ipmi/ipmi_si_platform.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c
> > > > > > index fb6e359ae494..e10b5d8af092 100644
> > > > > > --- a/drivers/char/ipmi/ipmi_si_platform.c
> > > > > > +++ b/drivers/char/ipmi/ipmi_si_platform.c
> > > > > > @@ -276,7 +276,7 @@ static int of_ipmi_probe(struct platform_device *pdev)
> > > > > >     io.regspacing   = regspacing ? be32_to_cpup(regspacing) : DEFAULT_REGSPACING;
> > > > > >     io.regshift     = regshift ? be32_to_cpup(regshift) : 0;
> > > > > >
> > > > > > -   io.irq          = irq_of_parse_and_map(pdev->dev.of_node, 0);
> > > > > > +   io.irq          = platform_get_irq(pdev, 0);
> > > > >
> > > > > This should be something like:
> > > > >
> > > > >       io.irq = platform_get_irq_optional(pdev, 0);
> > > > >       if (io.irq > 0)
> > > > >               io.irq_setup = ipmi_std_irq_setup;
> > > > >       else
> > > > >               io.irq = 0;
> > > > >
> > > > > right?
> > > >
> > > > Oops, cut and paste error, try:
> > > >
> > > >         io.irq = platform_get_irq_optional(pdev, 0);
> > > >         if (io.irq < 0)
> > > >                 io.irq = 0;
> > > I don't think that if is appropriate.
> >
> > Can platform_get_irq[_optional]() return < 0?  The driver still has to
> > work even if there is no interrupt specified.
> It can return -EPROBE_DEFER.
> 
> No idea how likely in this driver.

Well, I have to handle a negative return value here, the comments above
platform_get_irq_optional() say so.  The value is optional.  So it has
to be the above.

I'll modify the patch to be what I posted above.

> >
> > -corey
> >
> > > >
> > > > This just disables the interrupt if it can't get it.
> > > >
> > > > >
> > > > > -corey
> > > > >
> > > > > >     io.dev          = &pdev->dev;
> > > > > >
> > > > > >     dev_dbg(&pdev->dev, "addr 0x%lx regsize %d spacing %d irq %d\n",
> > > > > > --
> > > > > > 2.54.0
> > > > > >

On Wed, Jun 3, 2026 at 4:17 PM Corey Minyard <co...@mi...> wrote:
>
> On Wed, Jun 03, 2026 at 04:05:54PM -0700, Rosen Penev wrote:
> > On Wed, Jun 3, 2026 at 3:57 PM Corey Minyard <co...@mi...> wrote:
> > >
> > > On Wed, Jun 03, 2026 at 05:53:56PM -0500, Corey Minyard wrote:
> > > > On Wed, Jun 03, 2026 at 12:25:11PM -0700, Rosen Penev wrote:
> > > > > Use platform_get_irq() to retrieve the interrupt resource instead of
> > > > > directly parsing and mapping the OF node via irq_of_parse_and_map().
> > > > > This is the standard pattern for platform devices.
> > > > > irq_of_parse_and_map() requires ire_dispose_mapping(), which is missing.
> > > > >
> > > > > Assisted-by: Antigravity:Gemini-3.5-Flash
> > > > > Signed-off-by: Rosen Penev <ro...@gm...>
> > > > > ---
> > > > >  drivers/char/ipmi/ipmi_si_platform.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c
> > > > > index fb6e359ae494..e10b5d8af092 100644
> > > > > --- a/drivers/char/ipmi/ipmi_si_platform.c
> > > > > +++ b/drivers/char/ipmi/ipmi_si_platform.c
> > > > > @@ -276,7 +276,7 @@ static int of_ipmi_probe(struct platform_device *pdev)
> > > > >     io.regspacing   = regspacing ? be32_to_cpup(regspacing) : DEFAULT_REGSPACING;
> > > > >     io.regshift     = regshift ? be32_to_cpup(regshift) : 0;
> > > > >
> > > > > -   io.irq          = irq_of_parse_and_map(pdev->dev.of_node, 0);
> > > > > +   io.irq          = platform_get_irq(pdev, 0);
> > > >
> > > > This should be something like:
> > > >
> > > >       io.irq = platform_get_irq_optional(pdev, 0);
> > > >       if (io.irq > 0)
> > > >               io.irq_setup = ipmi_std_irq_setup;
> > > >       else
> > > >               io.irq = 0;
> > > >
> > > > right?
> > >
> > > Oops, cut and paste error, try:
> > >
> > >         io.irq = platform_get_irq_optional(pdev, 0);
> > >         if (io.irq < 0)
> > >                 io.irq = 0;
> > I don't think that if is appropriate.
>
> Can platform_get_irq[_optional]() return < 0?  The driver still has to
> work even if there is no interrupt specified.
It can return -EPROBE_DEFER.

No idea how likely in this driver.
>
> -corey
>
> > >
> > > This just disables the interrupt if it can't get it.
> > >
> > > >
> > > > -corey
> > > >
> > > > >     io.dev          = &pdev->dev;
> > > > >
> > > > >     dev_dbg(&pdev->dev, "addr 0x%lx regsize %d spacing %d irq %d\n",
> > > > > --
> > > > > 2.54.0
> > > > >

On Wed, Jun 03, 2026 at 04:05:54PM -0700, Rosen Penev wrote:
> On Wed, Jun 3, 2026 at 3:57 PM Corey Minyard <co...@mi...> wrote:
> >
> > On Wed, Jun 03, 2026 at 05:53:56PM -0500, Corey Minyard wrote:
> > > On Wed, Jun 03, 2026 at 12:25:11PM -0700, Rosen Penev wrote:
> > > > Use platform_get_irq() to retrieve the interrupt resource instead of
> > > > directly parsing and mapping the OF node via irq_of_parse_and_map().
> > > > This is the standard pattern for platform devices.
> > > > irq_of_parse_and_map() requires ire_dispose_mapping(), which is missing.
> > > >
> > > > Assisted-by: Antigravity:Gemini-3.5-Flash
> > > > Signed-off-by: Rosen Penev <ro...@gm...>
> > > > ---
> > > >  drivers/char/ipmi/ipmi_si_platform.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c
> > > > index fb6e359ae494..e10b5d8af092 100644
> > > > --- a/drivers/char/ipmi/ipmi_si_platform.c
> > > > +++ b/drivers/char/ipmi/ipmi_si_platform.c
> > > > @@ -276,7 +276,7 @@ static int of_ipmi_probe(struct platform_device *pdev)
> > > >     io.regspacing   = regspacing ? be32_to_cpup(regspacing) : DEFAULT_REGSPACING;
> > > >     io.regshift     = regshift ? be32_to_cpup(regshift) : 0;
> > > >
> > > > -   io.irq          = irq_of_parse_and_map(pdev->dev.of_node, 0);
> > > > +   io.irq          = platform_get_irq(pdev, 0);
> > >
> > > This should be something like:
> > >
> > >       io.irq = platform_get_irq_optional(pdev, 0);
> > >       if (io.irq > 0)
> > >               io.irq_setup = ipmi_std_irq_setup;
> > >       else
> > >               io.irq = 0;
> > >
> > > right?
> >
> > Oops, cut and paste error, try:
> >
> >         io.irq = platform_get_irq_optional(pdev, 0);
> >         if (io.irq < 0)
> >                 io.irq = 0;
> I don't think that if is appropriate.

Can platform_get_irq[_optional]() return < 0?  The driver still has to
work even if there is no interrupt specified.

-corey

> >
> > This just disables the interrupt if it can't get it.
> >
> > >
> > > -corey
> > >
> > > >     io.dev          = &pdev->dev;
> > > >
> > > >     dev_dbg(&pdev->dev, "addr 0x%lx regsize %d spacing %d irq %d\n",
> > > > --
> > > > 2.54.0
> > > >

On Wed, Jun 3, 2026 at 3:57 PM Corey Minyard <co...@mi...> wrote:
>
> On Wed, Jun 03, 2026 at 05:53:56PM -0500, Corey Minyard wrote:
> > On Wed, Jun 03, 2026 at 12:25:11PM -0700, Rosen Penev wrote:
> > > Use platform_get_irq() to retrieve the interrupt resource instead of
> > > directly parsing and mapping the OF node via irq_of_parse_and_map().
> > > This is the standard pattern for platform devices.
> > > irq_of_parse_and_map() requires ire_dispose_mapping(), which is missing.
> > >
> > > Assisted-by: Antigravity:Gemini-3.5-Flash
> > > Signed-off-by: Rosen Penev <ro...@gm...>
> > > ---
> > >  drivers/char/ipmi/ipmi_si_platform.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c
> > > index fb6e359ae494..e10b5d8af092 100644
> > > --- a/drivers/char/ipmi/ipmi_si_platform.c
> > > +++ b/drivers/char/ipmi/ipmi_si_platform.c
> > > @@ -276,7 +276,7 @@ static int of_ipmi_probe(struct platform_device *pdev)
> > >     io.regspacing   = regspacing ? be32_to_cpup(regspacing) : DEFAULT_REGSPACING;
> > >     io.regshift     = regshift ? be32_to_cpup(regshift) : 0;
> > >
> > > -   io.irq          = irq_of_parse_and_map(pdev->dev.of_node, 0);
> > > +   io.irq          = platform_get_irq(pdev, 0);
> >
> > This should be something like:
> >
> >       io.irq = platform_get_irq_optional(pdev, 0);
> >       if (io.irq > 0)
> >               io.irq_setup = ipmi_std_irq_setup;
> >       else
> >               io.irq = 0;
> >
> > right?
>
> Oops, cut and paste error, try:
>
>         io.irq = platform_get_irq_optional(pdev, 0);
>         if (io.irq < 0)
>                 io.irq = 0;
I don't think that if is appropriate.
>
> This just disables the interrupt if it can't get it.
>
> >
> > -corey
> >
> > >     io.dev          = &pdev->dev;
> > >
> > >     dev_dbg(&pdev->dev, "addr 0x%lx regsize %d spacing %d irq %d\n",
> > > --
> > > 2.54.0
> > >

On Wed, Jun 03, 2026 at 12:25:11PM -0700, Rosen Penev wrote:
> Use platform_get_irq() to retrieve the interrupt resource instead of
> directly parsing and mapping the OF node via irq_of_parse_and_map().
> This is the standard pattern for platform devices.
> irq_of_parse_and_map() requires ire_dispose_mapping(), which is missing.
> 
> Assisted-by: Antigravity:Gemini-3.5-Flash
> Signed-off-by: Rosen Penev <ro...@gm...>
> ---
>  drivers/char/ipmi/ipmi_si_platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c
> index fb6e359ae494..e10b5d8af092 100644
> --- a/drivers/char/ipmi/ipmi_si_platform.c
> +++ b/drivers/char/ipmi/ipmi_si_platform.c
> @@ -276,7 +276,7 @@ static int of_ipmi_probe(struct platform_device *pdev)
>  	io.regspacing	= regspacing ? be32_to_cpup(regspacing) : DEFAULT_REGSPACING;
>  	io.regshift	= regshift ? be32_to_cpup(regshift) : 0;
>  
> -	io.irq		= irq_of_parse_and_map(pdev->dev.of_node, 0);
> +	io.irq		= platform_get_irq(pdev, 0);

This should be something like:

	io.irq = platform_get_irq_optional(pdev, 0);
	if (io.irq > 0)
		io.irq_setup = ipmi_std_irq_setup;
	else
		io.irq = 0;

right?

-corey

>  	io.dev		= &pdev->dev;
>  
>  	dev_dbg(&pdev->dev, "addr 0x%lx regsize %d spacing %d irq %d\n",
> -- 
> 2.54.0
> 

On Wed, Jun 03, 2026 at 05:53:56PM -0500, Corey Minyard wrote:
> On Wed, Jun 03, 2026 at 12:25:11PM -0700, Rosen Penev wrote:
> > Use platform_get_irq() to retrieve the interrupt resource instead of
> > directly parsing and mapping the OF node via irq_of_parse_and_map().
> > This is the standard pattern for platform devices.
> > irq_of_parse_and_map() requires ire_dispose_mapping(), which is missing.
> > 
> > Assisted-by: Antigravity:Gemini-3.5-Flash
> > Signed-off-by: Rosen Penev <ro...@gm...>
> > ---
> >  drivers/char/ipmi/ipmi_si_platform.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c
> > index fb6e359ae494..e10b5d8af092 100644
> > --- a/drivers/char/ipmi/ipmi_si_platform.c
> > +++ b/drivers/char/ipmi/ipmi_si_platform.c
> > @@ -276,7 +276,7 @@ static int of_ipmi_probe(struct platform_device *pdev)
> >  	io.regspacing	= regspacing ? be32_to_cpup(regspacing) : DEFAULT_REGSPACING;
> >  	io.regshift	= regshift ? be32_to_cpup(regshift) : 0;
> >  
> > -	io.irq		= irq_of_parse_and_map(pdev->dev.of_node, 0);
> > +	io.irq		= platform_get_irq(pdev, 0);
> 
> This should be something like:
> 
> 	io.irq = platform_get_irq_optional(pdev, 0);
> 	if (io.irq > 0)
> 		io.irq_setup = ipmi_std_irq_setup;
> 	else
> 		io.irq = 0;
> 
> right?

Oops, cut and paste error, try:

 	io.irq = platform_get_irq_optional(pdev, 0);
 	if (io.irq < 0)
 		io.irq = 0;

This just disables the interrupt if it can't get it.

> 
> -corey
> 
> >  	io.dev		= &pdev->dev;
> >  
> >  	dev_dbg(&pdev->dev, "addr 0x%lx regsize %d spacing %d irq %d\n",
> > -- 
> > 2.54.0
> > 

Use platform_get_irq() to retrieve the interrupt resource instead of
directly parsing and mapping the OF node via irq_of_parse_and_map().
This is the standard pattern for platform devices.
irq_of_parse_and_map() requires ire_dispose_mapping(), which is missing.

Assisted-by: Antigravity:Gemini-3.5-Flash
Signed-off-by: Rosen Penev <ro...@gm...>
---
 drivers/char/ipmi/ipmi_si_platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c
index fb6e359ae494..e10b5d8af092 100644
--- a/drivers/char/ipmi/ipmi_si_platform.c
+++ b/drivers/char/ipmi/ipmi_si_platform.c
@@ -276,7 +276,7 @@ static int of_ipmi_probe(struct platform_device *pdev)
 	io.regspacing	= regspacing ? be32_to_cpup(regspacing) : DEFAULT_REGSPACING;
 	io.regshift	= regshift ? be32_to_cpup(regshift) : 0;
 
-	io.irq		= irq_of_parse_and_map(pdev->dev.of_node, 0);
+	io.irq		= platform_get_irq(pdev, 0);
 	io.dev		= &pdev->dev;
 
 	dev_dbg(&pdev->dev, "addr 0x%lx regsize %d spacing %d irq %d\n",
-- 
2.54.0

When a caller provides a `supplied_recv` message to i_ipmi_request(),
the function increments the user's `nr_msgs` reference count. If an
error occurs later, the out_err cleanup path only frees the recv_msg
if the function allocated it itself (i.e., !supplied_recv). In the
supplied_recv case the cleanup is skipped, leaving the reference count
elevated. The caller ipmi_request_supply_msgs() does not release the
supplied_recv on error, so the reference is permanently leaked.

Fix this by explicitly reverting the reference count operations when a
supplied recv_msg with a valid user pointer is present in the error
path: decrement nr_msgs and drop the user's kref.

Cc: st...@vg...
Fixes: b52da4054ee0 ("ipmi: Rework user message limit handling")
Signed-off-by: Wentao Liang <vu...@is...>
---
 drivers/char/ipmi/ipmi_msghandler.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 869ac87a4b6a..5b9d914cc7a9 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -2347,6 +2347,10 @@ static int i_ipmi_request(struct ipmi_user     *user,
 		if (smi_msg == NULL) {
 			if (!supplied_recv)
 				ipmi_free_recv_msg(recv_msg);
+			else if (recv_msg->user) {
+				atomic_dec(&recv_msg->user->nr_msgs);
+				kref_put(&recv_msg->user->refcount, free_ipmi_user);
+			}
 			return -ENOMEM;
 		}
 	}
@@ -2420,6 +2424,10 @@ static int i_ipmi_request(struct ipmi_user     *user,
 			ipmi_free_smi_msg(smi_msg);
 		if (!supplied_recv)
 			ipmi_free_recv_msg(recv_msg);
+		else if (recv_msg->user) {
+			atomic_dec(&recv_msg->user->nr_msgs);
+			kref_put(&recv_msg->user->refcount, free_ipmi_user);
+		}
 	}
 	return rv;
 }
-- 
2.34.1

On Wed, Jun 03, 2026 at 12:06:34PM +0000, Wentao Liang wrote:
> When a caller provides a `supplied_recv` message to i_ipmi_request(),
> the function increments the user's `nr_msgs` reference count. If an
> error occurs later, the out_err cleanup path only frees the recv_msg
> if the function allocated it itself (i.e., !supplied_recv). In the
> supplied_recv case the cleanup is skipped, leaving the reference count
> elevated. The caller ipmi_request_supply_msgs() does not release the
> supplied_recv on error, so the reference is permanently leaked.
> 
> Fix this by explicitly reverting the reference count operations when a
> supplied recv_msg with a valid user pointer is present in the error
> path: decrement nr_msgs and drop the user's kref.

This looks correct, it's in my next queue.

Thanks,

-corey

> 
> Cc: st...@vg...
> Fixes: b52da4054ee0 ("ipmi: Rework user message limit handling")
> Signed-off-by: Wentao Liang <vu...@is...>
> ---
>  drivers/char/ipmi/ipmi_msghandler.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index 869ac87a4b6a..5b9d914cc7a9 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -2347,6 +2347,10 @@ static int i_ipmi_request(struct ipmi_user     *user,
>  		if (smi_msg == NULL) {
>  			if (!supplied_recv)
>  				ipmi_free_recv_msg(recv_msg);
> +			else if (recv_msg->user) {
> +				atomic_dec(&recv_msg->user->nr_msgs);
> +				kref_put(&recv_msg->user->refcount, free_ipmi_user);
> +			}
>  			return -ENOMEM;
>  		}
>  	}
> @@ -2420,6 +2424,10 @@ static int i_ipmi_request(struct ipmi_user     *user,
>  			ipmi_free_smi_msg(smi_msg);
>  		if (!supplied_recv)
>  			ipmi_free_recv_msg(recv_msg);
> +		else if (recv_msg->user) {
> +			atomic_dec(&recv_msg->user->nr_msgs);
> +			kref_put(&recv_msg->user->refcount, free_ipmi_user);
> +		}
>  	}
>  	return rv;
>  }
> -- 
> 2.34.1
> 

On Tue, Jun 02, 2026 at 02:26:46PM +0300, Andy Shevchenko wrote:
> On Thu, May 21, 2026 at 06:33:14AM -0700, Kees Cook wrote:
> > 
> > param_array_get() appends each element's string representation into the
> > shared sysfs page buffer by passing buffer + off to the element getter.
> > 
> > That works for getters that only write a small bounded string, but
> > param_get_charp() and similar helpers format against PAGE_SIZE from the
> > pointer they receive. Once off is non-zero, an element getter can
> > therefore write past the end of the original sysfs page buffer.
> > 
> > Collect each element into a temporary PAGE_SIZE buffer first and then
> > copy only the remaining space into the caller's page buffer.
> 
> ...
> 
> > +	elem_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> 
> get_free_page() (or how it is called)?

I thought modern mm guidance was to use kmalloc whenever possible and
not use get_free_page() unless you intend to use the struct page bits?

Jason

On Tue, 2 Jun 2026 14:26:46 +0300
Andy Shevchenko <and...@li...> wrote:

> On Thu, May 21, 2026 at 06:33:14AM -0700, Kees Cook wrote:
> > 
> > param_array_get() appends each element's string representation into the
> > shared sysfs page buffer by passing buffer + off to the element getter.
> > 
> > That works for getters that only write a small bounded string, but
> > param_get_charp() and similar helpers format against PAGE_SIZE from the
> > pointer they receive. Once off is non-zero, an element getter can
> > therefore write past the end of the original sysfs page buffer.
> > 
> > Collect each element into a temporary PAGE_SIZE buffer first and then
> > copy only the remaining space into the caller's page buffer.  
> 
> ...
> 
> > +	elem_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);  
> 
> get_free_page() (or how it is called)?

The kmalloc() should be faster and I think has to be aligned.
There is another patch set to replace get_free_pages() with kmalloc().

Although all these 'show' functions should really head to using a safer
interface.
Although, at the moment, it is really difficult to find the ones that
are guaranteed to be passed a page aligned buffer.

-- David

> 
> > +	if (!elem_buf)
> > +		return -ENOMEM;
> > +
> >  	for (i = off = 0; i < (arr->num ? *arr->num : arr->max); i++) {
> > -		/* Replace \n with comma */
> > -		if (i)
> > -			buffer[off - 1] = ',';
> >  		p.arg = arr->elem + arr->elemsize * i;
> >  		check_kparam_locked(p.mod);
> > -		ret = arr->ops->get(buffer + off, &p);
> > +		ret = arr->ops->get(elem_buf, &p);
> >  		if (ret < 0)
> > -			return ret;
> > +			goto out;
> > +		ret = min(ret, (int)(PAGE_SIZE - 1 - off));  
> 
> It's usually discouraged to use castings in min/max/clamp. Can we make ret long
> or do something different here?
> 
> > +		if (!ret)
> > +			break;  
> 
> > +		/* Replace the previous element's trailing newline with a comma. */
> > +		if (i)
> > +			buffer[off - 1] = ',';  
> 
> Can't we do this after with help of strreplace()?
> 
> > +		memcpy(buffer + off, elem_buf, ret);
> >  		off += ret;
> > +		if (off == PAGE_SIZE - 1)
> > +			break;
> >  	}
> >  	buffer[off] = '\0';
> > -	return off;
> > +	ret = off;
> > +out:
> > +	kfree(elem_buf);
> > +	return ret;  
> 

On Thu, May 21, 2026 at 06:33:14AM -0700, Kees Cook wrote:
> 
> param_array_get() appends each element's string representation into the
> shared sysfs page buffer by passing buffer + off to the element getter.
> 
> That works for getters that only write a small bounded string, but
> param_get_charp() and similar helpers format against PAGE_SIZE from the
> pointer they receive. Once off is non-zero, an element getter can
> therefore write past the end of the original sysfs page buffer.
> 
> Collect each element into a temporary PAGE_SIZE buffer first and then
> copy only the remaining space into the caller's page buffer.

...

> +	elem_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);

get_free_page() (or how it is called)?

> +	if (!elem_buf)
> +		return -ENOMEM;
> +
>  	for (i = off = 0; i < (arr->num ? *arr->num : arr->max); i++) {
> -		/* Replace \n with comma */
> -		if (i)
> -			buffer[off - 1] = ',';
>  		p.arg = arr->elem + arr->elemsize * i;
>  		check_kparam_locked(p.mod);
> -		ret = arr->ops->get(buffer + off, &p);
> +		ret = arr->ops->get(elem_buf, &p);
>  		if (ret < 0)
> -			return ret;
> +			goto out;
> +		ret = min(ret, (int)(PAGE_SIZE - 1 - off));

It's usually discouraged to use castings in min/max/clamp. Can we make ret long
or do something different here?

> +		if (!ret)
> +			break;

> +		/* Replace the previous element's trailing newline with a comma. */
> +		if (i)
> +			buffer[off - 1] = ',';

Can't we do this after with help of strreplace()?

> +		memcpy(buffer + off, elem_buf, ret);
>  		off += ret;
> +		if (off == PAGE_SIZE - 1)
> +			break;
>  	}
>  	buffer[off] = '\0';
> -	return off;
> +	ret = off;
> +out:
> +	kfree(elem_buf);
> +	return ret;

-- 
With Best Regards,
Andy Shevchenko

On Thu, May 21, 2026 at 05:46:31PM +0100, David Laight wrote:
> On Thu, 21 May 2026 06:33:14 -0700
> Kees Cook <ke...@ke...> wrote:
> > Collect each element into a temporary PAGE_SIZE buffer first and then
> > copy only the remaining space into the caller's page buffer.
> 
> Should this be using a 4k buffer on all architectures?
> Initially perhaps just using a different name for the constant until
> all the associated PAGE_SIZE limits have been removed.

If we're acually going to think about this, even 4KiB is too big.
An 80x25 terminal is 2000 bytes (assuming no utf8), so 4KiB is two
entire screenfuls.  Limiting to 2048 would seem reasonable to me.

On Tue, May 26, 2026 at 08:53:06AM +0200, Petr Pavlu wrote:
> On 5/21/26 3:33 PM, Kees Cook wrote:
> > Hi,
> > 
> > I tried to trim the CC list here, but it's still pretty huge...
> > 
> > We've had a long-standing issue with "write to a string pointer" callbacks
> > that don't bounds check the destination (and for which the bounds is
> > also not part of the callback prototype, even if it is "known" to be
> > PAGE_SIZE, which sysfs_emit() depends on). Both moduleparams and sysfs
> > use this pattern. As a first step, and to test the migration method,
> > migrate moduleparams first.
> > 
> > There are 2 "mechanical" treewide patches that are handled by Coccinelle:
> > - treewide: Convert struct kernel_param_ops initializers to DEFINE_KERNEL_PARAM_OPS
> > - treewide: Convert custom kernel_param_ops .get callbacks to seq_buf via cocci
> > 
> > The last treewide patch is manual, and may need to be broken up into
> > per-subsystem patches, though I'd prefer to avoid this, as it would
> > extend the migration from 1 relase to at least 2 releases. (1 to
> > release the migration infrastructure, then 1 release to collect all the
> > subsystem changes, and possibly 1 more release to remove the migration
> > infrastructure.)
> > 
> > Thoughts, questions?
> 
> This looks reasonable to me. I added a few minor comments on the patches
> but they already look solid.

Thanks for the review! I'll get a v2 prepared with your notes addressed. :)

-Kees

-- 
Kees Cook

On Fri, May 29, 2026 at 01:03:39PM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> The driver explicitly set the .driver_data member of struct
> platform_device_id to zero without relying on that value. Drop this
> unused assignments.
> 
> While touching this array use a named initializer for assigning .name.

Sure, this is fine, it's in my next tree.

-corey

> 
> Signed-off-by: Uwe Kleine-König (The Capable Hub) <u.k...@ba...>
> ---
> Hello,
> 
> this patch is a preparation for changing struct platform_device_id
> see e.g.
> https://lore.kernel.org/all/cov...@ba.../
> for the details about the quest.
> 
> Best regards
> Uwe
> 
>  drivers/char/ipmi/ipmi_ssif.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index add043b812ea..07f1d2327bb7 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -2127,7 +2127,7 @@ static void ssif_platform_remove(struct platform_device *dev)
>  }
>  
>  static const struct platform_device_id ssif_plat_ids[] = {
> -    { "dmi-ipmi-ssif", 0 },
> +    { .name = "dmi-ipmi-ssif" },
>      { }
>  };
>  MODULE_DEVICE_TABLE(platform, ssif_plat_ids);
> 
> base-commit: f7af91adc230aa99e23330ecf85bc9badd9780ad
> -- 
> 2.47.3
> 

The driver explicitly set the .driver_data member of struct
platform_device_id to zero without relying on that value. Drop this
unused assignments.

While touching this array use a named initializer for assigning .name.

Signed-off-by: Uwe Kleine-König (The Capable Hub) <u.k...@ba...>
---
Hello,

this patch is a preparation for changing struct platform_device_id
see e.g.
https://lore.kernel.org/all/cov...@ba.../
for the details about the quest.

Best regards
Uwe

 drivers/char/ipmi/ipmi_ssif.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index add043b812ea..07f1d2327bb7 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -2127,7 +2127,7 @@ static void ssif_platform_remove(struct platform_device *dev)
 }
 
 static const struct platform_device_id ssif_plat_ids[] = {
-    { "dmi-ipmi-ssif", 0 },
+    { .name = "dmi-ipmi-ssif" },
     { }
 };
 MODULE_DEVICE_TABLE(platform, ssif_plat_ids);

base-commit: f7af91adc230aa99e23330ecf85bc9badd9780ad
-- 
2.47.3

On 5/21/26 3:33 PM, Kees Cook wrote:
> Hi,
> 
> I tried to trim the CC list here, but it's still pretty huge...
> 
> We've had a long-standing issue with "write to a string pointer" callbacks
> that don't bounds check the destination (and for which the bounds is
> also not part of the callback prototype, even if it is "known" to be
> PAGE_SIZE, which sysfs_emit() depends on). Both moduleparams and sysfs
> use this pattern. As a first step, and to test the migration method,
> migrate moduleparams first.
> 
> There are 2 "mechanical" treewide patches that are handled by Coccinelle:
> - treewide: Convert struct kernel_param_ops initializers to DEFINE_KERNEL_PARAM_OPS
> - treewide: Convert custom kernel_param_ops .get callbacks to seq_buf via cocci
> 
> The last treewide patch is manual, and may need to be broken up into
> per-subsystem patches, though I'd prefer to avoid this, as it would
> extend the migration from 1 relase to at least 2 releases. (1 to
> release the migration infrastructure, then 1 release to collect all the
> subsystem changes, and possibly 1 more release to remove the migration
> infrastructure.)
> 
> Thoughts, questions?

This looks reasonable to me. I added a few minor comments on the patches
but they already look solid.

-- 
Thanks,
Petr

On 5/21/26 3:33 PM, Kees Cook wrote:
> Convert the generic struct kernel_param_ops .get helpers in
> kernel/params.c directly to the seq_buf signature, drop their legacy
> "char *" form, and refresh prototypes in <linux/moduleparam.h>:
> 
>   param_get_byte/short/ushort/int/uint/long/ulong/ullong/hexint
>   param_get_charp/bool/invbool/string
>   param_array_get
> 
> The STANDARD_PARAM_DEF() macro expands to a seq_buf body for every
> numeric helper. param_array_get() now writes element output directly
> into the parent seq_buf when the element ops provide .get; it only
> allocates the per-call PAGE_SIZE bounce buffer when the element ops
> still use the legacy .get_str path. The common "rewrite the prior
> element's trailing newline as a comma" step lives outside both
> branches so the two paths share it.
> 
> The non-core changes in this commit (arch/x86/kvm, mm/kfence,
> drivers/dma/dmatest, security/apparmor) are the small set of callers that
> directly invoke one of the converted generic helpers from their own .get
> callback (e.g. an apparmor wrapper that adds a capability check and then
> delegates to param_get_bool()). Because the helpers' signature changes
> here, these wrappers must move in lockstep. Each of them is updated
> to take "struct seq_buf *" and pass it through; param_get_debug() in
> apparmor also pulls aa_print_debug_params() (and its val_mask_to_str()
> helper, in security/apparmor/lib.c) over to seq_buf, since that is the
> only consumer. No other behavioural change is intended.
> 
> Custom .get callbacks that do not delegate to a generic helper (and
> therefore still match the .get_str signature) are routed automatically
> to the .get_str field by the DEFINE_KERNEL_PARAM_OPS _Generic dispatcher
> and are deliberately left alone here, to be changed separately within
> their respective subsystems.
> 
> Signed-off-by: Kees Cook <ke...@ke...>
> ---
> [...]
> @@ -453,36 +457,46 @@ static int param_array_set(const char *val, const struct kernel_param *kp)
>  			   arr->num ?: &temp_num);
>  }
>  
> -static int param_array_get(char *buffer, const struct kernel_param *kp)
> +static int param_array_get(struct seq_buf *s, const struct kernel_param *kp)
>  {
> -	int i, off, ret;
> -	char *elem_buf;
>  	const struct kparam_array *arr = kp->arr;
>  	struct kernel_param p = *kp;
> +	char *elem_buf = NULL;
> +	int i, ret = 0;
>  
> -	elem_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> -	if (!elem_buf)
> -		return -ENOMEM;
> +	for (i = 0; i < (arr->num ? *arr->num : arr->max); i++) {
> +		size_t before = s->len;
>  
> -	for (i = off = 0; i < (arr->num ? *arr->num : arr->max); i++) {
>  		p.arg = arr->elem + arr->elemsize * i;
>  		check_kparam_locked(p.mod);
> -		ret = arr->ops->get_str(elem_buf, &p);
> -		if (ret < 0)
> -			goto out;
> -		ret = min(ret, (int)(PAGE_SIZE - 1 - off));
> -		if (!ret)
> +
> +		if (arr->ops->get) {
> +			ret = arr->ops->get(s, &p);
> +			if (ret < 0)
> +				goto out;
> +		} else {
> +			if (!elem_buf) {
> +				elem_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> +				if (!elem_buf) {
> +					ret = -ENOMEM;
> +					goto out;
> +				}
> +			}
> +			ret = arr->ops->get_str(elem_buf, &p);
> +			if (ret < 0)
> +				goto out;
> +			seq_buf_putmem(s, elem_buf, ret);
> +		}
> +
> +		/* Nothing got written (e.g. overflow) — stop. */
> +		if (s->len == before)
>  			break;
> +
>  		/* Replace the previous element's trailing newline with a comma. */
> -		if (i)
> -			buffer[off - 1] = ',';
> -		memcpy(buffer + off, elem_buf, ret);
> -		off += ret;
> -		if (off == PAGE_SIZE - 1)
> -			break;
> +		if (i && s->buffer[before - 1] == '\n')
> +			s->buffer[before - 1] = ',';
>  	}
> -	buffer[off] = '\0';
> -	ret = off;
> +	ret = 0;
>  out:
>  	kfree(elem_buf);
>  	return ret;

Since you're almost completely rewriting the logic in param_array_get(),
I suggest tightening it up a bit. The function could warn or return an
error when a kernel_param_ops::get/get_str() call adds a string that
doesn't terminate with '\n', specifically, when the call adds either
a zero-length string or a non-zero-length string that ends with
a different character (unless an overflow occurred).

The updated code silently stops the loop when a get call returns
a zero-length string. Similarly, handling of a string not terminated by
'\n' is halfway there because of the added check
"s->buffer[before - 1] == '\n'".

-- 
Thanks,
Petr

On 5/21/26 3:33 PM, Kees Cook wrote:
> Make the DEFINE_KERNEL_PARAM_OPS family route their _get argument to
> either .get (struct seq_buf *) or .get_str (char *) at compile time
> based on the pointer's actual function signature. Two helper macros
> do the routing:
> 
>   _KERNEL_PARAM_OPS_GET     - return the pointer if it has the seq_buf
>                               signature, otherwise NULL of that type
>   _KERNEL_PARAM_OPS_GET_STR - mirror image for the char * signature
> 
> Both use _Generic; only the two valid function-pointer types are
> listed, so any third-party type is a compile error rather than
> silently falling through.
> 
> Now a callback whose body has been migrated from char * to struct
> seq_buf * needs no change at its kernel_param_ops initialization site,
> because the macro picks up the new type automatically and assigns to
> the correct field.
> 
> Signed-off-by: Kees Cook <ke...@ke...>
> ---
>  include/linux/moduleparam.h | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index c52120f6ac28..795bc7c654ef 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -85,15 +85,32 @@ struct kernel_param_ops {
>   *
>   *   static DEFINE_KERNEL_PARAM_OPS(my_ops, my_set, my_get);
>   *
> - * Routing the @_set and @_get function pointers through the macro
> - * (rather than naming the struct fields at every call site) lets the
> - * field layout change in one place when callbacks are migrated to a
> - * new signature.
> + * @_get may be either of:
> + *   int (*)(struct seq_buf *, const struct kernel_param *) (seq_buf)
> + *   int (*)(char *, const struct kernel_param *)           (legacy)
> + *
> + * The macro uses _Generic to route the function pointer to the
> + * matching field (.get or .get_str) at compile time, leaving the
> + * other field NULL. Each helper matches the wrong prototype signature
> + * and returns NULL, falling through to the default branch otherwise;
> + * if @_get has neither expected signature the assignment to the
> + * fields gets a normal compile-time type-mismatch error.
>   */
> +#define _KERNEL_PARAM_OPS_GET(_get)					\
> +	_Generic((_get),						\
> +	    int (*)(char *, const struct kernel_param *): NULL,		\
> +	    default: (_get))
> +
> +#define _KERNEL_PARAM_OPS_GET_STR(_get)					\
> +	_Generic((_get),						\
> +	    int (*)(struct seq_buf *, const struct kernel_param *): NULL, \
> +	    default: (_get))
> +
>  #define DEFINE_KERNEL_PARAM_OPS(_name, _set, _get)			\
>  	const struct kernel_param_ops _name = {				\
>  		.set = (_set),						\
> -		.get_str = (_get),					\
> +		.get = _KERNEL_PARAM_OPS_GET(_get),			\
> +		.get_str = _KERNEL_PARAM_OPS_GET_STR(_get),		\
>  	}
>  
>  /* As DEFINE_KERNEL_PARAM_OPS, with KERNEL_PARAM_OPS_FL_NOARG set. */
> @@ -101,14 +118,16 @@ struct kernel_param_ops {
>  	const struct kernel_param_ops _name = {				\
>  		.flags = KERNEL_PARAM_OPS_FL_NOARG,			\
>  		.set = (_set),						\
> -		.get_str = (_get),					\
> +		.get = _KERNEL_PARAM_OPS_GET(_get),			\
> +		.get_str = _KERNEL_PARAM_OPS_GET_STR(_get),		\
>  	}
>  
>  /* As DEFINE_KERNEL_PARAM_OPS, with an additional .free callback. */
>  #define DEFINE_KERNEL_PARAM_OPS_FREE(_name, _set, _get, _free)		\
>  	const struct kernel_param_ops _name = {				\
>  		.set = (_set),						\
> -		.get_str = (_get),					\
> +		.get = _KERNEL_PARAM_OPS_GET(_get),			\
> +		.get_str = _KERNEL_PARAM_OPS_GET_STR(_get),		\
>  		.free = (_free),					\
>  	}
>  

Reviewed-by: Petr Pavlu <pet...@su...>

-- Petr

On 5/21/26 3:33 PM, Kees Cook wrote:
> Add a new struct kernel_param_ops::get callback whose signature
> takes a struct seq_buf instead of a raw char buffer:
> 
>   int (*get)(struct seq_buf *sb, const struct kernel_param *kp);
> 
> The previously-legacy .get field is now .get_str (char *buffer);
> .get is the new seq_buf-aware form.  param_attr_show() prefers .get
> when set, otherwise falls back to .get_str.  WARN_ON_ONCE() if both
> are set.  Return contract for .get:
> 
>   < 0 : errno propagated to userspace; seq_buf contents discarded
>   = 0 : success; length derived from seq_buf_used()
>   > 0 : forbidden; the dispatcher WARN_ON_ONCE()s and treats as 0
> 
> The default policy on seq_buf_has_overflowed() is silent truncation,
> matching scnprintf()/sysfs_emit() behaviour.  Callbacks that want a
> specific overflow errno can check seq_buf_has_overflowed() and
> return their preferred error.
> 
> No callbacks use .get yet; the legacy path is still the only one in use
> after this commit. A subsequent commit teaches DEFINE_KERNEL_PARAM_OPS
> to route initializers by type.
> 
> Signed-off-by: Kees Cook <ke...@ke...>

Reviewed-by: Petr Pavlu <pet...@su...>

-- Petr

On 5/21/26 3:33 PM, Kees Cook wrote:
> Using Coccinelle, rewrite every struct kernel_param_ops initializer that
> sets .get into a DEFINE_KERNEL_PARAM_OPS-family macro invocation,
> for example:
> 
> @@
> declarer name DEFINE_KERNEL_PARAM_OPS;
> identifier OPS;
> expression SET, GET;
> @@
> - const struct kernel_param_ops OPS = {
> -       .set = SET,
> -       .get = GET,
> - };
> + DEFINE_KERNEL_PARAM_OPS(OPS, SET, GET);
> 
> Using the macro for initialization means future changes can manipulate
> the struct layout and callback prototypes without having to change every
> initializer.

Nit: For consistency, I suggest also converting the few remaining
kernel_param_ops instances that specify only .set and no .get, such as
simdisk_param_ops_filename.

-- 
Thanks,
Petr

On 5/21/26 3:33 PM, Kees Cook wrote:
> Add macros that define a struct kernel_param_ops initializer through a
> macro so the underlying field layout can evolve without touching every
> call site. Three variants cover the three cases:
> 
>  DEFINE_KERNEL_PARAM_OPS(name, set, get) // basic
>  DEFINE_KERNEL_PARAM_OPS_NOARG(name, set, get) // set KERNEL_PARAM_OPS_FL_NOARG
>  DEFINE_KERNEL_PARAM_OPS_FREE(name, set, get, free) // also set .free
> 
> Callers prefix their own visibility qualifiers, e.g.:
> 
>   static DEFINE_KERNEL_PARAM_OPS(my_ops, my_set, my_get);
> 
> Also update module_param_call() and STANDARD_PARAM_DEF() to use
> DEFINE_KERNEL_PARAM_OPS internally so the generated ops table will go
> through the same macro as everything else.
> 
> Subsequent commits convert all open-coded struct kernel_param_ops
> definitions to use these macros, in preparation for migrating to a
> seq_buf .get API.
> 
> Signed-off-by: Kees Cook <ke...@ke...>
> ---
>  include/linux/moduleparam.h | 36 ++++++++++++++++++++++++++++++++++--
>  kernel/params.c             |  6 ++----
>  2 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index 075f28585074..26bf45b36d02 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -68,6 +68,39 @@ struct kernel_param_ops {
>  	void (*free)(void *arg);
>  };
>  
> +/*
> + * Define a const struct kernel_param_ops initializer. Callers prefix with
> + * any required visibility qualifiers (typically "static"):
> + *
> + *   static DEFINE_KERNEL_PARAM_OPS(my_ops, my_set, my_get);
> + *
> + * Routing the @_set and @_get function pointers through the macro
> + * (rather than naming the struct fields at every call site) lets the
> + * field layout change in one place when callbacks are migrated to a
> + * new signature.
> + */

Nit: The newly introduced DEFINE_KERNEL_PARAM_OPS*() macros remain in
place at the end of the series after the migration is complete and this
comment is removed in patch 7. It would be helpful to describe in the
commit message why these macros are generally preferable to defining
kernel_param_ops instances directly.

I assume the motivation is that the structure is simple enough and using
macros then makes defining kernel_param_ops instances a bit more
concise. A minor disadvantage is that some analysis tools, such as
ctags, may no longer see the generated definition, but that is also the
case for DEFINE_MUTEX() and other similar macros.

-- 
Thanks,
Petr