From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756066AbcEES4i (ORCPT ); Thu, 5 May 2016 14:56:38 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:59118 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753955AbcEES4h (ORCPT ); Thu, 5 May 2016 14:56:37 -0400 Date: Thu, 5 May 2016 11:56:36 -0700 From: Greg Kroah-Hartman To: Reza Arbab Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH] memory-hotplug: fix store_mem_state() return value Message-ID: <20160505185636.GA24423@kroah.com> References: <1462473704-10671-1-git-send-email-arbab@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1462473704-10671-1-git-send-email-arbab@linux.vnet.ibm.com> User-Agent: Mutt/1.6.1 (2016-04-27) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 05, 2016 at 01:41:44PM -0500, Reza Arbab wrote: > Attempting to online memory which is already online will cause this: > > 1. store_mem_state() called with buf="online" > 2. device_online() returns 1 because device is already online > 3. store_mem_state() returns 1 > 4. calling code interprets this as 1-byte buffer read > 5. store_mem_state() called again with buf="nline" > 6. store_mem_state() returns -EINVAL > > Example: > > $ cat /sys/devices/system/memory/memory0/state > online > $ echo online > /sys/devices/system/memory/memory0/state > -bash: echo: write error: Invalid argument > > Fix the return value of store_mem_state() so this doesn't happen. > > Signed-off-by: Reza Arbab > --- > drivers/base/memory.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/base/memory.c b/drivers/base/memory.c > index 961e2cf..031950f 100644 > --- a/drivers/base/memory.c > +++ b/drivers/base/memory.c > @@ -359,9 +359,7 @@ store_mem_state(struct device *dev, > err: > unlock_device_hotplug(); > > - if (ret) > - return ret; > - return count; > + return ret < 0 ? ret : count; Please spell this out as a real if () statement, it's very hard to now see if you really did change the logic here or not. Hint, it's only a 1 line patch that way, right? thanks, greg k-h