Messages in this thread | | | Date | Mon, 1 Feb 2010 18:36:17 -0200 | From | Thadeu Lima de Souza Cascardo <> | Subject | Re: [PATCH] irda: remove BKL from irnet open function |
| |
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. [unhandled content-type:application/pgp-signature] | |