On Tue, Mar 09, 2010 at 09:12:13PM +0100, Arnd Bergmann wrote:
> On Tuesday 09 March 2010, Thadeu Lima de Souza Cascardo wrote:
> > MODULE_AUTHOR("Vojtech Pavlik <vojtech@xxxxxxx>");
> > @@ -1881,7 +1880,7 @@ static int input_open_file(struct inode *inode, struct file *file)
> > const struct file_operations *old_fops, *new_fops = NULL;
> > int err;
> >
> > - lock_kernel();
> > + mutex_lock(&input_mutex);
> > /* No load-on-demand here? */
> > handler = input_table[iminor(inode) >> 5];
> > if (!handler || !(new_fops = fops_get(handler->fops))) {
> > @@ -1909,7 +1908,7 @@ static int input_open_file(struct inode *inode, struct file *file)
> > }
> > fops_put(old_fops);
> > out:
> > - unlock_kernel();
> > + mutex_unlock(&input_mutex);
> > return err;
> > }
> >
> > --
>
> Well, actually please have a look at
> http://git.kernel.org/?p=linux/kernel/git/arnd/playground.git;a=commitdiff;h=c06fd0234357618a5741ce958d58901ae4cb7ac1
>
> * use mutex_lock_interruptible() where possible
> * you probably don't want to hold input_mutex when calling into the lower
> device's open function
>
> Arnd
Yeah. I was just looking at your appointed branch. And I've noticed your
change was better.
I was willing to let the open call go unprotected. But ended up checking
that the three callees were fine (they do not call any of the other
functions that take the mutex).
Since the fops_put/fops_get do protect that section from the handler
removal and I can't think of any other race right now, I think your
version is really better.
Acked-by: Thadeu Lima de Souza Cascardo <cascardo@xxxxxxxxxxxxxx>
Regards,
Cascardo.
Attachment:
signature.asc
Description: Digital signature
