Re: [patch 6/9] signalfd/timerfd - timerfd core ...

[Thomas Gleixner]

Davide,

On Sat, 2007-03-10 at 18:22 -0800, Davide Libenzi wrote:

Some remarks:

> +
> +asmlinkage long sys_timerfd(int ufd, int clockid, int tmrtype,
> +			    const struct timespec __user *utmr)
> +{
> +	int error;
> +	struct timerfd_ctx *ctx;
> +	struct file *file;
> +	struct inode *inode;
> +	ktime_t tval, tnow;
> +	struct timespec ktmr, tmrnow;
> +
> +	error = -EFAULT;
> +	if (copy_from_user(&ktmr, utmr, sizeof(ktmr)))
> +		goto err_exit;

Please do not use goto for a simple
		return -EFAULT;

Please validate the timespec before converting it.

        if (!timespec_valid(&ktmr))
                return -EINVAL;


> +	tval = timespec_to_ktime(ktmr);
> +	error = -EINVAL;
> +	if (clockid != CLOCK_MONOTONIC &&
> +	    clockid != CLOCK_REALTIME)
> +		goto err_exit;
> +	switch (tmrtype) {
> +	case TFD_TIMER_REL:
> +	case TFD_TIMER_SEQ:
> +		break;
> +	case TFD_TIMER_ABS:
> +		getnstimeofday(&tmrnow);
> +		tnow = timespec_to_ktime(tmrnow);

		tnow = ktime_get();

> +		if (ktime_to_ns(tval) <= ktime_to_ns(tnow))
> +			goto err_exit;
> +		tval = ktime_sub(tval, tnow);

Why do you want to do that ? hrtimers handle relative and absolute
expiry times. You break down everything to relative time and lose the
accuracy for absolute timers. 

> +		break;
> +	default:
> +		goto err_exit;
> +	}
> +
> +	if (ufd == -1) {
> +		error = -ENOMEM;
> +		ctx = kmem_cache_alloc(timerfd_ctx_cachep, GFP_KERNEL);
> +		if (!ctx)
> +			goto err_exit;
> +
> +		init_waitqueue_head(&ctx->wqh);
> +		spin_lock_init(&ctx->lock);
> +		ctx->ticks = 0;
> +		ctx->tmrtype = tmrtype;
> +		ctx->clockid = clockid;
> +		ctx->tval = tval;
> +		hrtimer_init(&ctx->tmr, ctx->clockid, HRTIMER_REL);
> +		ctx->tmr.expires = ctx->tval;
> +		ctx->tmr.function = timerfd_tmrproc;
> +
> +		hrtimer_start(&ctx->tmr, ctx->tval, HRTIMER_REL);
> +
> +		/*
> +		 * When we call this, the initialization must be complete, since
> +		 * aino_getfd() will install the fd.
> +		 */
> +		error = aino_getfd(&ufd, &inode, &file, "[timerfd]",
> +				   &timerfd_fops, ctx);
> +		if (error)
> +			goto err_fdalloc;

Why is the timer started before we have everything in place ? 

Also if you turn it around then the (re)programming part of the timer
can be shared.

> +	} else {
> +		error = -EBADF;
> +		file = fget(ufd);
> +		if (!file)
> +			goto err_exit;
> +		ctx = file->private_data;
> +		error = -EINVAL;
> +		if (file->f_op != &timerfd_fops) {
> +			fput(file);
> +			goto err_exit;
> +		}
> +
> +		/*
> +		 * We need to stop the exiting timer before. We call
> +		 * hrtimer_cancel() w/out holding our lock.
> +		 */
> +		spin_lock_irq(&ctx->lock);
> +		while (hrtimer_active(&ctx->tmr)) {
> +			spin_unlock_irq(&ctx->lock);
> +			hrtimer_cancel(&ctx->tmr);
> +			spin_lock_irq(&ctx->lock);
> +		}

Please use hrtimer_try_to_cancel()

retry:
	spin_lock_irq(....):
	if (hrtimer_try_to_cancel(&ctx->tmr) < 0) {
		spin_unlock_irq();
		cpu_relax();
		goto retry;
	}

> +
> +static unsigned int timerfd_poll(struct file *file, poll_table *wait)
> +{
> +	struct timerfd_ctx *ctx = file->private_data;
> +
> +	poll_wait(file, &ctx->wqh, wait);
> +
> +	return ctx->ticks ? POLLIN: 0;

This is racy:

	timer is set up (non periodic)
	timer expires
	poll 

	now poll is stuck for ever !


	tglx


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/