[PATCH v2 0/3] ipmi: Close the race between __scan_channels() and deliver_response()

[PATCH v2 0/3] ipmi: Close the race between __scan_channels() and deliver_response()

[Jinhui Guo]

The command "ipmi -b -t" would occasionally fail:
  #ipmitool -b 6 -t 0x2c raw 0x6 0x01
  Unable to send command: Invalid argument
  Unable to send RAW command (channel=0x6 netfn=0x6 lun=0x0 cmd=0x1)

The race window between __scan_channels() and deliver_response() causes
the parameters of some channels to be set to 0.

Fix the race between __scan_channels() and deliver_response() with the
following changes.

1. Only assign intf->channel_list = intf->wchannels and set
   intf->channels_ready = true in channel_handler() after all channels_ready
   have been successfully scanned or after failing to send the IPMI
   request.
2. channel_handler() sets intf->channels_ready to true but no one clears
   it, preventing __scan_channels() from rescanning channels. When the BMC
   firmware changes a rescan is required. Allow it by clearing the flag
   before starting a new scan.
3. Channels remain static unless the BMC firmware changes. Skip channel
   rescan when no BMC firmware update has occurred.


v1: https://lore.kernel.org/all/20250929081602.1901-1-guojinhui.liam@bytedance.com/

Changelog in v1 -> v2 (suggested by corey):
 - Split the fix into three independent patches, each addressing a
   separate issue.
 - Clear intf->channels_ready only when the BMC firmware changes.

Jinhui Guo (3):
  ipmi: Fix the race between __scan_channels() and deliver_response()
  ipmi: Fix __scan_channels() failing to rescan channels
  ipmi: Skip channel scan if channels are already marked ready

 drivers/char/ipmi/ipmi_msghandler.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] ipmi: Fix the race between __scan_channels() and deliver_response()

[Jinhui Guo]

The race window between __scan_channels() and deliver_response() causes
the parameters of some channels to be set to 0.

1.[CPUA] __scan_channels() issues an IPMI request and waits with
         wait_event() until all channels have been scanned.
         wait_event() internally calls might_sleep(), which might
         yield the CPU. (Moreover, an interrupt can preempt
         wait_event() and force the task to yield the CPU.)
2.[CPUB] deliver_response() is invoked when the CPU receives the
         IPMI response. After processing a IPMI response,
         deliver_response() directly assigns intf->wchannels to
         intf->channel_list and sets intf->channels_ready to true.
         However, not all channels are actually ready for use.
3.[CPUA] Since intf->channels_ready is already true, wait_event()
         never enters __wait_event(). __scan_channels() immediately
         clears intf->null_user_handler and exits.
4.[CPUB] Once intf->null_user_handler is set to NULL, deliver_response()
         ignores further IPMI responses, leaving the remaining
	 channels zero-initialized and unusable.

CPUA                             CPUB
-------------------------------  -----------------------------
__scan_channels()
 intf->null_user_handler
       = channel_handler;
 send_channel_info_cmd(intf,
       0);
 wait_event(intf->waitq,
       intf->channels_ready);
  do {
   might_sleep();
                                 deliver_response()
                                  channel_handler()
                                   intf->channel_list =
				         intf->wchannels + set;
                                   intf->channels_ready = true;
                                   send_channel_info_cmd(intf,
                                         intf->curr_channel);
   if (condition)
    break;
   __wait_event(wq_head,
          condition);
  } while(0)
 intf->null_user_handler
       = NULL;
                                 deliver_response()
                                  if (!msg->user)
                                   if (intf->null_user_handler)
                                    rv = -EINVAL;
                                  return rv;
-------------------------------  -----------------------------

Fix the race between __scan_channels() and deliver_response() by
deferring both the assignment intf->channel_list = intf->wchannels
and the flag intf->channels_ready = true until all channels have
been successfully scanned or until the IPMI request has failed.

