Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S937092AbdCYCx3 (ORCPT ); Fri, 24 Mar 2017 22:53:29 -0400 Received: from mail-pg0-f65.google.com ([74.125.83.65]:36156 "EHLO mail-pg0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752051AbdCYCxY (ORCPT ); Fri, 24 Mar 2017 22:53:24 -0400 Reply-To: minyard@acm.org Subject: Re: [PATCH] ipmi: Fix kernel panic at ipmi_ssif_thread() References: <1490252832-20381-1-git-send-email-joechang@qti.qualcomm.com> To: Joeseph Chang , openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org Cc: anjiandi@qti.qualcomm.com, Joeseph Chang From: Corey Minyard Message-ID: <6d686ff9-e94b-ddc0-cf9f-13a839acbb08@acm.org> Date: Fri, 24 Mar 2017 21:53:16 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <1490252832-20381-1-git-send-email-joechang@qti.qualcomm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2579 Lines: 75 This is incorrect. These values *must* be set before ssif_i2c_send() is done. Once ssif_i2c_send() is called, msg_written_handler can be called at any time after that, including before where you moved the new code. If I understand this correctly, I think you need to add a variable, maybe named "bytes_to_write" and do bytes_to_write = ssif_info->multi_data + ssif_info->multi_pos; before the comparison and pass bytes_to_write into ssif_i2c_send(). Thanks, -corey On 03/23/2017 02:07 AM, Joeseph Chang wrote: > From: Joeseph Chang > > msg_written_handler() may set ssif_info->multi_data to NULL > when using ipmitool to write fru. > Change the ssif i2c send data sequence in msg_written_handler() > to fix NULL pointer kernel panic and incorrect ssif_info->multi_pos. > > Signed-off-by: Joeseph Chang > --- > drivers/char/ipmi/ipmi_ssif.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c > index cca6e5b..39346ee 100644 > --- a/drivers/char/ipmi/ipmi_ssif.c > +++ b/drivers/char/ipmi/ipmi_ssif.c > @@ -899,21 +899,13 @@ static void msg_written_handler(struct ssif_info *ssif_info, int result, > left = 32; > /* Length byte. */ > ssif_info->multi_data[ssif_info->multi_pos] = left; > - ssif_info->multi_pos += left; > - if (left < 32) > - /* > - * Write is finished. Note that we must end > - * with a write of less than 32 bytes to > - * complete the transaction, even if it is > - * zero bytes. > - */ > - ssif_info->multi_data = NULL; > > rv = ssif_i2c_send(ssif_info, msg_written_handler, > I2C_SMBUS_WRITE, > SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE, > ssif_info->multi_data + ssif_info->multi_pos, > I2C_SMBUS_BLOCK_DATA); > + > if (rv < 0) { > /* request failed, just return the error. */ > ssif_inc_stat(ssif_info, send_errors); > @@ -922,6 +914,16 @@ static void msg_written_handler(struct ssif_info *ssif_info, int result, > pr_info("Error from i2c_non_blocking_op(3)\n"); > msg_done_handler(ssif_info, -EIO, NULL, 0); > } > + > + ssif_info->multi_pos += left; > + if (left < 32) > + /* > + * Write is finished. Note that we must end > + * with a write of less than 32 bytes to > + * complete the transaction, even if it is > + * zero bytes. > + */ > + ssif_info->multi_data = NULL; > } else { > /* Ready to request the result. */ > unsigned long oflags, *flags;