linux-kernel - Re: [patch 09/22] pollfs: pollable hrtimers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1178140567.2340.16.camel@localhost.localdomain>
Date:	Wed, 02 May 2007 23:16:07 +0200
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Davi Arnaut <davi@...ent.com.br>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Davide Libenzi <davidel@...ilserver.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [patch 09/22] pollfs: pollable hrtimers

On Wed, 2007-05-02 at 02:22 -0300, Davi Arnaut wrote:
> plain text document attachment (pollfs-timer.patch)
> Per file descriptor high-resolution timers. A classic unix file interface for
> the POSIX timer_(create|settime|gettime|delete) family of functions.
> 
> Signed-off-by: Davi E. M. Arnaut <davi@...ent.com.br>

Nacked-by-me.

Aside of the fact, that it is a bad clone of the timerfd code, it is
simply broken and untested.

> +
> +struct hrtimerspec {
> +	int flags;
> +	clockid_t clock;
> +	struct itimerspec expr;
> +};

How exactly knows userspace what a struct hrtimerspec is ? Is the c file
exported as a header ?

> +static ssize_t read(struct pfs_timer *evs, struct itimerspec __user *uspec)
> +{
> +	int ret = -EAGAIN;
> +	ktime_t remaining = {};
> +	unsigned long overruns = 0;
> +	struct itimerspec spec = {};
> +	struct hrtimer *timer = &evs->timer;
> +
> +	spin_lock_irq(&evs->lock);
> +
> +	if (!evs->overruns)
> +		goto out_unlock;
> +
> +	if (hrtimer_active(timer))
> +		remaining = hrtimer_get_remaining(timer);
> +	else if (evs->interval.tv64 > 0)
> +		overruns = hrtimer_forward(timer, hrtimer_cb_get_time(timer),
> +					   evs->interval);

Where is the logic here ? 

If no overrun, return remaining time = 0

If active, return the real remaining time. This path is never hit, as
the timer is nowhere restarted.

If not active, return remanining time = 0. How does the caller know how
many events are missed ? 

> +	ret = -EOVERFLOW;
> +	if (overruns > (ULONG_MAX - evs->overruns))
> +		goto out_unlock;
> +	else
> +		evs->overruns += overruns;

Interesting feature. evs->overruns is adding up forever and then limited
to ULONG_MAX

> +static enum hrtimer_restart timer_fn(struct hrtimer *timer)
> +{
> +	struct pfs_timer *evs = container_of(timer, struct pfs_timer, timer);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&evs->lock, flags);
> +	/* timer tick, interval has elapsed */
> +	if (!evs->overruns++)
> +		wake_up_all(&evs->wait);

Cool. Waiters, which came after the first event are stuck. Simply
because there is no second event.

> +static ssize_t write(struct pfs_timer *evs,
> +		     const struct hrtimerspec __user *uspec)
> +{
> +	struct hrtimerspec spec;

See first comment !

> +	if (copy_from_user(&spec, uspec, sizeof(spec)))
> +		return -EFAULT;
> +
> +	if (spec_invalid(&spec))
> +		return -EINVAL;
> +
> +	rearm_timer(evs, &spec);
> +
> +	return 0;
> +}
> +
> +static int poll(struct pfs_timer *evs)
> +{
> +	int ret;
> +
> +	ret = evs->overruns ? POLLIN : 0;
> +
> +	return ret;
> +}

Creative lockless programming style with 4 lines overhead and a
guaranteed return POLLIN after the first timer event. This is really
cute as it covers the missing timer restart and guarantees 100% CPU load
for ever. Hmm, maybe it's correct: polling should loop for ever,
shouldn't it ?

> +static const struct pfs_operations timer_ops = {
> +	.read = PFS_READ(read, struct pfs_timer, struct itimerspec),
> +	.write = PFS_WRITE(write, struct pfs_timer, struct hrtimerspec),
> +	.poll = PFS_POLL(poll, struct pfs_timer),
> +	.release = PFS_RELEASE(release, struct pfs_timer),
> +	.rsize = sizeof(struct itimerspec),
> +	.wsize = sizeof(struct hrtimerspec),

See first comment !

	tglx



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