[patch 4/4] mISDN: misc timerdev fixes

[patch 4/4] mISDN: misc timerdev fixes

[akpm]

From: Andi Kleen <andi@firstfloor.org>

- Remove noop VFS stubs. The VFS does that on a NULL pointer anyways.
- Fix timer handler prototype to be correct
- Comment ugly SMP race I didn't fix.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
Cc: Karsten Keil <kkeil@suse.de>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/isdn/mISDN/timerdev.c |   22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff -puN drivers/isdn/mISDN/timerdev.c~misdn-misc-timerdev-fixes drivers/isdn/mISDN/timerdev.c
--- a/drivers/isdn/mISDN/timerdev.c~misdn-misc-timerdev-fixes
+++ a/drivers/isdn/mISDN/timerdev.c
@@ -124,18 +124,6 @@ mISDN_read(struct file *filep, char *buf
 	return ret;
 }
 
-static loff_t
-mISDN_llseek(struct file *filep, loff_t offset, int orig)
-{
-	return -ESPIPE;
-}
-
-static ssize_t
-mISDN_write(struct file *filep, const char *buf, size_t count, loff_t *off)
-{
-	return -EOPNOTSUPP;
-}
-
 static unsigned int
 mISDN_poll(struct file *filep, poll_table *wait)
 {
@@ -157,8 +145,9 @@ mISDN_poll(struct file *filep, poll_tabl
 }
 
 static void
-dev_expire_timer(struct mISDNtimer *timer)
+dev_expire_timer(unsigned long data)
 {
+	struct mISDNtimer *timer = (void *)data;
 	u_long			flags;
 
 	spin_lock_irqsave(&timer->dev->lock, flags);
@@ -191,7 +180,7 @@ misdn_add_timer(struct mISDNtimerdev *de
 		spin_unlock_irqrestore(&dev->lock, flags);
 		timer->dev = dev;
 		timer->tl.data = (long)timer;
-		timer->tl.function = (void *) dev_expire_timer;
+		timer->tl.function = dev_expire_timer;
 		init_timer(&timer->tl);
 		timer->tl.expires = jiffies + ((HZ * (u_long)timeout) / 1000);
 		add_timer(&timer->tl);
@@ -211,6 +200,9 @@ misdn_del_timer(struct mISDNtimerdev *de
 	list_for_each_entry(timer, &dev->pending, list) {
 		if (timer->id == id) {
 			list_del_init(&timer->list);
+			/* RED-PEN AK: race -- timer can be still running on
+			 * other CPU. Needs reference count I think
+ 		 	 */
 			del_timer(&timer->tl);
 			ret = timer->id;
 			kfree(timer);
@@ -268,9 +260,7 @@ mISDN_ioctl(struct inode *inode, struct 
 }
 
 static struct file_operations mISDN_fops = {
-	.llseek		= mISDN_llseek,
 	.read		= mISDN_read,
-	.write		= mISDN_write,
 	.poll		= mISDN_poll,
 	.ioctl		= mISDN_ioctl,
 	.open		= mISDN_open,
_

Re: [patch 4/4] mISDN: misc timerdev fixes

[David Miller]

From: akpm@linux-foundation.org
Date: Mon, 22 Sep 2008 14:51:03 -0700

> - Remove noop VFS stubs. The VFS does that on a NULL pointer anyways.
> - Fix timer handler prototype to be correct
> - Comment ugly SMP race I didn't fix.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Applied to net-next-2.6

Re: [patch 4/4] mISDN: misc timerdev fixes

[Andi Kleen]

On Mon, Sep 22, 2008 at 07:18:26PM -0700, David Miller wrote:
> From: akpm@linux-foundation.org
> Date: Mon, 22 Sep 2008 14:51:03 -0700
> 
> > - Remove noop VFS stubs. The VFS does that on a NULL pointer anyways.
> > - Fix timer handler prototype to be correct
> > - Comment ugly SMP race I didn't fix.
> > 
> > Signed-off-by: Andi Kleen <ak@linux.intel.com>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> 
> Applied to net-next-2.6

I'm hoping someone takes care of the SMP race. I think the timers
need all reference counting similar to other network objects to
handle this cleanly.

It might be a good idea to mark it BROKEN_ON_SMP in the meantime.

-Andi

-- 
ak@linux.intel.com

Re: [patch 4/4] mISDN: misc timerdev fixes

[Karsten Keil]

On Tue, Sep 23, 2008 at 04:27:32AM +0200, Andi Kleen wrote:
> On Mon, Sep 22, 2008 at 07:18:26PM -0700, David Miller wrote:
> > From: akpm@linux-foundation.org
> > Date: Mon, 22 Sep 2008 14:51:03 -0700
> > 
> > > - Remove noop VFS stubs. The VFS does that on a NULL pointer anyways.
> > > - Fix timer handler prototype to be correct
> > > - Comment ugly SMP race I didn't fix.
> > > 
> > > Signed-off-by: Andi Kleen <ak@linux.intel.com>
> > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > 
> > Applied to net-next-2.6
> 
> I'm hoping someone takes care of the SMP race. I think the timers
> need all reference counting similar to other network objects to
> handle this cleanly.

Yes I already have some idea without refcounting, checking how much time is left
if it is more as some treshhold (maybe 2 or 10 jiffies) delete the timer,
if not mark it for deletion only and delete it during the run function without
triggering the device. In the race case it would trigger the device too but
this is not critical.
Another easier implementation could mark it only, without trying to delete
it imediately. But in this case unneeded timers would hang around (on big busy PBX
boxes this can be some 100) for some time.

What do you think ?

> 
> It might be a good idea to mark it BROKEN_ON_SMP in the meantime.

No really needed, normally the timers deleted long time before running out
or never so it should never see such a race, maybe if the application is
aborted and cleanup all timers this could happen but also this should be no
normal case, since usually you do not kill a PBX if any connections are
active.
Yes it could be used to do some deny on service kind of attack, so it should
be fixed at all.


-- 
Karsten Keil
SuSE Labs
ISDN and VOIP development
SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg)

Re: [patch 4/4] mISDN: misc timerdev fixes

[Christoph Hellwig]

On Mon, Sep 22, 2008 at 02:51:03PM -0700, akpm@linux-foundation.org wrote:
> From: Andi Kleen <andi@firstfloor.org>
> 
> - Remove noop VFS stubs. The VFS does that on a NULL pointer anyways.

> -static loff_t
> -mISDN_llseek(struct file *filep, loff_t offset, int orig)
> -{
> -	return -ESPIPE;
> -}

>  static struct file_operations mISDN_fops = {
> -	.llseek		= mISDN_llseek,

This is wrong.  no llseek means we use default_llseek, which is
different from returning -ESPIPE.

Re: [patch 4/4] mISDN: misc timerdev fixes

[Andrew Morton]

On Tue, 23 Sep 2008 06:40:26 -0400 Christoph Hellwig <hch@infradead.org> wrote:

> On Mon, Sep 22, 2008 at 02:51:03PM -0700, akpm@linux-foundation.org wrote:
> > From: Andi Kleen <andi@firstfloor.org>
> > 
> > - Remove noop VFS stubs. The VFS does that on a NULL pointer anyways.
> 
> > -static loff_t
> > -mISDN_llseek(struct file *filep, loff_t offset, int orig)
> > -{
> > -	return -ESPIPE;
> > -}
> 
> >  static struct file_operations mISDN_fops = {
> > -	.llseek		= mISDN_llseek,
> 
> This is wrong.  no llseek means we use default_llseek, which is
> different from returning -ESPIPE.

so.. this?

--- a/drivers/isdn/mISDN/timerdev.c~misdn-misc-timerdev-fixes-fix
+++ a/drivers/isdn/mISDN/timerdev.c
@@ -61,7 +61,7 @@ mISDN_open(struct inode *ino, struct fil
 	init_waitqueue_head(&dev->wait);
 	filep->private_data = dev;
 	__module_get(THIS_MODULE);
-	return 0;
+	return nonseekable_open(ino, filep);
 }
 
 static int
_

Re: [patch 4/4] mISDN: misc timerdev fixes

[Christoph Hellwig]

On Tue, Sep 23, 2008 at 04:30:05AM -0700, Andrew Morton wrote:
> so.. this?

Much better.

> --- a/drivers/isdn/mISDN/timerdev.c~misdn-misc-timerdev-fixes-fix
> +++ a/drivers/isdn/mISDN/timerdev.c
> @@ -61,7 +61,7 @@ mISDN_open(struct inode *ino, struct fil
>  	init_waitqueue_head(&dev->wait);
>  	filep->private_data = dev;
>  	__module_get(THIS_MODULE);
> -	return 0;
> +	return nonseekable_open(ino, filep);

But this also shows that mISDN is kinda stuck in a different century.
Doing __module_get(THIS_MODULE) at the end of ->open is utterly racy,
it really needs to set a owner field in file_operations and rip this
cruft out.

Btw, can anyone explain WTF this timerdev module is doing?  It's not
using any functionality from the rest of mISDN, it's not exporting
any functionality to it either but just provides a really awkward way
to expose dumb timers to userspace.  What does it provide that the
normal timer syscalls can't provide?

Re: [patch 4/4] mISDN: misc timerdev fixes

[Christoph Hellwig]

On Tue, Sep 23, 2008 at 08:34:07AM +0200, Karsten Keil wrote:
> No really needed, normally the timers deleted long time before running out
> or never so it should never see such a race, maybe if the application is
> aborted and cleanup all timers this could happen but also this should be no
> normal case, since usually you do not kill a PBX if any connections are
> active.
> Yes it could be used to do some deny on service kind of attack, so it should
> be fixed at all.

All these timers are completely user controller and non-prviliegued.  So
this _is_ a serious issue.  In fact I think we should just rip out this
whole module, it's a really useless duplicate timer interface.

Gotta love merging unreviewed code.

Re: [patch 4/4] mISDN: misc timerdev fixes

[Karsten Keil]

On Tue, Sep 23, 2008 at 07:45:34AM -0400, Christoph Hellwig wrote:
> On Tue, Sep 23, 2008 at 04:30:05AM -0700, Andrew Morton wrote:
> > so.. this?
> 
> Much better.
> 
> > --- a/drivers/isdn/mISDN/timerdev.c~misdn-misc-timerdev-fixes-fix
> > +++ a/drivers/isdn/mISDN/timerdev.c
> > @@ -61,7 +61,7 @@ mISDN_open(struct inode *ino, struct fil
> >  	init_waitqueue_head(&dev->wait);
> >  	filep->private_data = dev;
> >  	__module_get(THIS_MODULE);
> > -	return 0;
> > +	return nonseekable_open(ino, filep);
> 
> But this also shows that mISDN is kinda stuck in a different century.
> Doing __module_get(THIS_MODULE) at the end of ->open is utterly racy,
> it really needs to set a owner field in file_operations and rip this
> cruft out.
> 
> Btw, can anyone explain WTF this timerdev module is doing?  It's not
> using any functionality from the rest of mISDN, it's not exporting
> any functionality to it either but just provides a really awkward way
> to expose dumb timers to userspace.  What does it provide that the
> normal timer syscalls can't provide?

This version only makes the programing of upper ISDN layers easier,
you only need to watch /dev/mISDNtimer together with the sockets in one
select call.
The next version will have a option to synchronise the timer with
the ISDN hardware clock which would avoid additional jitter if you need to
bridge channels in software.

-- 
Karsten Keil
SuSE Labs
ISDN and VOIP development
SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg)

Re: [patch 4/4] mISDN: misc timerdev fixes

[Andi Kleen]

On Tue, Sep 23, 2008 at 06:40:26AM -0400, Christoph Hellwig wrote:
> On Mon, Sep 22, 2008 at 02:51:03PM -0700, akpm@linux-foundation.org wrote:
> > From: Andi Kleen <andi@firstfloor.org>
> > 
> > - Remove noop VFS stubs. The VFS does that on a NULL pointer anyways.
> 
> > -static loff_t
> > -mISDN_llseek(struct file *filep, loff_t offset, int orig)
> > -{
> > -	return -ESPIPE;
> > -}
> 
> >  static struct file_operations mISDN_fops = {
> > -	.llseek		= mISDN_llseek,
> 
> This is wrong.  no llseek means we use default_llseek, which is
> different from returning -ESPIPE.

No it's correct. Character devices don't have FMODE_LSEEK set 
and then no_llseek is the default fallback.

-Andi
-- 
ak@linux.intel.com

Re: [patch 4/4] mISDN: misc timerdev fixes

[Andi Kleen]

On Tue, Sep 23, 2008 at 04:30:05AM -0700, Andrew Morton wrote:
> On Tue, 23 Sep 2008 06:40:26 -0400 Christoph Hellwig <hch@infradead.org> wrote:
> 
> > On Mon, Sep 22, 2008 at 02:51:03PM -0700, akpm@linux-foundation.org wrote:
> > > From: Andi Kleen <andi@firstfloor.org>
> > > 
> > > - Remove noop VFS stubs. The VFS does that on a NULL pointer anyways.
> > 
> > > -static loff_t
> > > -mISDN_llseek(struct file *filep, loff_t offset, int orig)
> > > -{
> > > -	return -ESPIPE;
> > > -}
> > 
> > >  static struct file_operations mISDN_fops = {
> > > -	.llseek		= mISDN_llseek,
> > 
> > This is wrong.  no llseek means we use default_llseek, which is
> > different from returning -ESPIPE.
> 
> so.. this?

No, Christoph was wrong on that one. The original patch is ok.
He would have been right if that was a file system, but it isn't.

-Andi

Re: [patch 4/4] mISDN: misc timerdev fixes II

[Andi Kleen]

On Tue, Sep 23, 2008 at 04:30:05AM -0700, Andrew Morton wrote:
> On Tue, 23 Sep 2008 06:40:26 -0400 Christoph Hellwig <hch@infradead.org> wrote:
> 
> > On Mon, Sep 22, 2008 at 02:51:03PM -0700, akpm@linux-foundation.org wrote:
> > > From: Andi Kleen <andi@firstfloor.org>
> > > 
> > > - Remove noop VFS stubs. The VFS does that on a NULL pointer anyways.
> > 
> > > -static loff_t
> > > -mISDN_llseek(struct file *filep, loff_t offset, int orig)
> > > -{
> > > -	return -ESPIPE;
> > > -}
> > 
> > >  static struct file_operations mISDN_fops = {
> > > -	.llseek		= mISDN_llseek,
> > 
> > This is wrong.  no llseek means we use default_llseek, which is
> > different from returning -ESPIPE.
> 
> so.. this?

Hmm actually on double checking it's really needed, sorry.

-Andi

Re: [patch 4/4] mISDN: misc timerdev fixes II

[Christoph Hellwig]

On Tue, Sep 23, 2008 at 04:17:40PM +0200, Andi Kleen wrote:
> > > >  static struct file_operations mISDN_fops = {
> > > > -	.llseek		= mISDN_llseek,
> > > 
> > > This is wrong.  no llseek means we use default_llseek, which is
> > > different from returning -ESPIPE.
> > 
> > so.. this?
> 
> Hmm actually on double checking it's really needed, sorry.

Yes, unfortunately the only way it's cleared currently is through using
nonseekable_open, but I've started preparing a war plan to sort this whole
mess out.

Re: [patch 4/4] mISDN: misc timerdev fixes II

[Andi Kleen]

On Tue, Sep 23, 2008 at 11:58:44AM -0400, Christoph Hellwig wrote:
> On Tue, Sep 23, 2008 at 04:17:40PM +0200, Andi Kleen wrote:
> > > > >  static struct file_operations mISDN_fops = {
> > > > > -	.llseek		= mISDN_llseek,
> > > > 
> > > > This is wrong.  no llseek means we use default_llseek, which is
> > > > different from returning -ESPIPE.
> > > 
> > > so.. this?
> > 
> > Hmm actually on double checking it's really needed, sorry.
> 
> Yes, unfortunately the only way it's cleared currently is through using
> nonseekable_open, but I've started preparing a war plan to sort this whole
> mess out.

Yes it would be much more logical if FMODE_SEEK was cleared on character devices
by default. That is what I assumed with the original patch.

-Andi

-- 
ak@linux.intel.com

Re: [patch 4/4] mISDN: misc timerdev fixes II

[Christoph Hellwig]

On Tue, Sep 23, 2008 at 06:46:52PM +0200, Andi Kleen wrote:
> > Yes, unfortunately the only way it's cleared currently is through using
> > nonseekable_open, but I've started preparing a war plan to sort this whole
> > mess out.
> 
> Yes it would be much more logical if FMODE_SEEK was cleared on character devices
> by default. That is what I assumed with the original patch.

Actually I'd prefer to not have by default at all.  While all
filesystems should support seeking they also can easily set a .llseek
instead of the current horrible default.  llseek is the only file
operation with a default (and a really bad one), and I'd prefer it not
to have for consistency.

Re: [patch 4/4] mISDN: misc timerdev fixes

[Andrew Morton]

On Tue, 23 Sep 2008 14:00:30 +0200 Karsten Keil <kkeil@suse.de> wrote:

> On Tue, Sep 23, 2008 at 07:45:34AM -0400, Christoph Hellwig wrote:
> > On Tue, Sep 23, 2008 at 04:30:05AM -0700, Andrew Morton wrote:
> > > so.. this?
> > 
> > Much better.
> > 
> > > --- a/drivers/isdn/mISDN/timerdev.c~misdn-misc-timerdev-fixes-fix
> > > +++ a/drivers/isdn/mISDN/timerdev.c
> > > @@ -61,7 +61,7 @@ mISDN_open(struct inode *ino, struct fil
> > >  	init_waitqueue_head(&dev->wait);
> > >  	filep->private_data = dev;
> > >  	__module_get(THIS_MODULE);
> > > -	return 0;
> > > +	return nonseekable_open(ino, filep);
> > 
> > But this also shows that mISDN is kinda stuck in a different century.
> > Doing __module_get(THIS_MODULE) at the end of ->open is utterly racy,
> > it really needs to set a owner field in file_operations and rip this
> > cruft out.
> > 
> > Btw, can anyone explain WTF this timerdev module is doing?  It's not
> > using any functionality from the rest of mISDN, it's not exporting
> > any functionality to it either but just provides a really awkward way
> > to expose dumb timers to userspace.  What does it provide that the
> > normal timer syscalls can't provide?
> 
> This version only makes the programing of upper ISDN layers easier,
> you only need to watch /dev/mISDNtimer together with the sockets in one
> select call.

sys_timerfd_create() can do this?

> The next version will have a option to synchronise the timer with
> the ISDN hardware clock which would avoid additional jitter if you need to
> bridge channels in software.

hrm.  If that's really really useful and actually works then I guess it
might then be justifiable.

Re: [patch 4/4] mISDN: misc timerdev fixes

[Christoph Hellwig]

The bogus version which removes ->llseek just got into Linus' tree..

Re: [patch 4/4] mISDN: misc timerdev fixes

[Andrew Morton]

> On Sun, 12 Oct 2008 07:05:10 -0400 Christoph Hellwig <hch@infradead.org> wrote:
> 
> The bogus version which removes ->llseek just got into Linus' tree..

Well I still have this queued:

From: Andrew Morton <akpm@linux-foundation.org>

Cc: Andi Kleen <andi@firstfloor.org>
Cc: Karsten Keil <kkeil@suse.de>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/isdn/mISDN/timerdev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -puN drivers/isdn/mISDN/timerdev.c~misdn-misc-timerdev-fixes-fix drivers/isdn/mISDN/timerdev.c
--- a/drivers/isdn/mISDN/timerdev.c~misdn-misc-timerdev-fixes-fix
+++ a/drivers/isdn/mISDN/timerdev.c
@@ -61,7 +61,7 @@ mISDN_open(struct inode *ino, struct fil
 	init_waitqueue_head(&dev->wait);
 	filep->private_data = dev;
 	__module_get(THIS_MODULE);
-	return 0;
+	return nonseekable_open(ino, filep);
 }
 
 static int
_


So presumably whoever merged the unfixed version flubbed the fix.

If that'll fix it.  Is it sufficient?  I think so.

Re: [patch 4/4] mISDN: misc timerdev fixes

[Christoph Hellwig]

On Sun, Oct 12, 2008 at 07:11:13PM -0700, Andrew Morton wrote:
> > On Sun, 12 Oct 2008 07:05:10 -0400 Christoph Hellwig <hch@infradead.org> wrote:
> > 
> > The bogus version which removes ->llseek just got into Linus' tree..
> 
> Well I still have this queued:

Yeah, that'll fix it.

Re: [patch 4/4] mISDN: misc timerdev fixes

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Sun, 12 Oct 2008 19:11:13 -0700

> > On Sun, 12 Oct 2008 07:05:10 -0400 Christoph Hellwig <hch@infradead.org> wrote:
> > 
> > The bogus version which removes ->llseek just got into Linus' tree..
> 
> Well I still have this queued:
> 
> From: Andrew Morton <akpm@linux-foundation.org>
> 
> Cc: Andi Kleen <andi@firstfloor.org>
> Cc: Karsten Keil <kkeil@suse.de>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Andrew, want me to such this into my net-2.6 tree?

Re: [patch 4/4] mISDN: misc timerdev fixes

[Andrew Morton]

> On Sun, 12 Oct 2008 21:00:31 -0700 (PDT) David Miller <davem@davemloft.net> wrote:
> From: Andrew Morton <akpm@linux-foundation.org>
> Date: Sun, 12 Oct 2008 19:11:13 -0700
> 
> > > On Sun, 12 Oct 2008 07:05:10 -0400 Christoph Hellwig <hch@infradead.org> wrote:
> > > 
> > > The bogus version which removes ->llseek just got into Linus' tree..
> > 
> > Well I still have this queued:
> > 
> > From: Andrew Morton <akpm@linux-foundation.org>
> > 
> > Cc: Andi Kleen <andi@firstfloor.org>
> > Cc: Karsten Keil <kkeil@suse.de>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> 
> Andrew, want me to such this into my net-2.6 tree?

yes please.  I'll send it over with a slight changelog.

Re: [patch 4/4] mISDN: misc timerdev fixes

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Mon, 13 Oct 2008 04:56:58 -0700

> > On Sun, 12 Oct 2008 21:00:31 -0700 (PDT) David Miller <davem@davemloft.net> wrote:
> >
> > Andrew, want me to suck this into my net-2.6 tree?
> 
> yes please.  I'll send it over with a slight changelog.

Thanks, will pull those in.