linux-kernel - Re: [patch 10/13] signal/timer/event fds v8

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]

Date:	Fri, 30 Mar 2007 18:11:55 -0700 (PDT)
From:	Davide Libenzi <davidel@...ilserver.org>
To:	Andrew Morton <akpm@...ux-foundation.org>
cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>,
	Suparna Bhattacharya <suparna@...ibm.com>,
	Zach Brown <zach.brown@...cle.com>,
	Benjamin LaHaise <bcrl@...ck.org>
Subject: Re: [patch 10/13] signal/timer/event fds v8 - eventfd core ...

On Fri, 30 Mar 2007, Andrew Morton wrote:

> > +struct eventfd_ctx {
> > +	spinlock_t lock;
> > +	wait_queue_head_t wqh;
> > +	__u64 count;
> > +};
> 
> Again, can we borrow wqh.lock?
> 
> `count' needs documentation - these things are key to understanding the
> code.

Added.



> > + */
> 
> So it is the caller's responsibility to ensure that *file refers to an
> eventfd file?

In which function? I lost you ...



> > +int eventfd_signal(struct file *file, int n)
> > +{
> > +	struct eventfd_ctx *ctx = file->private_data;
> > +	unsigned long flags;
> > +
> > +	if (n < 0)
> > +		return -EINVAL;
> > +	spin_lock_irqsave(&ctx->lock, flags);
> > +	if (ULLONG_MAX - ctx->count < n)
> > +		n = (int) (ULLONG_MAX - ctx->count);
> > +	ctx->count += n;
> > +	if (waitqueue_active(&ctx->wqh))
> > +		wake_up_locked(&ctx->wqh);
> > +	spin_unlock_irqrestore(&ctx->lock, flags);
> > +
> > +	return n;
> > +}
> 
> Neither the incoming arg (usefully named "n") nor the return value are
> documented.

Documented now.




> Needs interface documentation, please.  Even the changelog doesn't tell us
> what an EAGAIN return from read() means.

I'll be adding the errno documentation to all of them.




> > +static ssize_t eventfd_write(struct file *file, const char __user *buf, size_t count,
> > +			     loff_t *ppos)
> > +{
> > +	struct eventfd_ctx *ctx = file->private_data;
> > +	ssize_t res;
> > +	__u64 ucnt;
> > +	DECLARE_WAITQUEUE(wait, current);
> > +
> > +	if (count < sizeof(ucnt))
> > +		return -EINVAL;
> > +	if (get_user(ucnt, (const __u64 __user *) buf))
> > +		return -EFAULT;
> 
> Some architectures do not implement 64-bit get_user()

copy_from_user it is, then ...



- Davide


-
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/