Signed-off-by: Jinhui Guo <guojinhui.liam@bytedance.com>
---
 drivers/char/ipmi/ipmi_msghandler.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 8e9050f99e9e..a6e2e8246ab1 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -3406,8 +3406,6 @@ channel_handler(struct ipmi_smi *intf, struct ipmi_recv_msg *msg)
 			intf->channels_ready = true;
 			wake_up(&intf->waitq);
 		} else {
-			intf->channel_list = intf->wchannels + set;
-			intf->channels_ready = true;
 			rv = send_channel_info_cmd(intf, intf->curr_channel);
 		}
 
-- 
2.20.1

[PATCH v2 2/3] ipmi: Fix __scan_channels() failing to rescan channels

[Jinhui Guo]

channel_handler() sets intf->channels_ready to true but never
clears it, so __scan_channels() skips any rescan. When the BMC
firmware changes a rescan is required. Allow it by clearing
the flag before starting a new scan.

Signed-off-by: Jinhui Guo <guojinhui.liam@bytedance.com>
---
 drivers/char/ipmi/ipmi_msghandler.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index a6e2e8246ab1..536484b8e52d 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -590,7 +590,8 @@ static void __ipmi_bmc_unregister(struct ipmi_smi *intf);
 static int __ipmi_bmc_register(struct ipmi_smi *intf,
 			       struct ipmi_device_id *id,
 			       bool guid_set, guid_t *guid, int intf_num);
