Re: [Openipmi-developer] [PATCH v2] ipmi: Fix kernel panic at ipmi_ssif_thread()

[Corey Minyard]

On 03/27/2017 09:29 PM, joechang@xxxxxxxxxxxxxx wrote:
> Hi Corey,
>
> I really appreciate your patience and detail code review.

Not a problem at all, thanks for sticking with this.

This will be queued for the next release, and I'll request backports to 
stable trees.

Thanks,

-corey

> I re-generate new patch and send new patch to you.
> Please feel free to feedback to me if something need to modify from the
> new patch..
>
> Thank you,
> Joseph.
>
> Corey Minyard æ 2017-03-27 20:32 åå:
> > On 03/27/2017 02:31 AM, Joeseph Chang wrote:
> > > From: Joeseph Chang <joechang@xxxxxxxxxxxxxx>
> > >
> > > Since patch v1 touch ssif_info->multi_pos and ssif_info->multi_data
> > > after ssif_i2c_send(). msg_written_handler can be called at any time
> > > after ssif_i2c_send(). There is possible to have concurrent access to
> > > ssif_info->multi_pos or ssif_info->multi_data at msg_written_handler.
> > >
> > > Revert patch v1 and add new local pointer "i2c_data" which point to
> > > i2c data and size that will be sent out to avoid concurrent access in
> > > msg_written_handler().
> > >
> > > Signed-off-by: Joeseph Chang <joechang@xxxxxxxxxxxxxx>
> > > ---
> > >    drivers/char/ipmi/ipmi_ssif.c | 24 ++++++++++++------------
> > >    1 file changed, 12 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/char/ipmi/ipmi_ssif.c
> > > b/drivers/char/ipmi/ipmi_ssif.c
> > > index 39346ee..f36f018 100644
> > > --- a/drivers/char/ipmi/ipmi_ssif.c
> > > +++ b/drivers/char/ipmi/ipmi_ssif.c
> > > @@ -852,6 +852,7 @@ static void msg_written_handler(struct ssif_info
> > > *ssif_info, int result,
> > >    				unsigned char *data, unsigned int len)
> > >    {
> > >    	int rv;
> > > +	unsigned char *i2c_data;
> > The variable should be in the block where it is used.  The basic rule
> > is to limit scope as much as possible.
> >
> > The name is also a little vague, though not terrible.
> >
> > >      	/* We are single-threaded here, so no need for a lock. */
> > >    	if (result < 0) {
> > > @@ -899,13 +900,22 @@ 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;
> > > +		i2c_data = ssif_info->multi_data + ssif_info->multi_pos;
> > > +		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;
> > Can you make this change not relative to the previous change?  Just do
> > a "git rebase -i ...." and squash the
> > changes.
> >
> > I saw your previous emails, but I can't take those because of the
> > comments at the top and the text from
> > the reply.  You can add comments to a submission that don't go into
> > git, just put them after the '---" in
> > the header.
> >
> > Thanks,
> >
> > -corey
> >
> > >      		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_data,
> > >    				  I2C_SMBUS_BLOCK_DATA);
> > > -
> > >    		if (rv < 0) {
> > >    			/* request failed, just return the error. */
> > >    			ssif_inc_stat(ssif_info, send_errors);
> > > @@ -914,16 +924,6 @@ 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;
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Openipmi-developer mailing list
> Openipmi-developer@xxxxxxxxxxxxxxxxxxxxx
> https://lists.sourceforge.net/lists/listinfo/openipmi-developer