[PATCH 0/4] ipmi: Some minor fixes

[PATCH 0/4] ipmi: Some minor fixes

[Corey Minyard]

Some minor fixes I had queued up.  The last one came in recently (patch 4) 
and it and patch 2 are candidates for stable-kernel.

[PATCH 1/4] drivers: char: ipmi: Replaced kmalloc and strcpy with kstrdup

[Corey Minyard]

From: Alexandru Gheorghiu <gheorghiuandru@gmail.com>

Replaced calls to kmalloc followed by strcpy with a sincle call to kstrdup.
Patch found using coccinelle.

Signed-off-by: Alexandru Gheorghiu <gheorghiuandru@gmail.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/char/ipmi/ipmi_msghandler.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 4d439d2..4445fa1 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -2037,12 +2037,11 @@ int ipmi_smi_add_proc_entry(ipmi_smi_t smi, char *name,
 	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
 	if (!entry)
 		return -ENOMEM;
-	entry->name = kmalloc(strlen(name)+1, GFP_KERNEL);
+	entry->name = kstrdup(name, GFP_KERNEL);
 	if (!entry->name) {
 		kfree(entry);
 		return -ENOMEM;
 	}
-	strcpy(entry->name, name);
 
 	file = proc_create_data(name, 0, smi->proc_dir, proc_ops, data);
 	if (!file) {
-- 
1.7.9.5

[PATCH 2/4] drivers/char/ipmi: memcpy, need additional 2 bytes to avoid memory overflow

[Corey Minyard]

From: Chen Gang <gang.chen@asianux.com>

  when calling memcpy, read_data and write_data need additional 2 bytes.

  write_data:
    for checking:  "if (size > IPMI_MAX_MSG_LENGTH)"
    for operating: "memcpy(bt->write_data + 3, data + 1, size - 1)"

  read_data:
    for checking:  "if (msg_len < 3 || msg_len > IPMI_MAX_MSG_LENGTH)"
    for operating: "memcpy(data + 2, bt->read_data + 4, msg_len - 2)"

Signed-off-by: Chen Gang <gang.chen@asianux.com>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: stable@vger.kernel.org
---
 drivers/char/ipmi/ipmi_bt_sm.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_bt_sm.c b/drivers/char/ipmi/ipmi_bt_sm.c
index cdd4c09f..a22a7a5 100644
--- a/drivers/char/ipmi/ipmi_bt_sm.c
+++ b/drivers/char/ipmi/ipmi_bt_sm.c
@@ -95,9 +95,9 @@ struct si_sm_data {
 	enum bt_states	state;
 	unsigned char	seq;		/* BT sequence number */
 	struct si_sm_io	*io;
-	unsigned char	write_data[IPMI_MAX_MSG_LENGTH];
+	unsigned char	write_data[IPMI_MAX_MSG_LENGTH + 2]; /* +2 for memcpy */
 	int		write_count;
-	unsigned char	read_data[IPMI_MAX_MSG_LENGTH];
+	unsigned char	read_data[IPMI_MAX_MSG_LENGTH + 2]; /* +2 for memcpy */
 	int		read_count;
 	int		truncated;
 	long		timeout;	/* microseconds countdown */
-- 
1.7.9.5

[PATCH 3/4] ipmi: Improve error messages on failed irq enable

[Corey Minyard]

When the interrupt enable message returns an error, the messages are
not entirely accurate nor helpful.  So improve them.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Andy Lutomirski <luto@amacapital.net>
---
 drivers/char/ipmi/ipmi_si_intf.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 313538a..af4b23f 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -663,8 +663,10 @@ static void handle_transaction_done(struct smi_info *smi_info)
 		/* We got the flags from the SMI, now handle them. */
 		smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
 		if (msg[2] != 0) {
-			dev_warn(smi_info->dev, "Could not enable interrupts"
-				 ", failed get, using polled mode.\n");
+			dev_warn(smi_info->dev,
+				 "Couldn't get irq info: %x.\n", msg[2]);
+			dev_warn(smi_info->dev,
+				 "Maybe ok, but ipmi might run very slowly.\n");
 			smi_info->si_state = SI_NORMAL;
 		} else {
 			msg[0] = (IPMI_NETFN_APP_REQUEST << 2);
@@ -685,10 +687,12 @@ static void handle_transaction_done(struct smi_info *smi_info)
 
 		/* We got the flags from the SMI, now handle them. */
 		smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
-		if (msg[2] != 0)
-			dev_warn(smi_info->dev, "Could not enable interrupts"
-				 ", failed set, using polled mode.\n");
-		else
+		if (msg[2] != 0) {
+			dev_warn(smi_info->dev,
+				 "Couldn't set irq info: %x.\n", msg[2]);
+			dev_warn(smi_info->dev,
+				 "Maybe ok, but ipmi might run very slowly.\n");
+		} else
 			smi_info->interrupt_disabled = 0;
 		smi_info->si_state = SI_NORMAL;
 		break;
-- 
1.7.9.5

Re: [PATCH 3/4] ipmi: Improve error messages on failed irq enable

[Andy Lutomirski]

On Thu, May 16, 2013 at 12:04 PM, Corey Minyard <cminyard@mvista.com> wrote:
> When the interrupt enable message returns an error, the messages are
> not entirely accurate nor helpful.  So improve them.
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Andy Lutomirski <luto@amacapital.net>
> ---
>  drivers/char/ipmi/ipmi_si_intf.c |   16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 313538a..af4b23f 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -663,8 +663,10 @@ static void handle_transaction_done(struct smi_info *smi_info)
>                 /* We got the flags from the SMI, now handle them. */
>                 smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
>                 if (msg[2] != 0) {
> -                       dev_warn(smi_info->dev, "Could not enable interrupts"
> -                                ", failed get, using polled mode.\n");
> +                       dev_warn(smi_info->dev,
> +                                "Couldn't get irq info: %x.\n", msg[2]);
> +                       dev_warn(smi_info->dev,
> +                                "Maybe ok, but ipmi might run very slowly.\n");
>                         smi_info->si_state = SI_NORMAL;
>                 } else {
>                         msg[0] = (IPMI_NETFN_APP_REQUEST << 2);
> @@ -685,10 +687,12 @@ static void handle_transaction_done(struct smi_info *smi_info)
>
>                 /* We got the flags from the SMI, now handle them. */
>                 smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
> -               if (msg[2] != 0)
> -                       dev_warn(smi_info->dev, "Could not enable interrupts"
> -                                ", failed set, using polled mode.\n");
> -               else
> +               if (msg[2] != 0) {
> +                       dev_warn(smi_info->dev,
> +                                "Couldn't set irq info: %x.\n", msg[2]);
> +                       dev_warn(smi_info->dev,
> +                                "Maybe ok, but ipmi might run very slowly.\n");
> +               } else

Minor nit: it would be nice if these warnings were collapsed into a
single printk -- that would save me a whitelist entry of acceptable
KERN_WARNING messages :)

My Dell 12g server says:

[97627.407724] ipmi_si ipmi_si.0: Using irq 10
[97627.421369] ipmi_si ipmi_si.0: Couldn't set irq info: cc.
[97627.427389] ipmi_si ipmi_si.0: Maybe ok, but ipmi might run very slowly.

Tested-by: Andy Lutomirski <luto@amacapital.net>

--Andy

Re: [PATCH 3/4] ipmi: Improve error messages on failed irq enable

[Corey Minyard]

On 05/16/2013 05:23 PM, Andy Lutomirski wrote:
>
>                  /* We got the flags from the SMI, now handle them. */
>                  smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
> -               if (msg[2] != 0)
> -                       dev_warn(smi_info->dev, "Could not enable interrupts"
> -                                ", failed set, using polled mode.\n");
> -               else
> +               if (msg[2] != 0) {
> +                       dev_warn(smi_info->dev,
> +                                "Couldn't set irq info: %x.\n", msg[2]);
> +                       dev_warn(smi_info->dev,
> +                                "Maybe ok, but ipmi might run very slowly.\n");
> +               } else
> Minor nit: it would be nice if these warnings were collapsed into a
> single printk -- that would save me a whitelist entry of acceptable
> KERN_WARNING messages :)

Yeah, the trouble is that checkpatch will give a warning if you split a 
string
between two lines or if a line is longer than 80 characters.  I'm not 
creative
enough to fit it into a single line.  Maybe I'm trying to be too literal 
here,
but I split it into two prints to avoid the warning.

>
> My Dell 12g server says:
>
> [97627.407724] ipmi_si ipmi_si.0: Using irq 10
> [97627.421369] ipmi_si ipmi_si.0: Couldn't set irq info: cc.
> [97627.427389] ipmi_si ipmi_si.0: Maybe ok, but ipmi might run very slowly.
>
> Tested-by: Andy Lutomirski <luto@amacapital.net>

Thanks a bunch.

-corey

> --Andy

Re: [PATCH 3/4] ipmi: Improve error messages on failed irq enable

[Joe Perches]

On Thu, 2013-05-16 at 22:47 -0500, Corey Minyard wrote:
> On 05/16/2013 05:23 PM, Andy Lutomirski wrote:
> >
> >                  /* We got the flags from the SMI, now handle them. */
> >                  smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
> > -               if (msg[2] != 0)
> > -                       dev_warn(smi_info->dev, "Could not enable interrupts"
> > -                                ", failed set, using polled mode.\n");
> > -               else
> > +               if (msg[2] != 0) {
> > +                       dev_warn(smi_info->dev,
> > +                                "Couldn't set irq info: %x.\n", msg[2]);
> > +                       dev_warn(smi_info->dev,
> > +                                "Maybe ok, but ipmi might run very slowly.\n");
> > +               } else
> > Minor nit: it would be nice if these warnings were collapsed into a
> > single printk -- that would save me a whitelist entry of acceptable
> > KERN_WARNING messages :)
> 
> Yeah, the trouble is that checkpatch will give a warning if you split a 
> string
> between two lines or if a line is longer than 80 characters.

Hi Corey.

Yes it will and no it won't.

	dev_<level>(struct device *, "some really really long format string that makes the line longer than 80 chars\n");

passes checkpatch without warning just fine.

I'd use something like:

			dev_warn(smi_info->dev,
				 "Couldn't set irq info: %x - this may be OK, but ipmi might run very slowly\n",
				 msg[2]);

[PATCH 4/4] ipmi: ipmi_devintf: compat_ioctl method fails to take ipmi_mutex

[Corey Minyard]

From: Benjamin LaHaise <bcrl@kvack.org>

When a 32 bit version of ipmitool is used on a 64 bit kernel, the
ipmi_devintf code fails to correctly acquire ipmi_mutex.  This results in
incomplete data being retrieved in some cases, or other possible failures.
Add a wrapper around compat_ipmi_ioctl() to take ipmi_mutex to fix this.

Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: stable@vger.kernel.org
---
 drivers/char/ipmi/ipmi_devintf.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c
index 9eb360f..d5a5f02 100644
--- a/drivers/char/ipmi/ipmi_devintf.c
+++ b/drivers/char/ipmi/ipmi_devintf.c
@@ -837,13 +837,25 @@ static long compat_ipmi_ioctl(struct file *filep, unsigned int cmd,
 		return ipmi_ioctl(filep, cmd, arg);
 	}
 }
+
+static long unlocked_compat_ipmi_ioctl(struct file *filep, unsigned int cmd,
+				       unsigned long arg)
+{
+	int ret;
+
+	mutex_lock(&ipmi_mutex);
+	ret = compat_ipmi_ioctl(filep, cmd, arg);
+	mutex_unlock(&ipmi_mutex);
+
+	return ret;
+}
 #endif
 
 static const struct file_operations ipmi_fops = {
 	.owner		= THIS_MODULE,
 	.unlocked_ioctl	= ipmi_unlocked_ioctl,
 #ifdef CONFIG_COMPAT
-	.compat_ioctl   = compat_ipmi_ioctl,
+	.compat_ioctl   = unlocked_compat_ipmi_ioctl,
 #endif
 	.open		= ipmi_open,
 	.release	= ipmi_release,
-- 
1.7.9.5