diff mbox series

[1/2] leds: trigger: Fix error path to not unlock the unlocked mutex

Message ID 20210219082955.5007-1-u.kleine-koenig@pengutronix.de
State New
Headers show
Series [1/2] leds: trigger: Fix error path to not unlock the unlocked mutex | expand

Commit Message

Uwe Kleine-König Feb. 19, 2021, 8:29 a.m. UTC 
ttyname is allocated before the mutex is taken, so it must not be
unlocked in the error path.

Reported-by: Pavel Machek <pavel@ucw.cz>
Fixes: fd4a641ac88f ("leds: trigger: implement a tty trigger")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/leds/trigger/ledtrig-tty.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Pavel Machek Feb. 19, 2021, 9:46 a.m. UTC | #1 
On Fri 2021-02-19 09:29:54, Uwe Kleine-König wrote:
> ttyname is allocated before the mutex is taken, so it must not be
> unlocked in the error path.
> 

Acked-by: Pavel Machek <pavel@ucw.cz>
Uwe Kleine-König Feb. 19, 2021, 9:52 a.m. UTC | #2 
On Fri, Feb 19, 2021 at 10:46:32AM +0100, Pavel Machek wrote:
> On Fri 2021-02-19 09:29:55, Uwe Kleine-König wrote:
> > led_set_brightness() involves scheduling a workqueue. As here the led's
> > brightness setting is done in context of the trigger's workqueue this is
> > unjustified overhead and it's more sensible to use
> > led_set_brightness_sync().
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>

Who is expected to pick these two up? I assume Greg as the commit that
is fixed here is only in his try for now?

Best regards
Uwe
kernel test robot Feb. 19, 2021, 10:38 a.m. UTC | #3 
Hi "Uwe,

I love your patch! Perhaps something to improve:

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on next-20210218]
[cannot apply to pavel-linux-leds/for-next staging/staging-testing v5.11]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Uwe-Kleine-K-nig/leds-trigger-Fix-error-path-to-not-unlock-the-unlocked-mutex/20210219-163217
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: powerpc-randconfig-r032-20210219 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c9439ca36342fb6013187d0a69aef92736951476)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/0day-ci/linux/commit/f13a575b98965827ab968d1c88d98c6b044d4492
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Uwe-Kleine-K-nig/leds-trigger-Fix-error-path-to-not-unlock-the-unlocked-mutex/20210219-163217
        git checkout f13a575b98965827ab968d1c88d98c6b044d4492
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/leds/trigger/ledtrig-tty.c:70:1: warning: unused label 'out_unlock' [-Wunused-label]
   out_unlock:
   ^~~~~~~~~~~
   1 warning generated.


vim +/out_unlock +70 drivers/leds/trigger/ledtrig-tty.c

fd4a641ac88fbb Uwe Kleine-König 2021-01-13  39  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  40  static ssize_t ttyname_store(struct device *dev,
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  41  			     struct device_attribute *attr, const char *buf,
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  42  			     size_t size)
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  43  {
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  44  	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  45  	char *ttyname;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  46  	ssize_t ret = size;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  47  	bool running;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  48  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  49  	if (size > 0 && buf[size - 1] == '\n')
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  50  		size -= 1;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  51  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  52  	if (size) {
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  53  		ttyname = kmemdup_nul(buf, size, GFP_KERNEL);
f13a575b989658 Uwe Kleine-König 2021-02-19  54  		if (!ttyname)
f13a575b989658 Uwe Kleine-König 2021-02-19  55  			return -ENOMEM;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  56  	} else {
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  57  		ttyname = NULL;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  58  	}
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  59  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  60  	mutex_lock(&trigger_data->mutex);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  61  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  62  	running = trigger_data->ttyname != NULL;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  63  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  64  	kfree(trigger_data->ttyname);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  65  	tty_kref_put(trigger_data->tty);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  66  	trigger_data->tty = NULL;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  67  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  68  	trigger_data->ttyname = ttyname;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  69  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13 @70  out_unlock:
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  71  	mutex_unlock(&trigger_data->mutex);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  72  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  73  	if (ttyname && !running)
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  74  		ledtrig_tty_restart(trigger_data);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  75  
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  76  	return ret;
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  77  }
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  78  static DEVICE_ATTR_RW(ttyname);
fd4a641ac88fbb Uwe Kleine-König 2021-01-13  79  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Pavel Machek Feb. 19, 2021, 11:23 a.m. UTC | #4 
On Fri 2021-02-19 11:41:49, Greg Kroah-Hartman wrote:
> On Fri, Feb 19, 2021 at 10:52:54AM +0100, Uwe Kleine-König wrote:
> > On Fri, Feb 19, 2021 at 10:46:32AM +0100, Pavel Machek wrote:
> > > On Fri 2021-02-19 09:29:55, Uwe Kleine-König wrote:
> > > > led_set_brightness() involves scheduling a workqueue. As here the led's
> > > > brightness setting is done in context of the trigger's workqueue this is
> > > > unjustified overhead and it's more sensible to use
> > > > led_set_brightness_sync().
> > > 
> > > Acked-by: Pavel Machek <pavel@ucw.cz>
> > 
> > Who is expected to pick these two up? I assume Greg as the commit that
> > is fixed here is only in his try for now?
> 
> I will do so once Linus takes my existing pull request, don't worry,
> they will make it into 5.12-final.

It seems like the label is now unused, so this causes warnings... see
the bot output.

Best regards,
									Pavel
diff mbox series

Patch

diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
index d2ab6ab080ac..68ed2c87a65c 100644
--- a/drivers/leds/trigger/ledtrig-tty.c
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -51,10 +51,8 @@  static ssize_t ttyname_store(struct device *dev,
 
 	if (size) {
 		ttyname = kmemdup_nul(buf, size, GFP_KERNEL);
-		if (!ttyname) {
-			ret = -ENOMEM;
-			goto out_unlock;
-		}
+		if (!ttyname)
+			return -ENOMEM;
 	} else {
 		ttyname = NULL;
 	}