diff mbox

[4/4] mISDN: misc timerdev fixes

Message ID 200809222151.m8MLp3tb031906@imap1.linux-foundation.org
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Andrew Morton Sept. 22, 2008, 9:51 p.m. UTC 
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(-)

Comments

David Miller Sept. 23, 2008, 2:18 a.m. UTC | #1 
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
--
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
Andi Kleen Sept. 23, 2008, 2:27 a.m. UTC | #2 
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
Karsten Keil Sept. 23, 2008, 6:34 a.m. UTC | #3 
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.
Christoph Hellwig Sept. 23, 2008, 10:40 a.m. UTC | #4 
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.

--
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
Christoph Hellwig Sept. 23, 2008, 11:49 a.m. UTC | #5 
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.

--
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
Andi Kleen Sept. 23, 2008, 2:13 p.m. UTC | #6 
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
diff mbox

Patch

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,