-static int __scan_channels(struct ipmi_smi *intf, struct ipmi_device_id *id);
+static int __scan_channels(struct ipmi_smi *intf,
+				struct ipmi_device_id *id, bool rescan);
 
 static void free_ipmi_user(struct kref *ref)
 {
@@ -2657,7 +2658,7 @@ static int __bmc_get_device_id(struct ipmi_smi *intf, struct bmc_device *bmc,
 		if (__ipmi_bmc_register(intf, &id, guid_set, &guid, intf_num))
 			need_waiter(intf); /* Retry later on an error. */
 		else
-			__scan_channels(intf, &id);
+			__scan_channels(intf, &id, false);
 
 
 		if (!intf_set) {
@@ -2677,7 +2678,7 @@ static int __bmc_get_device_id(struct ipmi_smi *intf, struct bmc_device *bmc,
 		goto out_noprocessing;
 	} else if (memcmp(&bmc->fetch_id, &bmc->id, sizeof(bmc->id)))
 		/* Version info changes, scan the channels again. */
-		__scan_channels(intf, &bmc->fetch_id);
+		__scan_channels(intf, &bmc->fetch_id, true);
 
 	bmc->dyn_id_expiry = jiffies + IPMI_DYN_DEV_ID_EXPIRY;
 
@@ -3427,10 +3428,17 @@ channel_handler(struct ipmi_smi *intf, struct ipmi_recv_msg *msg)
 /*
  * Must be holding intf->bmc_reg_mutex to call this.
  */
-static int __scan_channels(struct ipmi_smi *intf, struct ipmi_device_id *id)
+static int __scan_channels(struct ipmi_smi *intf,
+				struct ipmi_device_id *id,
+				bool rescan)
 {
 	int rv;
 
+	if (rescan) {
+		/* Clear channels_ready to force channels rescan. */
+		intf->channels_ready = false;
+	}
+
 	if (ipmi_version_major(id) > 1
 			|| (ipmi_version_major(id) == 1
 			    && ipmi_version_minor(id) >= 5)) {
@@ -3632,7 +3640,7 @@ int ipmi_add_smi(struct module         *owner,
 	}
 
 	mutex_lock(&intf->bmc_reg_mutex);
-	rv = __scan_channels(intf, &id);
+	rv = __scan_channels(intf, &id, false);
 	mutex_unlock(&intf->bmc_reg_mutex);
 	if (rv)
 		goto out_err_bmc_reg;
-- 
2.20.1

[PATCH v2 3/3] ipmi: Skip channel scan if channels are already marked ready

[Jinhui Guo]

Channels remain static unless the BMC firmware changes.
Therefore, rescanning is unnecessary while they are marked
ready and no BMC update has occurred.

Signed-off-by: Jinhui Guo <guojinhui.liam@bytedance.com>
---
 drivers/char/ipmi/ipmi_msghandler.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 536484b8e52d..db8ef2e46488 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -3439,6 +3439,10 @@ static int __scan_channels(struct ipmi_smi *intf,
 		intf->channels_ready = false;
 	}
 
+	/* Skip channel scan if channels are already marked ready */
+	if (intf->channels_ready)
+		return 0;
+
 	if (ipmi_version_major(id) > 1
 			|| (ipmi_version_major(id) == 1
 			    && ipmi_version_minor(id) >= 5)) {
-- 
2.20.1

Re: [PATCH v2 0/3] ipmi: Close the race between __scan_channels() and deliver_response()

[Corey Minyard]

On Tue, Sep 30, 2025 at 03:42:36PM +0800, Jinhui Guo wrote:
> The command "ipmi -b -t" would occasionally fail:
>   #ipmitool -b 6 -t 0x2c raw 0x6 0x01
>   Unable to send command: Invalid argument
>   Unable to send RAW command (channel=0x6 netfn=0x6 lun=0x0 cmd=0x1)
> 
> The race window between __scan_channels() and deliver_response() causes
> the parameters of some channels to be set to 0.
> 
> Fix the race between __scan_channels() and deliver_response() with the
> following changes.
> 
> 1. Only assign intf->channel_list = intf->wchannels and set
>    intf->channels_ready = true in channel_handler() after all channels_ready
>    have been successfully scanned or after failing to send the IPMI
>    request.
> 2. channel_handler() sets intf->channels_ready to true but no one clears
>    it, preventing __scan_channels() from rescanning channels. When the BMC
>    firmware changes a rescan is required. Allow it by clearing the flag
>    before starting a new scan.
> 3. Channels remain static unless the BMC firmware changes. Skip channel
>    rescan when no BMC firmware update has occurred.
> 
> 
> v1: https://lore.kernel.org/all/20250929081602.1901-1-guojinhui.liam@bytedance.com/
> 
> Changelog in v1 -> v2 (suggested by corey):
>  - Split the fix into three independent patches, each addressing a
>    separate issue.
>  - Clear intf->channels_ready only when the BMC firmware changes.
> 
> Jinhui Guo (3):
>   ipmi: Fix the race between __scan_channels() and deliver_response()
>   ipmi: Fix __scan_channels() failing to rescan channels
>   ipmi: Skip channel scan if channels are already marked ready

I have these queued for 6.18.  I need to re-review them; that will
probably happen later in the cycle; I can't put them in until 6.17-rc1
releases.

Thanks,

-corey

> 
>  drivers/char/ipmi/ipmi_msghandler.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> -- 
> 2.20.1
> 

Re: [PATCH v2 0/3] ipmi: Close the race between __scan_channels() and deliver_response()

[Jinhui Guo]

On Fri, Oct 3, 2025 at 10:43:16AM -0500, Corey Minyard wrote:
> On Tue, Sep 30, 2025 at 03:42:36PM +0800, Jinhui Guo wrote:
> > The command "ipmi -b -t" would occasionally fail:
> >   #ipmitool -b 6 -t 0x2c raw 0x6 0x01
> >   Unable to send command: Invalid argument
> >   Unable to send RAW command (channel=0x6 netfn=0x6 lun=0x0 cmd=0x1)
> > 
> > The race window between __scan_channels() and deliver_response() causes
> > the parameters of some channels to be set to 0.
> > 
> > Fix the race between __scan_channels() and deliver_response() with the
> > following changes.
> > 
> > 1. Only assign intf->channel_list = intf->wchannels and set
> >    intf->channels_ready = true in channel_handler() after all channels_ready
> >    have been successfully scanned or after failing to send the IPMI
> >    request.
> > 2. channel_handler() sets intf->channels_ready to true but no one clears
> >    it, preventing __scan_channels() from rescanning channels. When the BMC
> >    firmware changes a rescan is required. Allow it by clearing the flag
> >    before starting a new scan.
> > 3. Channels remain static unless the BMC firmware changes. Skip channel
> >    rescan when no BMC firmware update has occurred.
> > 
> > 
> > v1: https://lore.kernel.org/all/20250929081602.1901-1-guojinhui.liam@bytedance.com/
> > 
> > Changelog in v1 -> v2 (suggested by corey):
> >  - Split the fix into three independent patches, each addressing a
> >    separate issue.
> >  - Clear intf->channels_ready only when the BMC firmware changes.
> > 
> > Jinhui Guo (3):
> >   ipmi: Fix the race between __scan_channels() and deliver_response()
> >   ipmi: Fix __scan_channels() failing to rescan channels
> >   ipmi: Skip channel scan if channels are already marked ready

> I have these queued for 6.18.  I need to re-review them; that will
> probably happen later in the cycle; I can't put them in until 6.17-rc1
> releases.

> Thanks,

> -corey

Hi, corey

Friendly ping — please let me know if you need anything else (rebase, more review,
test results) and I’ll be happy to take care of it.

Thanks for your time, and sorry for the noise.

Best regards,
Jinhui

> > 
> >  drivers/char/ipmi/ipmi_msghandler.c | 24 +++++++++++++++++-------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> > 
> > -- 
> > 2.20.1
> >

Re: [PATCH v2 0/3] ipmi: Close the race between __scan_channels() and deliver_response()

[Corey Minyard]

On Fri, Dec 05, 2025 at 04:01:02PM +0800, Jinhui Guo wrote:
> On Fri, Oct 3, 2025 at 10:43:16AM -0500, Corey Minyard wrote:
> > On Tue, Sep 30, 2025 at 03:42:36PM +0800, Jinhui Guo wrote:
> > > The command "ipmi -b -t" would occasionally fail:
> > >   #ipmitool -b 6 -t 0x2c raw 0x6 0x01
> > >   Unable to send command: Invalid argument
> > >   Unable to send RAW command (channel=0x6 netfn=0x6 lun=0x0 cmd=0x1)
> > > 
> > > The race window between __scan_channels() and deliver_response() causes
> > > the parameters of some channels to be set to 0.
> > > 
> > > Fix the race between __scan_channels() and deliver_response() with the
> > > following changes.
> > > 
> > > 1. Only assign intf->channel_list = intf->wchannels and set
> > >    intf->channels_ready = true in channel_handler() after all channels_ready
> > >    have been successfully scanned or after failing to send the IPMI
> > >    request.
> > > 2. channel_handler() sets intf->channels_ready to true but no one clears
> > >    it, preventing __scan_channels() from rescanning channels. When the BMC
> > >    firmware changes a rescan is required. Allow it by clearing the flag
> > >    before starting a new scan.
> > > 3. Channels remain static unless the BMC firmware changes. Skip channel
> > >    rescan when no BMC firmware update has occurred.
> > > 
> > > 
> > > v1: https://lore.kernel.org/all/20250929081602.1901-1-guojinhui.liam@bytedance.com/
> > > 
> > > Changelog in v1 -> v2 (suggested by corey):
> > >  - Split the fix into three independent patches, each addressing a
> > >    separate issue.
> > >  - Clear intf->channels_ready only when the BMC firmware changes.
> > > 
> > > Jinhui Guo (3):
> > >   ipmi: Fix the race between __scan_channels() and deliver_response()
> > >   ipmi: Fix __scan_channels() failing to rescan channels
> > >   ipmi: Skip channel scan if channels are already marked ready
> 
> > I have these queued for 6.18.  I need to re-review them; that will
> > probably happen later in the cycle; I can't put them in until 6.17-rc1
> > releases.
> 
> > Thanks,
> 
> > -corey
> 
> Hi, corey
> 
> Friendly ping — please let me know if you need anything else (rebase, more review,
> test results) and I’ll be happy to take care of it.
> 
> Thanks for your time, and sorry for the noise.

No, it was a good reminder for me, I needed to handle this.

It's been sent to Linus.

Thanks,

-corey

> 
> Best regards,
> Jinhui
> 
> > > 
> > >  drivers/char/ipmi/ipmi_msghandler.c | 24 +++++++++++++++++-------
> > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > > 
> > > -- 
> > > 2.20.1
> > >