From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2999039AbdDZL6P (ORCPT ); Wed, 26 Apr 2017 07:58:15 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:53124 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1949922AbdDZL6I (ORCPT ); Wed, 26 Apr 2017 07:58:08 -0400 Date: Wed, 26 Apr 2017 13:57:59 +0200 From: Greg KH To: Adrian Salido Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH] driver core: platform: fix race condition with driver_override Message-ID: <20170426115759.GB8813@kroah.com> References: <20170425235526.GA165155@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170425235526.GA165155@google.com> User-Agent: Mutt/1.8.2 (2017-04-18) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Apr 25, 2017 at 04:55:26PM -0700, Adrian Salido wrote: > The driver_override implementation is susceptible to race condition when > different threads are reading vs storing a different driver override. > Add locking to avoid race condition. > > Fixes: 3d713e0e382e ("driver core: platform: add device binding path 'driver_override'") > Cc: stable@vger.kernel.org > Signed-off-by: Adrian Salido > --- > drivers/base/platform.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index c2456839214a..493e03fa0e07 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -866,7 +866,7 @@ static ssize_t driver_override_store(struct device *dev, > const char *buf, size_t count) > { > struct platform_device *pdev = to_platform_device(dev); > - char *driver_override, *old = pdev->driver_override, *cp; > + char *driver_override, *old, *cp; > > if (count > PATH_MAX) > return -EINVAL; > @@ -879,12 +879,15 @@ static ssize_t driver_override_store(struct device *dev, > if (cp) > *cp = '\0'; > > + device_lock(dev); > + old = pdev->driver_override; > if (strlen(driver_override)) { > pdev->driver_override = driver_override; > } else { > kfree(driver_override); > pdev->driver_override = NULL; > } > + device_unlock(dev); > > kfree(old); Shouldn't you move the lock until after the kfree()? Or am I missing what the lock is trying to protect here? > > @@ -895,8 +898,12 @@ static ssize_t driver_override_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > struct platform_device *pdev = to_platform_device(dev); > + ssize_t len; > > - return sprintf(buf, "%s\n", pdev->driver_override); > + device_lock(dev); > + len = sprintf(buf, "%s\n", pdev->driver_override); > + device_unlock(dev); > + return len; Why does the show function need to be changed at all? How can anything "race" here? thanks, greg k-h