There were places where nothing would get started if a message
allocation failed, so the driver needs to return to normal state.
Fixes: 1da177e4c3 ("Linux-2.6.12-rc2")
Cc: <stable@vger.kernel.org>
Signed-off-by: Corey Minyard <corey@minyard.net>
The driver would just fetch events and receive messages until the
BMC said it was done. To avoid issues with BMCs that never say they are
done, add a limit of 10 fetches at a time.
In addition, an si interface has an attn state it can return from the
hardware which is supposed to cause a flag fetch to see if the driver
needs to fetch events or message or a few other things. If the attn
bit gets stuck, it's a similar problem. So allow messages in between
flag fetches so the driver itself doesn't get stuck.
This is a more general fix than the previous fix for the specific bad
BMC, but should fix the more general issue of a BMC that won't stop
saying it has data.
This has been there from the beginning of the driver. It's not a bug
per-se, but it is accounting for bugs in BMCs.
Reported-by: Matt Fleming <mfleming@cloudflare.com>
Closes: https://lore.kernel.org/lkml/20260415115930.3428942-1-matt@readmodwrite.com/
Fixes: <1da177e4c3f4> ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Corey Minyard <corey@minyard.net>
If an error occurs after the ssif kthread is created, but before the
main IPMI code starts the ssif interface, the ssif kthread will not
be stopped.
So make sure the kthread is stopped on an error condition if it is
running.
Fixes: 259307074b ("ipmi: Add SMBus interface driver (SSIF)")
Reported-by: Li Xiao <<252270051@hdu.edu.cn>
Cc: stable@vger.kernel.org
Reviewed-by: Li Xiao <252270051@hdu.edu.cn>
Signed-off-by: Corey Minyard <corey@minyard.net>
A section was in {} that didn't need to be, move the variable
definition to the top and set th eindentino properly.
Signed-off-by: Corey Minyard <corey@minyard.net>
Building with CONFIG_KUNIT=m and CONFIG_SSIF_IPMI_BMC_KUNIT_TEST=y
results in link errors such as:
undefined reference to `kunit_binary_assert_format'
undefined reference to `__kunit_do_failed_assertion'
This happens because the test code is built-in while the KUnit core
is built as a module, so the required KUnit symbols are not available
at link time.
Fix this by requiring KUNIT to be built-in when enabling
SSIF_IPMI_BMC_KUNIT_TEST.
Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202604071448.zUBjPYPu-lkp@intel.com/
Message-ID: <20260407094647.356661-1-zhangjian.3032@bytedance.com>
Signed-off-by: Corey Minyard <corey@minyard.net>
Long-running tests indicate that this logging can occasionally disrupt
timing and lead to request/response corruption.
Irq handler need to be executed as fast as possible,
most I2C slave IRQ implementations are byte-level, logging here
can significantly affect transfer behavior and timing. It is recommended
to use dev_dbg() for these messages.
Fixes: dd2bc5cc9e ("ipmi: ssif_bmc: Add SSIF BMC driver")
Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com>
Message-ID: <20260403090603.3988423-4-zhangjian.3032@bytedance.com>
Signed-off-by: Corey Minyard <corey@minyard.net>
copy_to_user() returns the number of bytes that could not be copied,
with a non-zero value indicating a partial or complete failure. The
current code only checks for negative return values and treats all
non-negative results as success.
Treating any positive return value from copy_to_user() as
an error and returning -EFAULT.
Fixes: dd2bc5cc9e ("ipmi: ssif_bmc: Add SSIF BMC driver")
Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com>
Message-ID: <20260403090603.3988423-2-zhangjian.3032@bytedance.com>
Signed-off-by: Corey Minyard <corey@minyard.net>
The response timer can stay armed across device teardown. If it fires after
remove, the callback dereferences the SSIF context and the i2c client after
teardown has started.
Cancel the timer in remove so the callback cannot run after the device is
unregistered.
Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com>
Message-ID: <20260403090603.3988423-1-zhangjian.3032@bytedance.com>
Signed-off-by: Corey Minyard <corey@minyard.net>
This patch continues the effort to refactor workqueue APIs, which has begun
with the changes introducing new workqueues and a new alloc_workqueue flag:
commit 128ea9f6cc ("workqueue: Add system_percpu_wq and system_dfl_wq")
commit 930c2ea566 ("workqueue: Add new WQ_PERCPU flag")
The point of the refactoring is to eventually alter the default behavior of
workqueues to become unbound by default so that their workload placement is
optimized by the scheduler.
Before that to happen after a careful review and conversion of each individual
case, workqueue users must be converted to the better named new workqueues with
no intended behaviour changes:
system_wq -> system_percpu_wq
system_unbound_wq -> system_dfl_wq
This way the old obsolete workqueues (system_wq, system_unbound_wq) can be
removed in the future.
Suggested-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
Message-ID: <20251224161301.135382-1-marco.crivellari@suse.com>
Signed-off-by: Corey Minyard <corey@minyard.net>
Pull IPMI driver fixes from Corey Minyard:
"This mostly revolves around getting the driver to behave when the IPMI
device misbehaves. Past attempts have not worked very well because I
didn't have hardware I could make do this, and AI was fairly useless
for help on this.
So I modified qemu and my test suite so I could reproduce a
misbehaving IPMI device, and with that I was able to fix the issues"
* tag 'for-linus-7.0-1' of https://github.com/cminyard/linux-ipmi:
ipmi:si: Fix check for a misbehaving BMC
ipmi:msghandler: Handle error returns from the SMI sender
ipmi:si: Don't block module unload if the BMC is messed up
ipmi:si: Use a long timeout when the BMC is misbehaving
ipmi:si: Handle waiting messages when BMC failure detected
ipmi:ls2k: Make ipmi_ls2k_platform_driver static
ipmi: ipmb: initialise event handler read bytes
ipmi: Consolidate the run to completion checking for xmit msgs lock
ipmi: Fix use-after-free and list corruption on sender error
There is a race on checking the state in the sender, it needs to be
checked under a lock. But you also need a check to avoid issues with
a misbehaving BMC for run to completion mode. So leave the check at
the beginning for run to completion, and add a check under the lock
to avoid the race.
Reported-by: Rafael J. Wysocki <rafael@kernel.org>
Fixes: bc3a9d2177 ("ipmi:si: Gracefully handle if the BMC is non-functional")
Cc: stable@vger.kernel.org # 4.18
Signed-off-by: Corey Minyard <corey@minyard.net>
Reviewed-by: Rafael J. Wysocki (Intel) <rafael@kernel.org>
It used to be, until recently, that the sender operation on the low
level interfaces would not fail. That's not the case any more with
recent changes.
So check the return value from the sender operation, and propagate it
back up from there and handle the errors in all places.
Reported-by: Rafael J. Wysocki <rafael@kernel.org>
Fixes: bc3a9d2177 ("ipmi:si: Gracefully handle if the BMC is non-functional")
Cc: stable@vger.kernel.org # 4.18
Signed-off-by: Corey Minyard <corey@minyard.net>
Reviewed-by: Rafael J. Wysocki (Intel) <rafael@kernel.org>
If the BMC is in a bad state, don't bother waiting for queues messages
since there can't be any. Otherwise the unload is blocked until the
BMC is back in a good state.
Reported-by: Rafael J. Wysocki <rafael@kernel.org>
Fixes: bc3a9d2177 ("ipmi:si: Gracefully handle if the BMC is non-functional")
Cc: stable@vger.kernel.org # 4.18
Signed-off-by: Corey Minyard <corey@minyard.net>
Reviewed-by: Rafael J. Wysocki (Intel) <rafael@kernel.org>
This was done entirely with mindless brute force, using
git grep -l '\<k[vmz]*alloc_objs*(.*, GFP_KERNEL)' |
xargs sed -i 's/\(alloc_objs*(.*\), GFP_KERNEL)/\1)/'
to convert the new alloc_obj() users that had a simple GFP_KERNEL
argument to just drop that argument.
Note that due to the extreme simplicity of the scripting, any slightly
more complex cases spread over multiple lines would not be triggered:
they definitely exist, but this covers the vast bulk of the cases, and
the resulting diff is also then easier to check automatically.
For the same reason the 'flex' versions will be done as a separate
conversion.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
This is the result of running the Coccinelle script from
scripts/coccinelle/api/kmalloc_objs.cocci. The script is designed to
avoid scalar types (which need careful case-by-case checking), and
instead replace kmalloc-family calls that allocate struct or union
object instances:
Single allocations: kmalloc(sizeof(TYPE), ...)
are replaced with: kmalloc_obj(TYPE, ...)
Array allocations: kmalloc_array(COUNT, sizeof(TYPE), ...)
are replaced with: kmalloc_objs(TYPE, COUNT, ...)
Flex array allocations: kmalloc(struct_size(PTR, FAM, COUNT), ...)
are replaced with: kmalloc_flex(*PTR, FAM, COUNT, ...)
(where TYPE may also be *VAR)
The resulting allocations no longer return "void *", instead returning
"TYPE *".
Signed-off-by: Kees Cook <kees@kernel.org>
It made things hard to read, move the check to a function.
Signed-off-by: Corey Minyard <corey@minyard.net>
Reviewed-by: Breno Leitao <leitao@debian.org>
The analysis from Breno:
When the SMI sender returns an error, smi_work() delivers an error
response but then jumps back to restart without cleaning up properly:
1. intf->curr_msg is not cleared, so no new message is pulled
2. newmsg still points to the message, causing sender() to be called
again with the same message
3. If sender() fails again, deliver_err_response() is called with
the same recv_msg that was already queued for delivery
This causes list_add corruption ("list_add double add") because the
recv_msg is added to the user_msgs list twice. Subsequently, the
corrupted list leads to use-after-free when the memory is freed and
reused, and eventually a NULL pointer dereference when accessing
recv_msg->done.
The buggy sequence:
sender() fails
-> deliver_err_response(recv_msg) // recv_msg queued for delivery
-> goto restart // curr_msg not cleared!
sender() fails again (same message!)
-> deliver_err_response(recv_msg) // tries to queue same recv_msg
-> LIST CORRUPTION
Fix this by freeing the message and setting it to NULL on a send error.
Also, always free the newmsg on a send error, otherwise it will leak.
Reported-by: Breno Leitao <leitao@debian.org>
Closes: https://lore.kernel.org/lkml/20260127-ipmi-v1-0-ba5cc90f516f@debian.org/
Fixes: 9cf93a8fa9 ("ipmi: Allow an SMI sender to return an error")
Cc: stable@vger.kernel.org # 4.18
Reviewed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Corey Minyard <corey@minyard.net>
Corey Minyard
Jian ZhangandCorey Minyard
Marco CrivellariandCorey Minyard
Linus Torvalds
Kees Cook
Matt JohnstonandCorey Minyard