Prev: [PATCH 6/6] procfs: Kill the bkl in ioctl
Next: procfs: Kill BKL in llseek on proc base
From: Alexey Dobriyan on 30 Mar 2010 02:40
On Tue, Mar 30, 2010 at 9:20 AM, Frederic Weisbecker <fweisbec(a)gmail.com> wrote:

> --- a/net/sunrpc/cache.c
> +++ b/net/sunrpc/cache.c
> @@ -1331,12 +1331,22 @@ static unsigned int cache_poll_procfs(struct file *filp, poll_table *wait)
> � � � �return cache_poll(filp, wait, cd);
> �}
>
> -static int cache_ioctl_procfs(struct inode *inode, struct file *filp,
> - � � � � � � � � � � � � � � unsigned int cmd, unsigned long arg)
> +static long cache_ioctl_procfs(struct file *filp,
> + � � � � � � � � � � � � � � �unsigned int cmd, unsigned long arg)
> �{
> - � � � struct cache_detail *cd = PDE(inode)->data;
> + � � � long ret;
> + � � � struct cache_detail *cd;
> + � � � struct inode *inode = filp->f_path.dentry->d_inode;
>
> - � � � return cache_ioctl(inode, filp, cmd, arg, cd);
> + � � � /* Pushed down from procfs ioctl handler */
> + � � � lock_kernel();
> +
> + � � � cd = PDE(inode)->data;

->data is not under BKL at all.

> + � � � ret = cache_ioctl(inode, filp, cmd, arg, cd);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 30 Mar 2010 03:10
On Tue, Mar 30, 2010 at 09:31:33AM +0300, Alexey Dobriyan wrote:
> On Tue, Mar 30, 2010 at 9:20 AM, Frederic Weisbecker <fweisbec(a)gmail.com> wrote:
>
> > --- a/net/sunrpc/cache.c
> > +++ b/net/sunrpc/cache.c
> > @@ -1331,12 +1331,22 @@ static unsigned int cache_poll_procfs(struct file *filp, poll_table *wait)
> > � � � �return cache_poll(filp, wait, cd);
> > �}
> >
> > -static int cache_ioctl_procfs(struct inode *inode, struct file *filp,
> > - � � � � � � � � � � � � � � unsigned int cmd, unsigned long arg)
> > +static long cache_ioctl_procfs(struct file *filp,
> > + � � � � � � � � � � � � � � �unsigned int cmd, unsigned long arg)
> > �{
> > - � � � struct cache_detail *cd = PDE(inode)->data;
> > + � � � long ret;
> > + � � � struct cache_detail *cd;
> > + � � � struct inode *inode = filp->f_path.dentry->d_inode;
> >
> > - � � � return cache_ioctl(inode, filp, cmd, arg, cd);
> > + � � � /* Pushed down from procfs ioctl handler */
> > + � � � lock_kernel();
> > +
> > + � � � cd = PDE(inode)->data;
>
> ->data is not under BKL at all.



Yeah. It's a very rough pushdown, I haven't looked at any bit to figure
out what could need to be protected or not. I even did not know what does
PDE. So I kept a plain bkl path. I just thought any further thinking should
be done in a further patch.

But I can move the bkl after in this same patch if you prefer.

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 30 Mar 2010 06:40
On Tuesday 30 March 2010, Frederic Weisbecker wrote:
> Push down the bkl from procfs's ioctl main handler to its users.
> Only three procfs users implement an ioctl (non unlocked) handler.
> Turn them into unlocked_ioctl and push down the Devil inside.

Looks good to me. I would have used a single unlock and return statement
in i8k_ioctl and isdn_divert_ioctl, with goto instead of adding an
unlock to each return, but it doesn't matter much.

Acked-by: Arnd Bergmann <arnd(a)arndb.de>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 30 Mar 2010 14:40
On Tue, Mar 30, 2010 at 11:37:27AM +0100, Arnd Bergmann wrote:
> On Tuesday 30 March 2010, Frederic Weisbecker wrote:
> > Push down the bkl from procfs's ioctl main handler to its users.
> > Only three procfs users implement an ioctl (non unlocked) handler.
> > Turn them into unlocked_ioctl and push down the Devil inside.
>
> Looks good to me. I would have used a single unlock and return statement
> in i8k_ioctl and isdn_divert_ioctl, with goto instead of adding an
> unlock to each return, but it doesn't matter much.


I did that first, but actually that didn't make much difference:

ret = foo; unlock_kernel()
goto end; VS return foo;




>
> Acked-by: Arnd Bergmann <arnd(a)arndb.de>


Thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 30 Mar 2010 15:00
On Tuesday 30 March 2010 20:27:12 Frederic Weisbecker wrote:
> On Tue, Mar 30, 2010 at 11:37:27AM +0100, Arnd Bergmann wrote:
> > On Tuesday 30 March 2010, Frederic Weisbecker wrote:
> > > Push down the bkl from procfs's ioctl main handler to its users.
> > > Only three procfs users implement an ioctl (non unlocked) handler.
> > > Turn them into unlocked_ioctl and push down the Devil inside.
> >
> > Looks good to me. I would have used a single unlock and return statement
> > in i8k_ioctl and isdn_divert_ioctl, with goto instead of adding an
> > unlock to each return, but it doesn't matter much.
>
>
> I did that first, but actually that didn't make much difference:
>
> ret = foo; unlock_kernel()
> goto end; VS return foo;

Yes, the amount of code needed is comparable, but it is much easier
to validate that you did not miss an unlock when you know that there
is a single return statement in the function. It also helps the next
person that may want to replace the BKL with a different lock.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: [PATCH 6/6] procfs: Kill the bkl in ioctl
Next: procfs: Kill BKL in llseek on proc base