| Message ID | 1265048321-8097-1-git-send-email-cascardo@holoscopio.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-)