[PATCH] irda: remove BKL from irnet open function

[PATCH] irda: remove BKL from irnet open function

[Thadeu Lima de Souza Cascardo]

Commit cddf63d99d0d145f18b293c3d0de4af7dab2a922 has push down the BKL
into irnet open function. However, there's nothing that needs locking in
there.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
---
 net/irda/irnet/irnet_ppp.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/net/irda/irnet/irnet_ppp.c b/net/irda/irnet/irnet_ppp.c
index 156020d..d6b502c 100644
--- a/net/irda/irnet/irnet_ppp.c
+++ b/net/irda/irnet/irnet_ppp.c
@@ -479,7 +479,6 @@ dev_irnet_open(struct inode *	inode,
   ap = kzalloc(sizeof(*ap), GFP_KERNEL);
   DABORT(ap == NULL, -ENOMEM, FS_ERROR, "Can't allocate struct irnet...\n");
 
-  lock_kernel();
   /* initialize the irnet structure */
   ap->file = file;
 
@@ -501,7 +500,6 @@ dev_irnet_open(struct inode *	inode,
     {
       DERROR(FS_ERROR, "Can't setup IrDA link...\n");
       kfree(ap);
-      unlock_kernel();
       return err;
     }
 
@@ -512,7 +510,6 @@ dev_irnet_open(struct inode *	inode,
   file->private_data = ap;
 
   DEXIT(FS_TRACE, " - ap=0x%p\n", ap);
-  unlock_kernel();
   return 0;
 }
 
-- 
1.6.6.1

Re: [PATCH] irda: remove BKL from irnet open function

[John Kacur]

On Mon, Feb 1, 2010 at 7:18 PM, Thadeu Lima de Souza Cascardo
<cascardo@holoscopio.com> wrote:
> Commit cddf63d99d0d145f18b293c3d0de4af7dab2a922 has push down the BKL
> into irnet open function. However, there's nothing that needs locking in
> there.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
> ---
>  net/irda/irnet/irnet_ppp.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
>
> diff --git a/net/irda/irnet/irnet_ppp.c b/net/irda/irnet/irnet_ppp.c
> index 156020d..d6b502c 100644
> --- a/net/irda/irnet/irnet_ppp.c
> +++ b/net/irda/irnet/irnet_ppp.c
> @@ -479,7 +479,6 @@ dev_irnet_open(struct inode *       inode,
>   ap = kzalloc(sizeof(*ap), GFP_KERNEL);
>   DABORT(ap == NULL, -ENOMEM, FS_ERROR, "Can't allocate struct irnet...\n");
>
> -  lock_kernel();
>   /* initialize the irnet structure */
>   ap->file = file;
>
> @@ -501,7 +500,6 @@ dev_irnet_open(struct inode *       inode,
>     {
>       DERROR(FS_ERROR, "Can't setup IrDA link...\n");
>       kfree(ap);
> -      unlock_kernel();
>       return err;
>     }
>
> @@ -512,7 +510,6 @@ dev_irnet_open(struct inode *       inode,
>   file->private_data = ap;
>
>   DEXIT(FS_TRACE, " - ap=0x%p\n", ap);
> -  unlock_kernel();
>   return 0;
>  }
>
> --
> 1.6.6.1

This is probably NOT safe to do, because the BKL is synchronizing the
ioctl code.

Thanks

Re: [PATCH] irda: remove BKL from irnet open function

[Thadeu Lima de Souza Cascardo]

[-- Attachment #1: Type: text/plain, Size: 2412 bytes --]

On Mon, Feb 01, 2010 at 09:32:30PM +0100, John Kacur wrote:
> On Mon, Feb 1, 2010 at 7:18 PM, Thadeu Lima de Souza Cascardo
> <cascardo@holoscopio.com> wrote:
> > Commit cddf63d99d0d145f18b293c3d0de4af7dab2a922 has push down the BKL
> > into irnet open function. However, there's nothing that needs locking in
> > there.
> >
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
> > ---
> >  net/irda/irnet/irnet_ppp.c |    3 ---
> >  1 files changed, 0 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/irda/irnet/irnet_ppp.c b/net/irda/irnet/irnet_ppp.c
> > index 156020d..d6b502c 100644
> > --- a/net/irda/irnet/irnet_ppp.c
> > +++ b/net/irda/irnet/irnet_ppp.c
> > @@ -479,7 +479,6 @@ dev_irnet_open(struct inode *       inode,
> >   ap = kzalloc(sizeof(*ap), GFP_KERNEL);
> >   DABORT(ap == NULL, -ENOMEM, FS_ERROR, "Can't allocate struct irnet...\n");
> >
> > -  lock_kernel();
> >   /* initialize the irnet structure */
> >   ap->file = file;
> >
> > @@ -501,7 +500,6 @@ dev_irnet_open(struct inode *       inode,
> >     {
> >       DERROR(FS_ERROR, "Can't setup IrDA link...\n");
> >       kfree(ap);
> > -      unlock_kernel();
> >       return err;
> >     }
> >
> > @@ -512,7 +510,6 @@ dev_irnet_open(struct inode *       inode,
> >   file->private_data = ap;
> >
> >   DEXIT(FS_TRACE, " - ap=0x%p\n", ap);
> > -  unlock_kernel();
> >   return 0;
> >  }
> >
> > --
> > 1.6.6.1
> 
> This is probably NOT safe to do, because the BKL is synchronizing the
> ioctl code.
> 
> Thanks

And is it possible that ioctl will be called before open returns? If it
is, then, yes, this is not safe. But I don't really believe the case. Or
is it?

Or is it only possible to happen with different struct file*? In that
case, open is only allocating and initializing the irnet_socket *ap.
Then, ioctl uses it. There is some race between the different ioctls,
but no race between open/ioctl for different opened devices. That is, a
process may open /dev/irnet while another process is issuing ioctls to
its own opened /dev/irnet.

Besides, dev_irnet_ioctl uses the file private_data to get to the
irnet_socket, which is the last thing the open call does. I assume doing
an attribution to a pointer is atomic in all architectures supported by
Linux currently, isn't it?

Regards,
Cascardo.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] irda: remove BKL from irnet open function

[Arnd Bergmann]

On Monday 01 February 2010, Thadeu Lima de Souza Cascardo wrote:
> On Mon, Feb 01, 2010 at 09:32:30PM +0100, John Kacur wrote:
> > On Mon, Feb 1, 2010 at 7:18 PM, Thadeu Lima de Souza Cascardo
> > <cascardo@holoscopio.com> wrote:
> And is it possible that ioctl will be called before open returns? If it
> is, then, yes, this is not safe. But I don't really believe the case. Or
> is it?

ioctl may be called on an open file descriptor while open is called
by another thread to open a second file descriptor for the same device.

> Or is it only possible to happen with different struct file*? In that
> case, open is only allocating and initializing the irnet_socket *ap.
> Then, ioctl uses it. There is some race between the different ioctls,
> but no race between open/ioctl for different opened devices. That is, a
> process may open /dev/irnet while another process is issuing ioctls to
> its own opened /dev/irnet.

right.
 
> Besides, dev_irnet_ioctl uses the file private_data to get to the
> irnet_socket, which is the last thing the open call does. I assume doing
> an attribution to a pointer is atomic in all architectures supported by
> Linux currently, isn't it?

The pointer assignment is atomic, but it may not be synchronized to data
pointed to it. On Alpha (probably no others so far), this would result
in irnet_socket seen as uninitialized after the pointer to is can be
seen as valid if there was no locking. No architecture would read an
invalid pointer though.

I guess that what John was trying to point out actually is the fact that
you shouldn't really do the BKL removal in one function only but rather
do it for the whole driver at once. The irnet driver uses the BKL in
open, ioctl and llseek, so if you want to clean up that driver, please
introduce proper locking in the driver and do all of the three.

	Arnd