From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julia Lawall Subject: Re: [PATCH 3/5 v2] drivers/hid/hid-roccat.c: eliminate a null pointer dereference Date: Sat, 29 Oct 2011 20:18:26 +0200 (CEST) Message-ID: References: <1319846297-2985-3-git-send-email-julia@diku.dk> <20111029062412.GG14881@longonot.mountain> <4EABC937.60600@broadcom.com> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Return-path: Received: from mgw2.diku.dk ([130.225.96.92]:35076 "EHLO mgw2.diku.dk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932323Ab1J2SSd (ORCPT ); Sat, 29 Oct 2011 14:18:33 -0400 In-Reply-To: Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Jiri Kosina Cc: David Herrmann , Arend van Spriel , Dan Carpenter , "kernel-janitors@vger.kernel.org" , "linux-input@vger.kernel.org" , "linux-kernel@vger.kernel.org" On Sat, 29 Oct 2011, Jiri Kosina wrote: > On Sat, 29 Oct 2011, Julia Lawall wrote: > > > From: Julia Lawall > > > > It is not possible to take the lock in device if device is NULL. > > The mutex_lock is thus moved after the NULL test, and the relevant part of > > the shared error handling code is moved up. > > > > The semantic match that finds this problem is as follows: > > (http://coccinelle.lip6.fr/) > > > > // > > @r@ > > expression E, E1; > > identifier f; > > statement S1,S2,S3; > > @@ > > > > if (E == NULL) > > { > > ... when != if (E == NULL || ...) S1 else S2 > > when != E = E1 > > *E->f > > ... when any > > return ...; > > } > > else S3 > > // > > > > Signed-off-by: Julia Lawall > > > > --- > > mutex_lock changed to mutex_unlock in error handling code > > > > drivers/hid/hid-roccat.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c > > index 2596321..36a28b8 100644 > > --- a/drivers/hid/hid-roccat.c > > +++ b/drivers/hid/hid-roccat.c > > @@ -163,14 +163,15 @@ static int roccat_open(struct inode *inode, struct file *file) > > > > device = devices[minor]; > > > > - mutex_lock(&device->readers_lock); > > - > > if (!device) { > > pr_emerg("roccat device with minor %d doesn't exist\n", minor); > > - error = -ENODEV; > > - goto exit_err; > > + kfree(reader); > > + mutex_unlock(&devices_lock); > > + return -ENODEV; > > } > > > > + mutex_lock(&device->readers_lock); > > + > > if (!device->open++) { > > /* power on device on adding first reader */ > > error = hid_hw_power(device->hid, PM_HINT_FULLON); > > Julia, > > thanks a lot for fixing this. > > Could you please redo the patch in a way that it adds second > 'exit_unlock1' label (and renames 'exit_unlock' to 'exit_unlock2') (or > any appropriate variation of the names) and preserve error path using goto > instead of mixture of returns and gotos that this patch would introduce? OK. At first I couldn't see how to do this without duplicating the unlocks in the success and failure cases, but perhaps there is a solution by adding more gotos. I'll try for that. julia